feat(sdk): Extends mcp protocol support #89
Conversation
- Add comprehensive theme system with Tailwind v4 Browser CDN support - Add platform configurations (OpenAI, Claude, Gemini, Ngrok) with capability detection - Add base layout system with responsive sizing and alignment options - Add UI components: button, card, form, badge, alert, avatar, modal, table, list - Add page templates for consent and error flows - Add OpenAI App SDK resource widgets - Add HTMX support across all components for progressive enhancement - Add script caching for blocked-network platforms (Claude Artifacts) - Add comprehensive tests with 95%+ coverage (327 tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # libs/sdk/src/__test-utils__/fixtures/index.ts # libs/sdk/src/auth/auth.registry.ts # libs/ui/README.md # libs/ui/package.json # libs/ui/src/components/button.test.ts # libs/ui/src/components/button.ts # libs/ui/src/components/index.ts # libs/ui/src/index.ts # libs/ui/src/theme/cdn.ts # libs/ui/src/theme/index.ts # libs/ui/src/theme/theme.test.ts # libs/ui/src/theme/theme.ts
…th session handling
…ription, enhance SSE handling
# Conflicts: # apps/auth/demo-public/src/main.ts # apps/demo/src/main.ts # docs/draft/docs/authentication/local.mdx # docs/draft/docs/authentication/modes.mdx # docs/draft/docs/authentication/overview.mdx # docs/draft/docs/authentication/production.mdx # docs/draft/docs/authentication/progressive.mdx # docs/draft/docs/authentication/remote-proxy.mdx # docs/draft/docs/authentication/remote.mdx # docs/draft/docs/authentication/token.mdx # libs/sdk/src/auth/flows/session.verify.flow.ts # libs/sdk/src/auth/session/transport-session.types.ts # libs/sdk/src/common/entries/scope.entry.ts # libs/sdk/src/common/types/auth/session.types.ts # libs/sdk/src/common/utils/decide-request-intent.utils.ts # libs/sdk/src/scope/flows/http.request.flow.ts # libs/sdk/src/transport/flows/handle.streamable-http.flow.ts # libs/sdk/src/transport/mcp-handlers/index.ts
WalkthroughAdds a per-session NotificationService and notification-related MCP handlers/flows; extends session transport protocol types and SSE handling (legacy /sse path, SSE listener, incremental SSE ids); introduces delete-session HTTP/MCP intent and termination handling; broad tool examples and synonym-expansion search features; multiple new flows and MCP handlers; numerous schema/type updates and large deletions in ast-guard and demo-showcase tests/docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as Request Router
participant Auth as Authorization
participant Flow as HttpRequestFlow
participant Notifier as NotificationService
participant MCP as MCP Handlers
Client->>Router: HTTP DELETE /... or MCP delete-session
Router->>Router: decideIntent(request) -> 'delete-session'?
Router->>Auth: authorize(request)
Auth-->>Router: authInfo (may include sessionId)
Router->>Flow: route -> handleDeleteSession
Flow->>Notifier: terminateSession(sessionId)
Note over Notifier: mark terminated, cleanup subscriptions/servers, invalidate caches
Notifier-->>Flow: termination result (found / not found)
alt session terminated
Flow-->>Client: 204 No Content / empty result
else session not found
Flow-->>Client: 404 Not Found
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
56-57: Typo in method name:paseInputshould beparseInput.While this is a decorator-based stage and the actual stage name in the decorator is 'parseInput', the method name contains a typo which affects code readability and discoverability.
@Stage('parseInput') -async paseInput() { +async parseInput() {libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
118-132: Missingthis.handled()inonMessagecreates inconsistent flow termination.The
onSseListenerstage (lines 151-152) callsthis.handled()aftertransport.handleRequest(), butonMessagedoesn't. This inconsistency could cause unexpected behavior where the message stage doesn't properly signal flow completion.async onMessage() { const transportService = (this.scope as Scope).transportService; const { request, response } = this.rawInput; const { token, session } = this.state.required; const transport = await transportService.getTransporter('streamable-http', token, session.id); if (!transport) { this.respond(httpRespond.rpcError('session not initialized')); return; } await transport.handleRequest(request, response); + this.handled(); }
🧹 Nitpick comments (24)
apps/demo/src/main.ts (1)
25-27: Legacy SSE-only transport: consider clarifying intentEnabling only
enableLegacySSEhere (withoutenableSseListener) is a reasonable choice if this demo is specifically for legacy SSE. If you want parity with the auth demo or to avoid future confusion, consider either adding a brief comment (e.g., “legacy SSE demo only”) or wiringenableSseListeneras well.libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
224-232: Clarify the getter's return value when no events have been sent.The getter name "lastEventId" could be clearer about its behavior before any events are sent. Currently it returns 0, but the documentation doesn't explicitly state this initial state.
Consider updating the documentation:
/** * Returns the current event ID counter value. * - * This is the ID that was used for the last sent event. + * This is the ID that was used for the last sent event, or 0 if no events have been sent yet. * Useful for testing and debugging reconnection scenarios. */libs/sdk/src/common/types/auth/session.types.ts (1)
4-7: Duplicate type definition withTransportProtocolintransport-session.types.ts.Both
TransportProtocolTypehere andTransportProtocolinlibs/sdk/src/auth/session/transport-session.types.tsdefine the same string literal union. Consider consolidating into a single canonical type to avoid drift and reduce maintenance burden.Based on coding guidelines, centralize record type definitions in
libs/sdk/src/common/records/instead of module-specific files.-/** - * Transport protocol types (excludes non-transport intents like 'delete-session') - */ -export type TransportProtocolType = 'legacy-sse' | 'sse' | 'streamable-http' | 'stateful-http' | 'stateless-http'; +// Re-export from centralized location or import from transport-session.types.ts +export { TransportProtocol as TransportProtocolType } from '../../auth/session/transport-session.types';libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
58-68: Use stricter typing instead ofanyforparamsandctx.Per coding guidelines, avoid
anywithout strong justification. SinceSubscribeRequestSchemais from the MCP SDK, you can derive proper types:- let params: any; - let ctx: any; + let params: z.infer<typeof SubscribeRequestSchema>['params']; + let ctx: { sessionId?: string };Alternatively, if typing is complex, use
unknownand add type guards.libs/sdk/src/completion/flows/complete.flow.ts (3)
8-11: Avoidz.any()for context schema.Per coding guidelines, use
unknownor a proper schema instead ofany.const inputSchema = z.object({ request: CompleteRequestSchema, - ctx: z.any(), + ctx: z.unknown(), });
128-144: Consider a typed interface for the completer pattern to avoidanycasts.The
instance as anycasts bypass type checking. Define an optional interface for completable entities:interface Completable { getArgumentCompleter?(argName: string): ((value: string) => Promise<{ values?: string[]; total?: number; hasMore?: boolean; }>) | undefined; }Then cast to
Completableinstead ofany:-const instance = prompt as any; +const instance = prompt as Completable;This provides partial type safety while supporting optional completer methods.
Also applies to: 160-176
116-180: Extract shared completer logic to reduce duplication.Both prompt and resource completion blocks follow the same pattern. Consider extracting a helper:
private async invokeCompleter( instance: unknown, argName: string, argValue: string, entityType: string, entityId: string ): Promise<{ values: string[]; total?: number; hasMore?: boolean }> { const completable = instance as Completable; if (typeof completable?.getArgumentCompleter === 'function') { const completer = completable.getArgumentCompleter(argName); if (completer) { try { const result = await completer(argValue); return { values: result.values || [], total: result.total, hasMore: result.hasMore }; } catch (e) { this.logger.warn(`complete: completer failed for ${entityType} "${entityId}" argument "${argName}": ${e}`); } } } return { values: [] }; }This eliminates ~30 lines of duplication and centralizes error handling.
libs/sdk/src/transport/adapters/transport.local.adapter.ts (1)
50-54: Dynamic completions capability and notification wiring look correct; consider replacing console loggingThe additions here make sense:
completionsCapabilityis enabled only when prompts or resources exist, matching how the completions handler is conditionally registered.- Registering/unregistering the
McpServerwithscope.notificationsonconnectServer/destroyis symmetric and keeps the notification service in sync with transport lifecycle.Two minor follow‑ups:
- For an SDK/library surface, using
this.loggerinstead ofconsole.log/console.warnindestroywould keep logging behavior consistent and easier to control.- If there are code paths that might call
destroybeforeconnectServerhas finished, consider guardingunregisterServerwith a check that the session was actually registered (e.g., makingunregisterServeridempotent or ignoring missing entries), if it isn’t already.Also applies to: 61-64, 77-79, 87-92
libs/sdk/src/auth/session/utils/session-id.utils.ts (1)
4-4: Session ID protocol typing is now aligned withTransportProtocolTypeUsing
TransportProtocolTypefor theprotocolparameter increateSessionIdmatches the updated session type definitions and avoids accidental ad‑hoc strings.You might optionally add an explicit return type (
{ id: string; payload: SessionIdPayload }) for clarity on this public helper, but the current inference is type‑safe.Also applies to: 160-172
libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts (1)
1-1: Persisting client capabilities inscope.notificationsis reasonable and safeThe handler now:
- Extracts
sessionIdfromctx.authInfo?.sessionId.- Maps
request.params.capabilitiesinto a narrowClientCapabilitiesobject.- Stores them via
scope.notifications.setClientCapabilities.This is a clean place to persist client capabilities for later use (e.g., roots support, logging). The guards around
sessionIdandcapabilitiesprevent nullish access.Optionally, you could log a debug/verbose message when capabilities are present but
sessionIdis missing to aid troubleshooting, but it isn’t required for correctness.Also applies to: 5-5, 16-17, 23-33
libs/sdk/src/transport/mcp-handlers/subscribe-request.handler.ts (1)
1-38: Subscribe handler logic looks good; alignMcpRequestHandlergeneric withEmptyResultThe runtime flow is solid:
- Validates via
SubscribeRequestSchema.- Safely handles missing
sessionIdwith a warning and no subscription.- Delegates subscription to
scope.notifications.subscribeResourceand logs idempotent behavior.- Returns
EmptyResultper MCP spec.The only nit is the type parameter on
McpRequestHandler:ctx: McpRequestHandler<SubscribeRequest, never>Since the handler returns
Promise<EmptyResult>, it would be clearer and more type‑accurate to useEmptyResult(or omit the second generic if it’s unused) instead ofnever, e.g.:- ctx: McpRequestHandler<SubscribeRequest, never>, + ctx: McpRequestHandler<SubscribeRequest, EmptyResult>,This keeps the handler and context generics consistent with the declared
McpHandler<SubscribeRequest, EmptyResult>.libs/sdk/src/auth/flows/session.verify.flow.ts (1)
144-152: Public‑mode protocol handling is safer now; avoid duplicating protocol literals and consider UUID sourcingThe updated logic in
handlePublicModelooks good from a behavior standpoint:
- New sessions no longer set a protocol eagerly; only existing sessions reuse the stored protocol, deferring intent/transport selection to the router.
- Public session payloads include a
protocolonly when it’s a known value, otherwiseundefined, which keeps them consistent with the rest of the session tooling.Two small refactors to consider:
Reuse the central protocol type instead of duplicating literals
You’re re‑encoding the allowed protocols:
const validProtocols = ['sse', 'legacy-sse', 'streamable-http', 'stateful-http', 'stateless-http'] as const; type ValidProtocol = (typeof validProtocols)[number];Since
TransportProtocolTypealready exists insession.types.ts, importing and using it here (and invalidProtocols) would avoid divergence if a new protocol is added later.Align UUID generation with existing crypto usage
This file currently uses
crypto.randomUUID()for the anonymous user and session UUIDs, whilesession-id.utils.tsimportsrandomUUIDfrom Node’scryptomodule. For consistency and clearer typing across the auth/session utilities, consider switching to the same import‑based pattern or a shared helper (e.g., a smallgenerateUuid()util).Also applies to: 182-196
libs/sdk/src/scope/scope.instance.ts (1)
48-48: Consider makingnotificationServiceprivate.The property is exposed both directly as a public field and via the
notificationsgetter (Line 180). For consistency with other registries (e.g.,scopeAuth,scopeTools) which are private with public getters, consider making this field private:- notificationService: NotificationService; + private notificationService: NotificationService;libs/sdk/src/transport/mcp-handlers/roots-list-changed-notification.handler.ts (1)
17-24: Consider adding a verbose log when sessionId is missing.Other handlers in this PR log a warning when
sessionIdis not found. For consistency and debugging purposes, consider adding a similar log here:const sessionId = ctx.authInfo?.sessionId; if (sessionId) { // Invalidate cached roots for this session scope.notifications.invalidateRootsCache(sessionId); + } else { + scope.logger.verbose('notifications/roots/list_changed: No session ID found in request context'); }libs/sdk/src/scope/flows/http.request.flow.ts (1)
277-285: Header lookup could be simplified using a case-insensitive approach.The current implementation is correct but verbose. Consider extracting a utility function for case-insensitive header access, as this pattern may be needed elsewhere.
+ private getHeader(request: ServerRequest, name: string): string | undefined { + const entry = Object.entries(request.headers).find(([k]) => k.toLowerCase() === name.toLowerCase()); + return entry?.[1] as string | undefined; + } async handleDeleteSession() { const { request } = this.rawInput; const sessionId = request[ServerRequestTokens.sessionId] ?? - Object.entries(request.headers).find(([k]) => k.toLowerCase() === 'mcp-session-id')?.[1]; + this.getHeader(request, 'mcp-session-id');libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (2)
8-11: Consider usingz.unknown()instead ofz.any()for stricter type safety.Per coding guidelines, prefer
unknownoveranyfor generic types. Thectxfield could usez.unknown()or a more specific schema if the context structure is known.const inputSchema = z.object({ request: UnsubscribeRequestSchema, - ctx: z.any(), + ctx: z.unknown(), });
58-68: Type variables usinganyshould use more specific types orunknown.The
paramsandctxvariables are typed asany. Consider using type inference from the schema orunknownwith proper type guards. Based on learnings, avoidanytypes without strong justification.- let params: any; - let ctx: any; + let params: z.infer<typeof UnsubscribeRequestSchema>['params']; + let ctx: unknown; try { const inputData = inputSchema.parse(this.rawInput); method = inputData.request.method; params = inputData.request.params; ctx = inputData.ctx;libs/sdk/src/logging/flows/set-level.flow.ts (2)
9-12: Consider usingz.unknown()instead ofz.any()for stricter type safety.Same pattern as in the unsubscribe flow - prefer
unknownoveranyper coding guidelines.const inputSchema = z.object({ request: SetLevelRequestSchema, - ctx: z.any(), + ctx: z.unknown(), });
58-69: Type variables usinganyshould use more specific types orunknown.Similar to the unsubscribe flow, prefer typed variables or
unknownoverany.- let params: any; - let ctx: any; + let params: z.infer<typeof SetLevelRequestSchema>['params']; + let ctx: unknown;libs/sdk/src/common/utils/decide-request-intent.utils.ts (1)
412-416: Redundant ternary in debug.flags.On line 415,
sessionId ? B_HAS_SESSION : 0is always truthy since the early return on line 404-411 ensuressessionIdexists at this point. Simplify to justB_HAS_SESSION.return { intent: 'delete-session', reasons: ['DELETE with Mcp-Session-Id → session termination.'], - debug: { key: 0, channel: 0, flags: sessionId ? B_HAS_SESSION : 0 }, + debug: { key: 0, channel: 0, flags: B_HAS_SESSION }, };libs/sdk/src/notification/notification.service.ts (4)
214-225: Validate session termination edge cases.
terminateSessioncallsunregisterServerwhich removes the session, then adds toterminatedSessions. This is correct. However, consider whetherterminatedSessions.add()should happen even if the session wasn't registered (current behavior) - the comment mentions "edge cases" but doesn't specify which. This is fine for safety, but document the specific edge case (e.g., repeated DELETE requests).
586-592: Type assertion onresult.roots.The assertion
result.roots as Root[]assumes the MCP SDK'sListRootsResultSchemareturns a compatible type. This is likely safe given the schema validation, but consider adding a runtime check or using a type guard for defense in depth.// Alternative: map to ensure type safety registered.cachedRoots = result.roots.map(r => ({ uri: r.uri, name: r.name }));
646-662:destroy()is async but performs no async operations.The method is declared
asyncbut only contains synchronous operations. This isn't incorrect, but if it's intended to support future async cleanup, consider documenting that intent. Otherwise, it could simply be synchronous.- async destroy(): Promise<void> { + destroy(): void {Or add a comment explaining the async signature is for future extensibility.
670-683: Auto-unregister on notification failure may hide issues.When
sendNotificationToServercatches an error, it automatically callsunregisterServer(sessionId). While this handles dead connections, it could mask transient network issues. Consider:
- Tracking consecutive failures before unregistering
- Distinguishing connection-closed errors from other failures
- Emitting an event/metric when auto-unregistering
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
apps/auth/demo-public/src/main.ts(1 hunks)apps/demo/src/main.ts(1 hunks)docs/draft/docs/authentication/modes.mdx(11 hunks)docs/draft/docs/authentication/production.mdx(13 hunks)docs/draft/docs/authentication/progressive.mdx(3 hunks)libs/sdk/src/auth/flows/session.verify.flow.ts(1 hunks)libs/sdk/src/auth/session/transport-session.types.ts(1 hunks)libs/sdk/src/auth/session/utils/session-id.utils.ts(2 hunks)libs/sdk/src/common/entries/scope.entry.ts(2 hunks)libs/sdk/src/common/schemas/http-output.schema.ts(3 hunks)libs/sdk/src/common/types/auth/session.types.ts(2 hunks)libs/sdk/src/common/utils/decide-request-intent.utils.ts(9 hunks)libs/sdk/src/completion/flows/complete.flow.ts(1 hunks)libs/sdk/src/logging/flows/set-level.flow.ts(1 hunks)libs/sdk/src/notification/index.ts(1 hunks)libs/sdk/src/notification/notification.service.ts(1 hunks)libs/sdk/src/resource/flows/subscribe-resource.flow.ts(1 hunks)libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts(1 hunks)libs/sdk/src/resource/resource.registry.ts(3 hunks)libs/sdk/src/scope/flows/http.request.flow.ts(4 hunks)libs/sdk/src/scope/scope.instance.ts(4 hunks)libs/sdk/src/transport/adapters/transport.local.adapter.ts(3 hunks)libs/sdk/src/transport/flows/handle.sse.flow.ts(1 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(3 hunks)libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts(4 hunks)libs/sdk/src/transport/mcp-handlers/complete-request.handler.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/index.ts(2 hunks)libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts(2 hunks)libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/roots-list-changed-notification.handler.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/subscribe-request.handler.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/sdk/src/transport/mcp-handlers/index.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/auth/session/utils/session-id.utils.tslibs/sdk/src/common/types/auth/session.types.tslibs/sdk/src/resource/resource.registry.tslibs/sdk/src/transport/mcp-handlers/roots-list-changed-notification.handler.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/notification/index.tsapps/auth/demo-public/src/main.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/sdk/src/transport/mcp-handlers/complete-request.handler.tslibs/sdk/src/transport/mcp-handlers/subscribe-request.handler.tslibs/sdk/src/auth/flows/session.verify.flow.tslibs/sdk/src/resource/flows/subscribe-resource.flow.tslibs/sdk/src/transport/legacy/legacy.sse.tranporter.tsapps/demo/src/main.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/common/schemas/http-output.schema.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/common/utils/decide-request-intent.utils.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.tslibs/sdk/src/notification/notification.service.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/transport/mcp-handlers/index.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/auth/session/utils/session-id.utils.tslibs/sdk/src/common/types/auth/session.types.tslibs/sdk/src/resource/resource.registry.tslibs/sdk/src/transport/mcp-handlers/roots-list-changed-notification.handler.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/notification/index.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/sdk/src/transport/mcp-handlers/complete-request.handler.tslibs/sdk/src/transport/mcp-handlers/subscribe-request.handler.tslibs/sdk/src/auth/flows/session.verify.flow.tslibs/sdk/src/resource/flows/subscribe-resource.flow.tslibs/sdk/src/transport/legacy/legacy.sse.tranporter.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/common/schemas/http-output.schema.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/common/utils/decide-request-intent.utils.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.tslibs/sdk/src/notification/notification.service.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/transport/mcp-handlers/index.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/auth/session/utils/session-id.utils.tslibs/sdk/src/common/types/auth/session.types.tslibs/sdk/src/resource/resource.registry.tslibs/sdk/src/transport/mcp-handlers/roots-list-changed-notification.handler.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/notification/index.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/sdk/src/transport/mcp-handlers/complete-request.handler.tslibs/sdk/src/transport/mcp-handlers/subscribe-request.handler.tslibs/sdk/src/auth/flows/session.verify.flow.tslibs/sdk/src/resource/flows/subscribe-resource.flow.tslibs/sdk/src/transport/legacy/legacy.sse.tranporter.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/common/schemas/http-output.schema.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/common/utils/decide-request-intent.utils.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.tslibs/sdk/src/notification/notification.service.ts
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/authentication/modes.mdxdocs/draft/docs/authentication/progressive.mdxdocs/draft/docs/authentication/production.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/authentication/modes.mdxdocs/draft/docs/authentication/progressive.mdxdocs/draft/docs/authentication/production.mdx
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/main.ts
🧠 Learnings (8)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methods
Applied to files:
libs/sdk/src/transport/mcp-handlers/index.tslibs/sdk/src/common/types/auth/session.types.tslibs/sdk/src/notification/index.tslibs/sdk/src/transport/mcp-handlers/complete-request.handler.tslibs/sdk/src/common/schemas/http-output.schema.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Applied to files:
libs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/adapters/src/**/*.ts : Do not hardcode capabilities in adapters - use registry.getCapabilities() for dynamic capability exposure
Applied to files:
libs/sdk/src/transport/adapters/transport.local.adapter.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/registry.ts : Implement getCapabilities() method in registries to expose dynamic capability flags (subscribe, listChanged, etc.) instead of hardcoding
Applied to files:
libs/sdk/src/resource/resource.registry.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/sdk/src/notification/index.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/sdk/src/notification/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Do not mutate rawInput in flows - use state.set() for flow state management instead
Applied to files:
libs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/sdk/src/auth/flows/session.verify.flow.tslibs/sdk/src/logging/flows/set-level.flow.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/logging/flows/set-level.flow.ts
🧬 Code graph analysis (11)
libs/sdk/src/common/entries/scope.entry.ts (2)
libs/sdk/src/notification/index.ts (1)
NotificationService(4-4)libs/sdk/src/notification/notification.service.ts (1)
NotificationService(102-685)
libs/sdk/src/scope/scope.instance.ts (2)
libs/sdk/src/notification/index.ts (1)
NotificationService(4-4)libs/sdk/src/notification/notification.service.ts (1)
NotificationService(102-685)
libs/sdk/src/auth/session/utils/session-id.utils.ts (1)
libs/sdk/src/common/types/auth/session.types.ts (1)
TransportProtocolType(7-7)
libs/sdk/src/transport/mcp-handlers/roots-list-changed-notification.handler.ts (1)
libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)
libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
Flow(44-128)
libs/sdk/src/transport/mcp-handlers/subscribe-request.handler.ts (1)
libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)
libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
Flow(44-112)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
libs/sdk/src/scope/scope.instance.ts (1)
Scope(33-207)
libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts (1)
libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)
libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts (2)
libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)libs/sdk/src/notification/notification.service.ts (1)
ClientCapabilities(23-30)
libs/sdk/src/notification/notification.service.ts (1)
libs/sdk/src/notification/index.ts (7)
Root(9-9)ClientCapabilities(10-10)McpLoggingLevel(7-7)MCP_LOGGING_LEVEL_PRIORITY(8-8)McpNotificationMethod(5-5)RegisteredServer(6-6)NotificationService(4-4)
🔇 Additional comments (37)
apps/auth/demo-public/src/main.ts (1)
22-23: Dual SSE flags enabled – confirm demo intentBoth
enableLegacySSEandenableSseListenerare enabled, so the public demo will expose both/sse(legacy) and the modern SSE listener simultaneously. That’s fine if the goal is to showcase both modes side‑by‑side; otherwise, consider enabling only the mode you want to demonstrate to avoid confusion.docs/draft/docs/authentication/modes.mdx (1)
32-52: Now let me examine a sample of the conflicts to verify they are indeed formatting-only:
<function_calls>
#!/bin/bashRead the first conflict block to see the exact differences
sed -n '32,52p' "docs/draft/docs/authentication/modes.mdx"
</function_calls><function_calls>
#!/bin/bashCheck another conflict block to verify the pattern
sed -n '82,98p' "docs/draft/docs/authentication/modes.mdx"
</function_calls><function_calls>
#!/bin/bashExtract and compare content from HEAD and main versions for first conflict
echo "=== First conflict (lines 32-52) - Content comparison ==="
echo ""
echo "HEAD version (lines 33-41):"
sed -n '33,41p' "docs/draft/docs/authentication/modes.mdx"
echo ""
echo "Main version (lines 43-51):"
sed -n '43,51p' "docs/draft/docs/authentication/modes.mdx"
</function_calls>libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (3)
117-119: LGTM! SSE event ID format is correct.The implementation correctly follows the SSE specification format with the
idfield for reconnection support. Using pre-increment ensures the first event starts at ID 1, which is semantically appropriate.
210-212: LGTM! Consistent event ID implementation.The message event ID follows the same pattern as the endpoint event, maintaining a single sequential ID counter across all SSE events. This is correct for client reconnection scenarios.
46-47: Implementation correctly complies with MCP 2025-11-25 specification for SSE event IDs.The MCP 2025-11-25 specification confirms that servers MAY include an "id" field in SSE events, and if present, IDs MUST be globally unique within the session. The implementation satisfies these requirements:
- Uniqueness: The incrementing counter (
_eventIdCounter) ensures globally unique IDs within the session- Pre-increment pattern: Using
++this._eventIdCountercorrectly produces IDs starting from 1 (not 0)- Single-stream architecture: This transport handles one SSE stream per instance, so a bare counter is appropriate; the spec's guidance to encode stream information applies to multi-stream scenarios
- Field initialization: Initializing
_eventIdCounter = 0is correct for pre-increment usage- Spec compliance: The comment accurately references MCP 2025-11-25 spec compliance
No issues found with the implementation at lines 46–47 or the related event ID handling throughout the file.
libs/sdk/src/common/types/auth/session.types.ts (1)
55-62: LGTM!The schema correctly mirrors the
SessionIdPayloadtype with proper validation. Thez.enumvalues align withTransportProtocolTypeand thesatisfies RawZodShape<SessionIdPayload>constraint ensures type safety.libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
71-85: Routing refactor looks clean; consider handling unmatched paths.The explicit suffix matching is clearer. However, when neither
/ssenor/messagematches,requestTyperemains undefined and no stage executes, causing the flow to complete silently without a response.Verify this is intentional (e.g., another flow handles unmatched paths) or consider adding a fallback response for unmatched routes.
libs/sdk/src/auth/session/transport-session.types.ts (1)
5-9: LGTM!This appears to be the authoritative location for
TransportProtocol. The type and accompanying schemas are well-structured and consistently used throughout the file.libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
86-127: LGTM!The
validateResource,subscribe, andfinalizestages are well-implemented:
- Proper resource existence check before subscription
- Clear logging at appropriate levels
- Empty result per MCP spec compliance
- Consistent with the companion
UnsubscribeResourceFlowpatternlibs/sdk/src/completion/flows/complete.flow.ts (1)
182-201: LGTM!The result building and finalize stages are well-implemented. The flow gracefully handles missing prompts/resources by returning empty completion values, which is appropriate behavior.
libs/sdk/src/resource/resource.registry.ts (1)
35-36: Resource subscription flows and capabilities are wired correctly
- Registering
SubscribeResourceFlowandUnsubscribeResourceFlowalongside the existing read/list flows duringinitializeis consistent with how other registries hook flows into the active scope.getCapabilitiesnow advertisingresources: { subscribe: true, listChanged: true }whenhasAny()is true matches the MCP 2025‑11‑25 subscription semantics and remains backward‑compatible (only adds a flag).No functional issues spotted here.
Also applies to: 158-164, 507-518
libs/sdk/src/transport/mcp-handlers/index.ts (1)
9-15: Handler composition for subscriptions, completion, logging, and roots notifications is coherent
- Adding
subscribeRequestHandler/unsubscribeRequestHandlerunder theresourcescapability matches the newresources.subscribeflag inResourceRegistry.getCapabilities().completionHandlerbeing enabled when eitherpromptsorresourcescapabilities are present lines up with the spec allowing completion over both prompts and resources.loggingHandleris gated oncapabilities.logging, which is always set to{}by the adapter, so logging/setLevel will be consistently available.- Including
rootsListChangedNotificationHandleralongsideinitializedNotificationHandleris a good place for out‑of‑band notifications.The capability checks use optional chaining and should behave correctly when certain registries are absent.
Also applies to: 22-28, 35-45, 49-55
libs/sdk/src/common/entries/scope.entry.ts (1)
22-23: AllScopeEntrysubclasses properly implement the newnotificationsgetterVerification confirms that
Scope(the only subclass ofScopeEntry) implements the abstractnotificationsgetter at line 180 oflibs/sdk/src/scope/scope.instance.ts. No compilation or runtime issues expected.libs/sdk/src/scope/scope.instance.ts (2)
102-107: LGTM!The initialization sequence is correct: NotificationService is created after all registries are ready, then initialized, and finally the logging and completion flows are registered. This ensures the notification service can subscribe to registry changes during initialization.
180-182: LGTM!The getter follows the established pattern for exposing registries and services, consistent with
tools,resources, andpromptsgetters.libs/sdk/src/transport/mcp-handlers/complete-request.handler.ts (1)
4-8: LGTM!Clean delegation to the flow system. The
satisfieskeyword ensures type safety for the returned handler object. The implementation is minimal and correctly routes to the completion flow.libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts (1)
16-34: LGTM!The handler logic is correct and follows the same pattern as the subscribe handler. The verbose logging for both subscribed and not-subscribed cases aids debugging. The URI is expected to be validated by the MCP schema before reaching this handler.
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (2)
134-153: SSE listener implementation looks correct.The handler properly checks for an existing transport, returns 404 if the session isn't found, and forwards the GET request to establish the SSE stream. The flow termination with
this.handled()is appropriate.
73-79: GET request routing to SSE listener is well-structured.Early detection of GET requests and short-circuiting to the SSE path avoids unnecessary JSON-RPC body parsing. The comment referencing MCP spec is helpful for maintainability.
libs/sdk/src/scope/flows/http.request.flow.ts (3)
123-136: DELETE handling logic is sound.The early return for DELETE requests with proper intent handling and forwarding unknown DELETEs to middleware is correct. This allows developers to implement custom DELETE handlers while still supporting MCP session termination.
146-152: Terminated session check follows MCP spec correctly.Properly returns 404 for requests to terminated sessions per MCP specification. The partial session ID logging (first 20 chars) is a good security practice to avoid exposing full session identifiers in logs.
267-303: OverallhandleDeleteSessionimplementation is well-structured.The stage properly validates session ID presence, terminates the session through the notification service, and returns appropriate HTTP responses (404 for not found, 204 for success).
libs/sdk/src/notification/index.ts (1)
1-11: Clean barrel file exports for notification module.The exports are well-organized with clear separation of the class, types, and constants. This follows the barrel file pattern guidelines.
libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (2)
86-103: Unsubscribe stage correctly follows MCP spec for silent success.The implementation properly handles the case where the session wasn't subscribed, succeeding silently per MCP specification. The logging distinguishes between actual unsubscriptions and no-op cases.
51-112: Flow structure is consistent with SubscribeResourceFlow.The overall flow pattern (parseInput → unsubscribe → finalize) mirrors the subscribe flow appropriately. State management correctly uses
state.set()without mutating rawInput, following the coding guidelines.libs/sdk/src/logging/flows/set-level.flow.ts (2)
45-113: Overall flow implementation follows established patterns.The SetLevelFlow properly implements the MCP logging/setLevel request with correct input validation, session ID requirement, and empty result response per specification. State management uses
state.set()correctly.
93-101: I'll help you verify whether the type cast toMcpLoggingLevelis necessary. Let me investigate the type definitions and method signature.
<function_calls>
#!/bin/bashSearch for McpLoggingLevel type definition
echo "=== Searching for McpLoggingLevel ==="
rg -n 'type McpLoggingLevel|interface McpLoggingLevel' libs/sdk/src --type=tsecho ""
echo "=== Searching for LoggingLevelSchema ==="
rg -n 'LoggingLevelSchema' libs/sdk/src --type=ts -B2 -A5echo ""
echo "=== Checking the set-level.flow.ts file context ==="
cat -n libs/sdk/src/logging/flows/set-level.flow.ts | head -100
</function_calls>libs/sdk/src/common/utils/decide-request-intent.utils.ts (4)
35-41: LGTM! Type updated to match schema.The
HttpRequestIntenttype now correctly includesdelete-sessionto match theintentSchemaunion. The explicit union type keeps things clear and maintainable.
130-133: LGTM! Clean path detection helper.The
isLegacySsePathfunction correctly handles both exact/ssepaths and paths ending with/sse(e.g.,/api/sse), consistent with the/messagepath handling on line 160.
220-245: LGTM! Comprehensive GET / SSE listener rules.The new rules (A2, A2b, A2c) correctly implement the MCP 2025-11-25 spec for SSE listeners:
- A2: GET / with session + listener/streamable enabled → streamable-http
- A2b: GET / with session but listener disabled → 405
- A2c: GET / without session → forward (unknown)
The rule ordering ensures proper matching with the bitmask-based priority system.
397-417: DELETE handler correctly short-circuits before bitmap computation.The early return for DELETE requests avoids unnecessary bitmap computation and rule matching. The 400 status for missing session ID aligns with MCP spec requirements.
libs/sdk/src/notification/notification.service.ts (6)
102-115: LGTM! Well-organized service state management.The class properly separates concerns with dedicated maps for servers, subscriptions, log levels, and terminated sessions. The private readonly modifiers ensure immutability of the collections themselves.
159-172: LGTM! Idempotent server registration with warning.The early return with warning on duplicate registration prevents accidental overwrites while maintaining idempotency. The truncated session ID in logs is a good practice for readability.
121-150: LGTM! Clean registry subscription setup.The initialization correctly subscribes to resource, tool, and prompt changes with
immediate: falseto avoid spurious notifications on startup. The unsubscribe functions are properly tracked for cleanup indestroy().
36-50: LGTM! Correct RFC 5424 logging levels.The
McpLoggingLeveltype andMCP_LOGGING_LEVEL_PRIORITYmap correctly implement the syslog severity levels per RFC 5424, with proper ordering from most verbose (debug=0) to least verbose (emergency=7).
425-452: LGTM! Correct log level filtering logic.The
sendLogMessageimplementation correctly filters messages by comparing numeric priorities - only sending whenmessagePriority >= sessionPriority. The decision to skip sessions without an explicit level is a reasonable default.
562-608: LGTM! Robust roots caching with graceful degradation.The
listRootsimplementation:
- Checks session registration and capability support
- Returns cached results when available (unless
forceRefresh)- Has configurable timeout (default 30s)
- Falls back to cached or empty array on error
This provides good resilience for client communication failures.
libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts
Outdated
Show resolved
Hide resolved
libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts
Outdated
Show resolved
Hide resolved
libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
libs/sdk/src/scope/flows/http.request.flow.ts (2)
289-299: Re‑confirm memory bounds for terminated sessions to avoid unbounded growthThis flow relies on
this.scope.notifications.terminateSession(sessionId)and comments note that the session ID is added to a “terminated set” even whenwasRegisteredis false. A prior review flagged thatterminatedSessionsinnotification.service.tswas an unboundedSetonly cleared indestroy(), so repeated DELETEs with random or maliciously generated session IDs could cause slow, unbounded memory growth over the process lifetime.If that implementation is still in place, please add a bound (TTL or max-size with eviction) around the terminated-session tracking, or otherwise ensure the underlying storage is self‑limiting.
Since the actual fix would live in
notification.service.ts, can you confirm whether this was already addressed, and if not, whether you want suggestions for a bounded structure?
276-285: Align invalid session ID handling with POST/PUT and avoid JSON‑RPC error for HTTP‑level 400For DELETE without a valid session ID you currently return
httpRespond.rpcError('No valid session ID provided'), which encodes this as a JSON‑RPC error rather than an HTTP 400. Earlier handlers for similar validation (e.g., POST/PUT) usebadRequest(), and returning a consistent HTTP‑level 400 here would match that behavior and be clearer to clients.You can also simplify header access given that headers are typically already normalized to lower case:
async handleDeleteSession() { const { request } = this.rawInput; - const sessionId = - request[ServerRequestTokens.sessionId] ?? - Object.entries(request.headers).find(([k]) => k.toLowerCase() === 'mcp-session-id')?.[1]; + const sessionId = + request[ServerRequestTokens.sessionId] ?? + (request.headers['mcp-session-id'] as string | undefined); if (!sessionId || typeof sessionId !== 'string') { this.logger.warn('DELETE request without valid session ID'); - this.respond(httpRespond.rpcError('No valid session ID provided')); + this.respond(httpRespond.badRequest('Bad Request: No valid session ID provided')); return; }This keeps behavior consistent across methods and avoids unnecessary work scanning header entries.
libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts (1)
1-4: Remove the unnecessary cast from SDKleveltoMcpLoggingLeveland align the types.
request.params.levelis already typed as the MCP SDK logging level union;McpLoggingLevelinnotification.service.tsis structurally identical. The explicit cast:const success = scope.notifications.setLogLevel(sessionId, level as McpLoggingLevel);is therefore redundant and can hide future mismatches.
Prefer either:
- Making
McpLoggingLevelan explicit alias of the SDK type, and updatingNotificationService.setLogLevelto accept that alias directly; or- Dropping
McpLoggingLeveland using the SDK type end‑to‑end.In both cases you can remove the cast here and in
SetLevelFlow.Also applies to: 24-24
libs/sdk/src/logging/flows/set-level.flow.ts (1)
5-8: Unify logging level types to avoid redundant casts.
levelhere is typed fromLoggingLevelSchema, whileNotificationService.setLogLevelexpectsMcpLoggingLevel; both unions contain the same literals. The cast:const success = this.scope.notifications.setLogLevel(sessionId, level as McpLoggingLevel);should be unnecessary.
To keep typing strict and avoid masking future drift, consider:
- Making
McpLoggingLevelan alias of the SDK’sLoggingLeveltype (or vice versa), and- Updating
setLogLevelto accept that unified type.Then you can drop the
as McpLoggingLevelhere (and in the MCP handler) entirely.Also applies to: 87-95
🧹 Nitpick comments (8)
libs/sdk/src/transport/flows/handle.sse.flow.ts (2)
56-69: Avoid mutatingrawInputwhen synthesizing the legacy SSE session
parseInputstill writesrequest[ServerRequestTokens.auth].session, which mutatesrawInputrather than relying solely onstate. This goes against the repo guidance for flows and makes behavior more implicit than necessary (downstream code already usesthis.state.requiredin this class).You can compute the session purely from the authorization object and keep it in state without touching
request:- const authorization = request[ServerRequestTokens.auth] as Authorization; - const { token } = authorization; - let { session } = authorization; - - if (!session) { - session = createSessionId('legacy-sse', token); - request[ServerRequestTokens.auth].session = session; - } - this.state.set(stateSchema.parse({ token, session })); + const authorization = request[ServerRequestTokens.auth] as Authorization; + const { token } = authorization; + const session = + authorization.session ?? createSessionId('legacy-sse', token); + + this.state.set(stateSchema.parse({ token, session }));Please double‑check whether anything outside this flow relies on
request[ServerRequestTokens.auth].sessionbeing backfilled before applying this change. Based on learnings, this keeps flows from mutatingrawInputand centralizes state inthis.state.
71-84: VerifybasePathnormalization and unmatched-path behavior in the routerThe new router logic builds
basePath = \${prefix}${scopePath}`and then matches exactly`${basePath}/sse`and`${basePath}/message`. This looks good for non‑rootrouteBasevalues, but correctness now depends on the precise behavior ofnormalizeScopeBase`:
- If
normalizeScopeBase(scope.routeBase)can ever return'/'for a root scope, this would produce'/prefix//sse'and fail to match a request path of'/prefix/sse'.- For any non‑matching path,
requestTypestays unset and the flow effectively becomes a no‑op; confirm this is intentional and that upstream routing (or another flow) returns an appropriate HTTP error in those cases.I’d recommend reviewing the possible
routeBasevalues andnormalizeScopeBasebehavior to ensure there are no double‑slash or root‑scope edge cases for legacy SSE routes.libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (2)
57-68: Avoid mutatingrequest[ServerRequestTokens.auth]insideparseInput
parseInputstill writesrequest[ServerRequestTokens.auth].session = session, which mutatesrawInputrather than relying solely onthis.state. This goes against the stated guideline to treatrawInputas immutable and usestate.set()for flow state.If feasible, consider refactoring so that you only derive
sessionand store it in state, and have downstream logic read the session fromthis.stateinstead ofrequest:- if (!session) { - session = createSessionId('streamable-http', token); - request[ServerRequestTokens.auth].session = session; - } - this.state.set(stateSchema.parse({ token, session })); + if (!session) { + session = createSessionId('streamable-http', token); + } + this.state.set(stateSchema.parse({ token, session }));This may require follow-up changes where
auth.sessionis consumed, so it’s a recommended (not blocking) cleanup.Based on learnings, this flow should avoid mutating
rawInput.
70-82: GET-based SSE listener routing looks good; consider tightening detectionRouting all
GETrequests by settingrequestType = 'sseListener'with an earlyreturncleanly separates SSE listener traffic from JSON-RPCPOSThandling.Optionally, you could further narrow this to SSE-specific requests (e.g., checking
Accept: text/event-streamand/or a dedicated path) to avoid accidentally treating unrelated GETs as SSE listener traffic.libs/sdk/src/completion/flows/complete.flow.ts (1)
23-36: Tighten typing for completers and resource URI validation.Two small type-safety improvements would help here:
- For
ResourceRefSchema.uri, consider applying the shared MCP URI refinement (e.g.,isValidMcpUri) instead of a barez.string(), to keep URI validation consistent across the SDK.- In the
completestage, bothpromptandresourceMatch.instanceare cast toany, and the completer return type is implicit. Defining a minimalArgumentCompletable/ArgumentCompleterResultinterface and using that instead ofanywould give better compile‑time guarantees while preserving the optionalgetArgumentCompleterpattern.Also applies to: 128-180
libs/sdk/src/notification/notification.service.ts (1)
12-17: Align Root typing with MCP schema and enforce documented URI constraint.
listRootscurrently does:const result = await registered.server.request({ method: 'roots/list' }, ListRootsResultSchema, ...); registered.cachedRoots = result.roots as Root[];while
Rootis a local interface with onlyuriandname?, and the comment saysuri“must start with file://”.To make this safer and more self‑documenting:
- Prefer aliasing
Rootdirectly to the MCP SDK’s root type (or deriving it fromListRootsResultSchema) instead of casting withas Root[].- Encode the
file://requirement either in the MCP root type alias or via a Zod refinement (or a sharedisValidMcpUrihelper constrained to file URIs), so violations are caught earlier rather than relying solely on the doc comment.Also applies to: 575-621
libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts (1)
9-35: Unsubscribe handler behavior looks correct and matches best‑effort semantics.The handler correctly:
- Validates via
UnsubscribeRequestSchema,- Derives
sessionIdfromctx.authInfo,- Delegates to
scope.notifications.unsubscribeResourceand logs whether a subscription existed,- Returns an empty result per MCP spec.
If you later want full consistency with
UnsubscribeResourceFlow, you could treat missingsessionIdas an MCP Invalid Input error instead of a silent no‑op, but the current design is coherent.libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
15-21: Use shared MCP URI validation helper foruriin state schema.
stateSchema.input.uriis currently justz.string().min(1), while unsubscribe operates on MCP resource URIs and other flows/services are moving toward centralized URI validation.If you have an
isValidMcpUri(or similar) refinement used elsewhere, consider applying it here as well, e.g.:uri: z.string().min(1).refine(isValidMcpUri, 'Invalid MCP resource URI'),to keep URI handling consistent and catch malformed unsubscribe requests early.
Also applies to: 86-100
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
libs/sdk/src/completion/flows/complete.flow.ts(1 hunks)libs/sdk/src/logging/flows/set-level.flow.ts(1 hunks)libs/sdk/src/notification/notification.service.ts(1 hunks)libs/sdk/src/prompt/prompt.events.ts(1 hunks)libs/sdk/src/prompt/prompt.registry.ts(2 hunks)libs/sdk/src/resource/flows/subscribe-resource.flow.ts(1 hunks)libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts(1 hunks)libs/sdk/src/scope/flows/http.request.flow.ts(4 hunks)libs/sdk/src/transport/flows/handle.sse.flow.ts(2 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(4 hunks)libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/subscribe-request.handler.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/transport/mcp-handlers/subscribe-request.handler.ts
- libs/sdk/src/resource/flows/subscribe-resource.flow.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/sdk/src/prompt/prompt.events.tslibs/sdk/src/prompt/prompt.registry.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/notification/notification.service.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/prompt/prompt.events.tslibs/sdk/src/prompt/prompt.registry.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/notification/notification.service.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.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/prompt/prompt.events.tslibs/sdk/src/prompt/prompt.registry.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/notification/notification.service.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.ts
🧠 Learnings (6)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Applied to files:
libs/sdk/src/prompt/prompt.events.tslibs/sdk/src/prompt/prompt.registry.tslibs/sdk/src/notification/notification.service.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Do not mutate rawInput in flows - use state.set() for flow state management instead
Applied to files:
libs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methods
Applied to files:
libs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.tslibs/sdk/src/notification/notification.service.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.tslibs/sdk/src/logging/flows/set-level.flow.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/logging/flows/set-level.flow.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Use consistent enum naming for variants ('primary', 'secondary', 'outline', 'ghost', 'danger', 'success') and sizes ('xs', 'sm', 'md', 'lg', 'xl')
Applied to files:
libs/sdk/src/notification/notification.service.ts
🧬 Code graph analysis (7)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (6)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
stateSchema(27-31)libs/sdk/src/transport/flows/handle.stateless-http.flow.ts (1)
stateSchema(24-28)libs/sdk/src/common/types/auth/session.types.ts (1)
sessionIdSchema(73-76)libs/sdk/src/scope/scope.instance.ts (1)
Scope(33-207)libs/sdk/src/common/interfaces/internal/primary-auth-provider.interface.ts (1)
transport(82-84)libs/sdk/src/common/schemas/http-output.schema.ts (1)
httpRespond(254-336)
libs/sdk/src/scope/flows/http.request.flow.ts (4)
libs/testing/src/transport/streamable-http.transport.ts (1)
request(117-241)libs/testing/src/client/mcp-test-client.ts (2)
request(819-868)sessionId(206-208)libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)apps/demo/src/apps/expenses/providers/session-redis.provider.ts (1)
k(18-20)
libs/sdk/src/transport/mcp-handlers/logging-set-level-request.handler.ts (2)
libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)libs/sdk/src/notification/notification.service.ts (1)
McpLoggingLevel(36-36)
libs/sdk/src/logging/flows/set-level.flow.ts (2)
libs/sdk/src/common/decorators/hook.decorator.ts (1)
FlowHooksOf(187-194)libs/sdk/src/notification/notification.service.ts (1)
McpLoggingLevel(36-36)
libs/sdk/src/transport/mcp-handlers/unsubscribe-request.handler.ts (1)
libs/sdk/src/transport/legacy/legacy.sse.tranporter.ts (1)
sessionId(220-222)
libs/sdk/src/notification/notification.service.ts (1)
libs/sdk/src/scope/scope.instance.ts (1)
Scope(33-207)
libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
Flow(44-128)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (8)
libs/sdk/src/prompt/prompt.registry.ts (1)
364-364: LGTM! Property rename implemented consistently.Both emission sites correctly use
changeScopeinstead ofscope, aligning with the type definition change inprompt.events.ts. The implementation is consistent with the coding guideline to avoid confusion with the Scope class.Based on learnings, this property name change is correct.
Also applies to: 374-374
libs/sdk/src/prompt/prompt.events.ts (1)
9-9: Breaking change is properly implemented—all code has been updated correctly.Verification confirmed:
- Property renamed from
scopetochangeScopeper coding guidelines ✓- All emit calls use
changeScope: 'global'(prompt.registry.ts:374) ✓- All consumer sites updated: notification.service.ts correctly references
event.changeScope(lines 131, 139, 147) ✓- No references to old
.scopeproperty on PromptChangeEvent found in codebase ✓- Tests exist for the subscribe mechanism ✓
- No SDK events API documentation previously existed, so no docs update required ✓
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (4)
19-24: SSE listener stage correctly added to the execution planIncluding
'onSseListener'inexecutematches the new@Stage('onSseListener', ...)below, so the new path is properly wired into the flow lifecycle.
26-30: State schema extension forsseListeneris consistentAdding
'sseListener'to therequestTypeenum aligns with the router branch and theonSseListenerStage filter; no type or routing inconsistencies here.
118-133: Explicithandled()after delegating to transportAdding
this.handled()afterawait transport.handleRequest(request, response);ensures the flow is explicitly marked as handled once the transporter finishes processing, which makes the control flow clearer and consistent with the new SSE listener stage.
135-153: SSE listener stage reuses existing transport and handles missing sessions cleanlyThe new
onSseListenerStage correctly:
- Reads
token/sessionfrom state (set inparseInput),- Looks up an existing
'streamable-http'transporter for the session,- Returns a 404 when no transporter exists, and
- Forwards the GET request to
transport.handleRequestbefore marking the flow as handled.This cleanly enforces “existing session only” semantics for the SSE listener while reusing the existing transport abstraction.
libs/sdk/src/scope/flows/http.request.flow.ts (2)
31-38: Execution plan wiring forhandleDeleteSessionlooks consistentAdding
handleDeleteSessionat the end of the execute pipeline keeps delete-session handling isolated and avoids interfering with other intents; no issues from a flow-ordering standpoint.
86-96: ExpandedcanActivatebase paths correctly cover modern and legacy transportsThe normalized
basePathplus explicit/sseand/messagevariants makes the activation guard consistent with both modern and legacy SSE endpoints; this looks coherent with the routing strategy.
…ety in logging handlers
# Conflicts: # libs/sdk/src/auth/flows/session.verify.flow.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
libs/plugins/src/codecall/tools/describe.tool.ts (1)
96-131: toJSONSchema migration intoJsonSchemamaintains behavior; consider updating the doc comment.The switch to
toJSONSchema(schema)for bothZodTypeinstances and raw shapes wrapped withz.object(...)preserves the previous behavior and keeps failure paths returningnull, which is consistent and safe.One minor polish item: the doc comment still says “Uses Zod v4's built-in
z.toJSONSchema()”, but the implementation now uses the standalonetoJSONSchemahelper. Updating that wording will avoid confusion for future readers; otherwise this helper looks solid.libs/sdk/src/completion/flows/complete.flow.ts (3)
8-11: Consider removing unusedctxfield from input schema.The
ctxfield is parsed but never accessed in any of the flow stages. If it's required for protocol/framework compatibility, consider adding a comment explaining its purpose. Otherwise, removing it would reduce schema complexity.
72-80: Avoid definite assignment assertion; restructure for safer initialization.Per coding guidelines, non-null assertions should be avoided. The
method!declaration relies on the try block always succeeding before the variable is read, but if parsing fails, the throw happens before assignment.- let method!: string; - let params: z.infer<typeof CompleteRequestSchema>['params']; - try { - const inputData = inputSchema.parse(this.rawInput); - method = inputData.request.method; - params = inputData.request.params; - } catch (e) { - throw new InvalidInputError('Invalid Input', e instanceof z.ZodError ? e.issues : undefined); - } + let inputData: z.infer<typeof inputSchema>; + try { + inputData = inputSchema.parse(this.rawInput); + } catch (e) { + throw new InvalidInputError('Invalid Input', e instanceof z.ZodError ? e.issues : undefined); + } + + const method = inputData.request.method; + const params = inputData.request.params;
163-183: Sameas anyissue; consider extracting shared completer logic.The resource branch has the same
as anycast issue as the prompt branch (line 166). Additionally, lines 163-182 duplicate the completer invocation pattern from lines 131-150.Extract a helper method to reduce duplication:
private async invokeCompleter( instance: unknown, argName: string, argValue: string, contextLabel: string, ): Promise<{ values: string[]; total?: number; hasMore?: boolean }> { if ( instance && typeof instance === 'object' && 'getArgumentCompleter' in instance && typeof (instance as { getArgumentCompleter: unknown }).getArgumentCompleter === 'function' ) { const completer = (instance as { getArgumentCompleter: (name: string) => unknown }).getArgumentCompleter(argName); if (typeof completer === 'function') { try { const result = await completer(argValue); return { values: result?.values || [], total: result?.total, hasMore: result?.hasMore, }; } catch (e) { this.logger.warn(`complete: completer failed for ${contextLabel}: ${e}`); } } } return { values: [] }; }libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
51-60: Avoidas anywhen callingtoJSONSchemato keep SDK typings strictUsing
requestedSchema as anyweakens type safety in a published SDK and goes against the “avoidany” guideline. It’s better to align the types so this cast isn’t needed. For example, ifzod/v4exportsZodType, you can import both from the same module and drop theanycast:-import { ZodType } from 'zod'; -import { toJSONSchema } from 'zod/v4'; +import { ZodType, toJSONSchema } from 'zod/v4'; - async sendElicitRequest<T extends ZodType>( + async sendElicitRequest<T extends ZodType>( @@ - requestedSchema: toJSONSchema(requestedSchema as any), + requestedSchema: toJSONSchema(requestedSchema),If
ZodTypeisn’t re‑exported from'zod/v4', another option is to introduce a small, centrally typed wrapper aroundtoJSONSchemaso this method doesn’t need ananyescape hatch. Either way, aim to keep this call fully typed.libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
54-84: Consider removing the definite assignment assertion.Line 58 uses
!which the coding guidelines advise against. While this is safe at runtime (always assigned in the try block before use), you could refactor to avoid it:- let method!: string; - let params: z.infer<typeof UnsubscribeRequestSchema>['params']; - let ctx: unknown; + let method: string; + let params: z.infer<typeof UnsubscribeRequestSchema>['params'] | undefined; + let ctx: unknown | undefined; try { const inputData = inputSchema.parse(this.rawInput); method = inputData.request.method; params = inputData.request.params; ctx = inputData.ctx; } catch (e) { throw new InvalidInputError('Invalid Input', e instanceof z.ZodError ? e.issues : undefined); } + if (!params) { + throw new InvalidInputError('Request params are required'); + } + if (method !== 'resources/unsubscribe') {Note: The same pattern appears in SubscribeResourceFlow, so you may want to refactor both for consistency.
libs/plugins/src/codecall/codecall.types.ts (1)
10-36: Filter tool info and filter function types look solid; consider DRYing shared fieldsThe new
DirectCallsFilterToolInfo/IncludeToolsFilterToolInfoshapes andDirectCallsFilterFn/IncludeToolsFilterFnsignatures are clear and match the rest of the CodeCall metadata. Thez.custom<...>wrappers give you a nice runtime guard that the value is at least a function.Two small, optional nits:
IncludeToolsFilterToolInfoduplicates all the fields fromDirectCallsFilterToolInfo. You could extend the first to keep the common shape in sync more easily:-export interface IncludeToolsFilterToolInfo { - name: string; - appId?: string; - source?: string; - description?: string; - tags?: string[]; -} +export interface IncludeToolsFilterToolInfo extends DirectCallsFilterToolInfo { + description?: string; +}
- For JS consumers (no TS), the runtime predicate can only assert “is a function”, not the full
(tool) => booleansignature, which is fine. Just be aware that mis‑typed/async filters from JS will be caught only at use time, not by Zod.Also applies to: 38-45
libs/sdk/src/common/interfaces/flow.interface.ts (1)
65-67: Tighten the cast inrespondto avoidZodObject<any>and keepFlowOutputOf<N>alignedThe new cast fixes the generic argument requirement but it both (a) assumes the output schema is always an object and (b) erases the concrete output type via
any. You can retain type‑safety and avoid the object‑only assumption by casting to a generic Zod type instead:- respond(output: FlowOutputOf<N>) { - throw FlowControl.respond((this.metadata.outputSchema as z.ZodObject<any>).parse(output)); - } + respond(output: FlowOutputOf<N>) { + throw FlowControl.respond( + (this.metadata.outputSchema as z.ZodType<FlowOutputOf<N>>).parse(output), + ); + }This keeps
parse’s return type in sync withFlowOutputOf<N>without relying onanyor object‑specific typing.libs/sdk/src/common/metadata/tool.metadata.ts (1)
117-122: Avoid duplicating the structured output union between metadata and decorator helpersThe updated
StructuredOutputTypeunion here matches the helper__StructuredOutputTypeintool.decorator.ts(same variants, now with explicit generics). Keeping two separate but equivalent unions increases the chance they’ll diverge on the next change.Consider extracting a single shared alias for this union (e.g., in a small common
tool-output.typesmodule) and reusing it in bothtool.metadata.tsandtool.decorator.tsso future updates only need to touch one place.libs/sdk/src/logging/flows/set-level.flow.ts (1)
15-21: AlignstateSchema’soutputfield with actual state usage
stateSchemadefines a requiredoutput: outputSchemafield, but the flow never populatesstate.outputandfinalizeresponds directly with{}:const stateSchema = z.object({ input: z.object({ level: LoggingLevelSchema }), sessionId: z.string(), output: outputSchema, }); // ... @Stage('finalize') async finalize() { this.respond({}); }To avoid confusion (and potential future reliance on
state.outputthat never gets set), consider either:
- Writing the empty result into state before responding, e.g.
this.state.set({ output: {} as z.infer<typeof outputSchema> });, or- Dropping
outputfromstateSchemaif you don’t intend to track it at the flow‑state level.Either option will make the state shape accurately reflect how the flow is used.
Also applies to: 106-112
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libs/plugins/src/codecall/codecall.types.ts(3 hunks)libs/plugins/src/codecall/services/enclave.service.ts(1 hunks)libs/plugins/src/codecall/tools/describe.tool.ts(3 hunks)libs/plugins/src/codecall/tools/execute.tool.ts(2 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(2 hunks)libs/sdk/src/common/interfaces/flow.interface.ts(1 hunks)libs/sdk/src/common/metadata/tool.metadata.ts(1 hunks)libs/sdk/src/completion/flows/complete.flow.ts(1 hunks)libs/sdk/src/logging/flows/set-level.flow.ts(1 hunks)libs/sdk/src/resource/flows/subscribe-resource.flow.ts(1 hunks)libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts(1 hunks)libs/sdk/src/tool/flows/tools-list.flow.ts(2 hunks)libs/sdk/src/transport/adapters/transport.sse.adapter.ts(2 hunks)libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts(2 hunks)libs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts(2 hunks)libs/ui/src/components/table.schema.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/resource/flows/subscribe-resource.flow.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/plugins/src/codecall/services/enclave.service.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/ui/src/components/table.schema.tslibs/sdk/src/transport/adapters/transport.sse.adapter.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/sdk/src/common/interfaces/flow.interface.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/plugins/src/codecall/codecall.types.tslibs/plugins/src/codecall/tools/describe.tool.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/plugins/src/codecall/tools/execute.tool.tslibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.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/plugins/src/codecall/services/enclave.service.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/ui/src/components/table.schema.tslibs/sdk/src/transport/adapters/transport.sse.adapter.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/sdk/src/common/interfaces/flow.interface.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/plugins/src/codecall/codecall.types.tslibs/plugins/src/codecall/tools/describe.tool.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/plugins/src/codecall/tools/execute.tool.tslibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/transport/adapters/transport.sse.adapter.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/sdk/src/common/interfaces/flow.interface.tslibs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/completion/flows/complete.flow.tslibs/sdk/src/resource/flows/unsubscribe-resource.flow.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts
libs/ui/src/components/**/*.schema.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/components/**/*.schema.ts: Every component must have a Zod schema with.strict()mode to reject unknown properties
Use consistent enum naming for variants ('primary', 'secondary', 'outline', 'ghost', 'danger', 'success') and sizes ('xs', 'sm', 'md', 'lg', 'xl')
Use HTMX schema with strict mode including get, post, put, delete, target, swap, and trigger properties
Files:
libs/ui/src/components/table.schema.ts
libs/ui/src/components/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/components/**/*.ts: Validate component options usingvalidateOptions()helper and return error box on validation failure
Add JSDoc examples with @example tags showing basic usage and options patterns for all components
Use pure HTML string generation without React/Vue/JSX - components return HTML strings only
Files:
libs/ui/src/components/table.schema.ts
libs/ui/src/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.ts: Always useescapeHtml()utility for all user-provided content to prevent XSS vulnerabilities
Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Files:
libs/ui/src/components/table.schema.ts
libs/ui/src/components/**
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
Organize components into schema.ts (definitions) and implementation .ts files with matching names
Files:
libs/ui/src/components/table.schema.ts
🧠 Learnings (10)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Do not mutate rawInput in flows - use state.set() for flow state management instead
Applied to files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/logging/flows/set-level.flow.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/ui/src/components/table.schema.tslibs/plugins/src/codecall/codecall.types.tslibs/plugins/src/codecall/tools/describe.tool.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/logging/flows/set-level.flow.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Use HTMX schema with strict mode including get, post, put, delete, target, swap, and trigger properties
Applied to files:
libs/ui/src/components/table.schema.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/** : Organize components into schema.ts (definitions) and implementation .ts files with matching names
Applied to files:
libs/ui/src/components/table.schema.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Use consistent enum naming for variants ('primary', 'secondary', 'outline', 'ghost', 'danger', 'success') and sizes ('xs', 'sm', 'md', 'lg', 'xl')
Applied to files:
libs/ui/src/components/table.schema.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Applied to files:
libs/ui/src/components/table.schema.tslibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/sdk/src/logging/flows/set-level.flow.tslibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Use type parameters with constraints instead of unconstrained generic types
Applied to files:
libs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/tool.decorator.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methods
Applied to files:
libs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts
🧬 Code graph analysis (4)
libs/sdk/src/tool/flows/tools-list.flow.ts (1)
libs/cli/src/templates/3rd-party-integration/src/tools/example.list.ts (1)
inputSchema(12-17)
libs/sdk/src/logging/flows/set-level.flow.ts (2)
libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
Flow(44-128)libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
Flow(44-112)
libs/sdk/src/completion/flows/complete.flow.ts (3)
libs/sdk/src/common/decorators/hook.decorator.ts (1)
FlowHooksOf(187-194)libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
Flow(44-128)libs/sdk/src/auth/flows/session.verify.flow.ts (1)
Flow(78-326)
libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (1)
libs/sdk/src/resource/flows/subscribe-resource.flow.ts (1)
Flow(44-128)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (18)
libs/plugins/src/codecall/tools/describe.tool.ts (1)
2-5: Zod v4 JSON‑schema import looks correct; watch for multiple Zod instances.Importing
toJSONSchemafrom'zod/v4'aligns with the v4 JSON‑schema helper and matches the broader migration. Just ensure the project doesn’t end up with separate runtime copies of Zod (e.g., some code importing from'zod'and others from a different bundled instance), otherwiseinstanceof ZodTypechecks might behave unexpectedly if schemas ever come from another copy.Please verify via your bundler/runtime (or a quick dependency tree check) that
'zod'and'zod/v4'resolve to the same package instance so theinstanceof ZodTypeguard remains reliable.libs/sdk/src/tool/flows/tools-list.flow.ts (1)
4-6: JSON‑schema conversion change is behavior‑preserving and consistent with the repo‑wide Zod v4 pattern.Switching from
z.toJSONSchema(z.object(tool.inputSchema))to the standalonetoJSONSchema(z.object(tool.inputSchema))keeps the same runtime semantics, and the existing error handling/fallback to an empty object schema remains intact. The added import from'zod/v4'is appropriate here and matches the rest of the migration.No further changes needed from a flow or MCP‑typing perspective.
Given this depends on the Zod v4
toJSONSchemahelper, please run the existing tools‑list flow tests (or an end‑to‑end “list tools” call) to confirm the generated JSON Schemas for a few representative tools are identical to the previous behavior.Also applies to: 172-187
libs/sdk/src/completion/flows/complete.flow.ts (3)
37-56: LGTM!The plan structure and global type augmentation follow established patterns consistent with other flows in the codebase.
185-196: LGTM!The completion result building correctly uses
this.state.set()for state management, following the coding guideline to not mutate rawInput. The conditional field addition pattern is clean.
198-204: LGTM!The finalize stage follows the established pattern seen in other flows like
subscribe-resource.flow.ts.libs/plugins/src/codecall/services/enclave.service.ts (1)
71-75: Config access refactor viagetAll()looks goodUsing
config.getAll()once and assigningresolvedVm/sidecarkeeps behavior equivalent to the previousconfig.get(...)calls while simplifying typing and avoiding deep dotted paths. No correctness or API-surface changes here.libs/plugins/src/codecall/tools/execute.tool.ts (1)
198-198: Timeout messagegetAll()usage preserves behaviorSwitching from
config.get('resolvedVm.timeoutMs')toconfig.getAll().resolvedVm.timeoutMsis consistent with the new pattern and keeps runtime behavior the same, givenconfigis required viathis.get(CodeCallConfig).libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
6-7: Zod v4toJSONSchemaimport looks consistent; please verify module resolutionImporting
toJSONSchemafrom'zod/v4'and keepingZodTypeseparate matches the described migration away fromz.toJSONSchema. Just make sure this subpath is supported across your build targets (Node, bundler, ESM/CJS) so consumers oflibs/sdkdon’t hit resolution issues.libs/sdk/src/resource/flows/unsubscribe-resource.flow.ts (4)
1-6: LGTM!The imports are well-organized and use typed protocol definitions from the MCP SDK. The specific error classes align with the coding guidelines.
8-27: LGTM!The schemas correctly use MCP SDK types and the plan structure matches the implemented stages. The state schema appropriately defines the internal flow state.
86-103: LGTM!The implementation correctly follows the MCP specification for
resources/unsubscribeby succeeding silently regardless of whether the resource exists or the session was subscribed. The use ofscope.notifications.unsubscribeResourceis consistent with the subscription flow pattern.
29-42: LGTM!The flow metadata, global interface augmentation, and finalize stage are all correctly implemented. The empty result response aligns with the MCP specification for
resources/unsubscribe, and theauthorizedaccess level is appropriate for resource operations.Also applies to: 44-50, 105-111
libs/sdk/src/transport/adapters/transport.sse.adapter.ts (1)
6-7: Fix incorrect Zod v4 import pattern—use theznamespace consistently.The current mixed import pattern violates Zod v4 conventions. According to Zod v4 documentation, both
ZodTypeandtoJSONSchemashould be accessed from theznamespace:import { z } from 'zod/v4';Then use
z.ZodTypeandz.toJSONSchema()throughout the file. The current imports from separate sources (ZodTypefrom'zod'andtoJSONSchemafrom'zod/v4') are inconsistent and not the recommended pattern for Zod v4.Additionally, this import inconsistency likely necessitated the
as anycast on line 60. Once imports are corrected, verify that the type safety can be improved by removing the cast.⛔ Skipped due to learnings
Learnt from: CR Repo: agentfront/frontmcp PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T17:22:58.085Z Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata levelLearnt from: CR Repo: agentfront/frontmcp PR: 0 File: libs/ui/CLAUDE.md:0-0 Timestamp: 2025-11-26T15:23:04.965Z Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown propertiesLearnt from: CR Repo: agentfront/frontmcp PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T17:22:58.085Z Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methodsLearnt from: CR Repo: agentfront/frontmcp PR: 0 File: libs/ui/CLAUDE.md:0-0 Timestamp: 2025-11-26T15:23:04.965Z Learning: Use Zod schema validation for all component inputs as a core validation strategylibs/sdk/src/transport/mcp-handlers/mcp-handlers.types.ts (1)
29-30: No changes needed —z.ZodObject<any>is the correct pattern for Zod v4.The review comment suggests using
unknowninstead ofany, but this conflicts with Zod v4's type architecture. In Zod v4,ZodObject's first generic parameter must be aZodRawShape(a map of Zod schemas), not an arbitrary type. Usingz.ZodObject<unknown>would be invalid.The pattern
z.ZodObject<any>is the documented best practice from the Zod community for accepting generic Zod objects with any shape. This is a justified use ofanyper the coding guidelines ("without strong justification"), as it's required by the library's type system. Concrete handler implementations use typed schemas (e.g.,InitializeRequestSchema), so type safety is preserved at usage sites.libs/plugins/src/codecall/codecall.types.ts (1)
118-122: Perfect! I now have comprehensive information to verify the review comment. Let me create the rewritten review comment based on my findings.
Documentation for filter function signatures must be updated to match implementation
The schema wiring is correctly implemented—
directCalls.filterandincludeToolsthroughdirectCallsFilterSchemaandincludeToolsFilterSchemaproperly export the inferred typesDirectCallsFilterFnandIncludeToolsFilterFn, keeping consumers' types aligned.However, the CodeCall configuration docs (
docs/draft/docs/plugins/codecall/configuration.mdx) contain outdated information:
The "Global Tool Filtering" section documents
includeToolsas receiving aToolEntryinterface with nestedmetadatastructure, but the actual implementation passesIncludeToolsFilterToolInfowith flat properties:{ name, appId?, source?, description?, tags? }.The
directCalls.filterfunction signature is not documented at all. It should document theDirectCallsFilterToolInfoparameter shape:{ name, appId?, source?, tags? }.Action: Update
docs/draft/docs/plugins/codecall/configuration.mdxto explicitly referenceDirectCallsFilterToolInfoandIncludeToolsFilterToolInfotypes in the filter function documentation, replacing the outdatedToolEntryinterface reference. This aligns docs with implementation and ensures SDK consumers understand the correct filter parameter shapes.libs/sdk/src/common/decorators/tool.decorator.ts (1)
75-76: Type helper updates are consistent with the metadata side and don’t change behaviorThe tweaks to
__AsZodObjand__StructuredOutputTypekeep the decorator’s inference helpers aligned with the metadata definitions while fixing the missing‑generic issue. They still enforce the same set of allowed input/output schema shapes, so this looks good and non‑breaking.Also applies to: 151-155
libs/sdk/src/logging/flows/set-level.flow.ts (2)
54-84: parseInput stage follows the established flow pattern and validates context correctly
parseInputcleanly follows the existing flow conventions: it validates againstinputSchema, maps validation failures toInvalidInputError, enforces the expected"logging/setLevel"method, extractssessionIdfromctx, and persists{ input: params, sessionId }viathis.state.set(...)without mutatingrawInput. This is consistent with the subscribe/unsubscribe flows and looks solid.
86-104: setLevel now surfaces internal failures instead of returning silent successThe
setLevelstage now treats afalsereturn fromscope.notifications.setLogLevel(...)as an error, logging a warning and throwingGenericServerError. This fixes the earlier behavior where failures still produced an empty success result and aligns with the MCP expectation that internal failures are returned as JSON‑RPC/MCP errors rather than silent success.Please just confirm that
GenericServerErroris wired to the MCP internal error code (e.g.,-32603) for logging/setLevel failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/plugins/src/codecall/__tests__/execute.tool.test.ts (2)
101-118: Avoidoverrides.resolvedVmclobbering the merged nestedresolvedVmconfigIn
defaults, the spread orderconst defaults = { resolvedVm: { /* merged defaults + overrides */ }, ...overrides, };means that if
overridescontains a top‑levelresolvedVmobject, it will overwrite the carefully mergedresolvedVm(includingresolvedVm.*flat overrides). That can silently drop theresolvedVmOverrideslogic.Consider separating
resolvedVmfrom the rest of the overrides so the merged structure always wins, e.g.:-function createMockConfig(overrides: Record<string, unknown> = {}) { +function createMockConfig(overrides: Record<string, unknown> = {}) { + const { resolvedVm: resolvedVmOverride, ...otherOverrides } = overrides; // Build nested structure from flat dot-notation overrides const resolvedVmOverrides: Record<string, unknown> = {}; for (const [key, value] of Object.entries(overrides)) { if (key.startsWith('resolvedVm.')) { const nestedKey = key.slice('resolvedVm.'.length); resolvedVmOverrides[nestedKey] = value; } } - const defaults = { - resolvedVm: { - timeoutMs: 5000, - allowConsole: false, - ...((overrides['resolvedVm'] as Record<string, unknown>) || {}), - ...resolvedVmOverrides, - }, - ...overrides, - }; + const defaults = { + ...otherOverrides, + resolvedVm: { + timeoutMs: 5000, + allowConsole: false, + ...((resolvedVmOverride as Record<string, unknown>) || {}), + ...resolvedVmOverrides, + }, + };You may also want to strip
resolvedVm.*keys out ofotherOverridesif you wantgetAll()to mirror the real nested config shape without duplicated flat keys.Also applies to: 121-123, 126-128
688-690: PreferconfigOverridesover per‑test stubbing ofgetAllHere you directly override
mockConfig.getandmockConfig.getAll:mockConfig.getAll.mockReturnValue({ resolvedVm: { timeoutMs: 5000, allowConsole: true }, });Given
createMockConfigalready supports flatresolvedVm.*overrides and wires them into bothgetandgetAll, you can make this test closer to the realCodeCallConfigbehavior by passingconfigOverrideswhen creating the tool instead of re‑mocking methods, e.g.:-const { tool, mockEnclave, mockConfig } = createExecuteTool(); +const { tool, mockEnclave } = createExecuteTool({ + configOverrides: { + 'resolvedVm.timeoutMs': 5000, + 'resolvedVm.allowConsole': true, + }, +});This keeps the test intent the same (console enabled) while relying on the shared helper logic instead of duplicating config behavior in individual tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/plugins/src/codecall/__tests__/execute.tool.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/plugins/src/codecall/__tests__/execute.tool.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use Jest with 95%+ coverage requirement enforced through test configuration
Include constructor validation tests and error class instanceof checks in all test suites
Do not use prefixes like PT-001 in test names - use descriptive test names instead
Test all code paths including error cases and edge conditions
Files:
libs/plugins/src/codecall/__tests__/execute.tool.test.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/plugins/src/codecall/__tests__/execute.tool.test.ts
🧠 Learnings (1)
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/plugins/src/codecall/__tests__/execute.tool.test.ts
⏰ 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). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/plugins/src/codecall/tools/search.schema.ts (1)
4-21: Update documentation to match new default values.The description on line 16 states
"topK?: number (default 5)"but the schema (line 31) now defaults to 10. Similarly, line 17 mentions"minRelevanceScore?: number (default 0.3)"but the schema (line 37) now defaults to 0.1.Apply this diff to sync the documentation:
-topK?: number (default 5) - results per query -minRelevanceScore?: number (default 0.3) - minimum match threshold +topK?: number (default 10) - results per query +minRelevanceScore?: number (default 0.1) - minimum match threshold
🧹 Nitpick comments (2)
libs/plugins/src/codecall/codecall.types.ts (1)
38-45: Runtime validation only checks function type, not signature.The
z.customvalidators verify the value is a function but cannot introspect the actual signature (parameter types, arity, return type) at runtime. TypeScript provides compile-time safety, but runtime validation is inherently limited in JavaScript.Consider updating the error messages to reflect this limitation more accurately, or accept this as a reasonable trade-off.
libs/sdk/src/tool/flows/tools-list.flow.ts (1)
181-183: Remove or update the outdated comment.The comment on lines 181-182 references "as any used here" but line 183 doesn't contain an
as anycast. This comment appears to be outdated and should be removed or updated to accurately describe the current code.Apply this diff to remove the outdated comment:
- } else if (tool.inputSchema && Object.keys(tool.inputSchema).length > 0) { - // tool.inputSchema is a ZodRawShape (extracted .shape from ZodObject in ToolInstance constructor) - // Convert to JSON Schema - try { - // as any used here to prevent hard ts-check on tool input that is redundant - // and just slow down the build process. types here are unnecessary. - inputSchema = toJSONSchema(z.object(tool.inputSchema)); + } else if (tool.inputSchema && Object.keys(tool.inputSchema).length > 0) { + // tool.inputSchema is a ZodRawShape (extracted .shape from ZodObject in ToolInstance constructor) + // Convert to JSON Schema + try { + inputSchema = toJSONSchema(z.object(tool.inputSchema));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/plugins/src/codecall/codecall.types.ts(4 hunks)libs/plugins/src/codecall/tools/execute.tool.ts(3 hunks)libs/plugins/src/codecall/tools/search.schema.ts(1 hunks)libs/sdk/src/prompt/flows/prompts-list.flow.ts(2 hunks)libs/sdk/src/resource/flows/resource-templates-list.flow.ts(2 hunks)libs/sdk/src/resource/flows/resources-list.flow.ts(2 hunks)libs/sdk/src/tool/flows/tools-list.flow.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/plugins/src/codecall/tools/execute.tool.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/plugins/src/codecall/tools/search.schema.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/plugins/src/codecall/codecall.types.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.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/flows/tools-list.flow.tslibs/plugins/src/codecall/tools/search.schema.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/plugins/src/codecall/codecall.types.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Do not mutate rawInput in flows - use state.set() for flow state management instead
Applied to files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/sdk/src/tool/flows/tools-list.flow.tslibs/plugins/src/codecall/codecall.types.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methods
Applied to files:
libs/sdk/src/prompt/flows/prompts-list.flow.ts
🧬 Code graph analysis (4)
libs/sdk/src/tool/flows/tools-list.flow.ts (2)
libs/cli/src/templates/3rd-party-integration/src/tools/example.list.ts (1)
inputSchema(12-17)libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(17-41)
libs/sdk/src/prompt/flows/prompts-list.flow.ts (1)
libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(17-41)
libs/sdk/src/resource/flows/resources-list.flow.ts (1)
libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(17-41)
libs/sdk/src/resource/flows/resource-templates-list.flow.ts (1)
libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(17-41)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (10)
libs/plugins/src/codecall/codecall.types.ts (5)
5-26: LGTM: Well-defined filter interfaces.The filter info interfaces are cleanly typed with appropriate optional fields and clear documentation.
28-36: LGTM: Clean function type definitions.The filter function types provide clear type safety at compile time.
106-122: LGTM: Cleaner filter schema integration.The refactored filter field using the custom schema is more maintainable than inline
z.function()definitions.
225-270: LGTM: Consistent filter pattern.The
includeToolsfilter follows the same clean pattern asdirectCalls.
286-286: Verify the migration to.prefault()is intentional and complete.According to Zod 4 documentation,
.prefault()applies defaults before parsing (old Zod 3 behavior), while.default()now short-circuits when input is undefined. This behavioral change affects how defaults interact with transformations.Other schemas in this file (lines 104, 163, 219) still use
.default(() => {...}). Ensure this mixed approach is intentional and that the new.prefault()behavior aligns with your requirements.Based on the Zod 4 library documentation provided.
libs/plugins/src/codecall/tools/search.schema.ts (1)
31-31: Verify the impact of significantly lowered relevance threshold.The
minRelevanceScoredefault has been reduced from 0.3 to 0.1 (a 70% reduction), andtopKhas doubled from 5 to 10. These changes will substantially alter search behavior:
- Lower threshold includes tools with weaker semantic matches
- Doubled result count increases coverage but may overwhelm users
Ensure this retuning aligns with observed user needs and that the trade-off between recall and precision is acceptable.
Also applies to: 37-38
libs/sdk/src/resource/flows/resources-list.flow.ts (1)
3-3: LGTM! Consistent FlowControl handling.The addition of
FlowControlto imports and the rethrow guard in the catch block correctly ensures that flow control exceptions (respond, next, handled, fail, abort) propagate without being logged as errors. This pattern is consistently applied across all flow files in this PR.Also applies to: 192-192
libs/sdk/src/resource/flows/resource-templates-list.flow.ts (1)
3-3: LGTM! Consistent FlowControl handling.The FlowControl import and rethrow guard follow the same correct pattern as the other flow files, ensuring flow control mechanisms propagate properly without being logged as errors.
Also applies to: 192-192
libs/sdk/src/tool/flows/tools-list.flow.ts (1)
2-2: LGTM! Consistent FlowControl handling.The FlowControl import and rethrow guard are correctly implemented, matching the pattern used across other flow files in this PR.
Also applies to: 210-210
libs/sdk/src/prompt/flows/prompts-list.flow.ts (1)
3-3: LGTM! Consistent FlowControl handling.The FlowControl import and rethrow guard correctly implement the established pattern, ensuring flow control exceptions propagate properly without being logged as regular errors.
Also applies to: 185-185
| import { Flow, FlowBase, FlowControl, FlowHooksOf, FlowPlan, FlowRunOptions, ToolEntry } from '../../common'; | ||
| import 'reflect-metadata'; | ||
| import { z } from 'zod'; | ||
| import { toJSONSchema } from 'zod/v4'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Zod 4 toJSONSchema export path subpath imports
💡 Result:
Short answer — use the v4 subpath and call z.toJSONSchema(...) (or use the third‑party zod-to-json-schema if you need extra options).
Examples
-
Zod v4 built‑in:
import { z } from "zod/v4";
const s = z.object({ name: z.string() });
const json = z.toJSONSchema(s); // returns JSON Schema for s. [1] -
If you prefer the external converter (different API/naming):
import { zodToJsonSchema } from "zod-to-json-schema";
const json = zodToJsonSchema(s, "MySchema");. [3]
Notes
- Zod 4 exposes subpath imports (e.g. "zod/v4") intentionally so libraries can import a specific major version. Use "zod/v4" to ensure v4 behavior. [2]
Sources
[1] Zod docs: JSON Schema (z.toJSONSchema). (zod.nodejs.cn)
[2] Zod v4 subpath export/versioning explanation. (deepwiki.com)
[3] zod-to-json-schema package README / npm. (npm.io)
Citations:
- 1: https://zod.nodejs.cn/json-schema?utm_source=openai
- 2: https://deepwiki.com/colinhacks/zod/5.4-zod-4-release-and-migration?utm_source=openai
- 3: https://npm.io/package/zod-to-json-schema?utm_source=openai
🏁 Script executed:
# Check the actual file to see the import and context
find . -type f -name "tools-list.flow.ts" | head -5Repository: agentfront/frontmcp
Length of output: 107
🏁 Script executed:
cat -n ./libs/sdk/src/tool/flows/tools-list.flow.ts | head -20Repository: agentfront/frontmcp
Length of output: 864
🏁 Script executed:
cat -n ./libs/sdk/src/tool/flows/tools-list.flow.ts | sed -n '175,190p'Repository: agentfront/frontmcp
Length of output: 1068
Fix the toJSONSchema import path.
Zod 4 does not export toJSONSchema as a named export from 'zod/v4'. According to Zod 4 documentation, use z.toJSONSchema() by importing the z object: import { z } from 'zod/v4' and call it as z.toJSONSchema(z.object(...)). Alternatively, use the third-party package zod-to-json-schema for additional conversion options.
Also applies to: 183-183
🤖 Prompt for AI Agents
In libs/sdk/src/tool/flows/tools-list.flow.ts around lines 5 and 183, the code
imports toJSONSchema from 'zod/v4' which is incorrect for Zod v4; change the
import to import { z } from 'zod/v4' and replace uses of toJSONSchema(...) with
z.toJSONSchema(...) (passing the z schema), or alternatively switch to using the
zod-to-json-schema package if you need its extra options; update both
occurrences (line 5 and line 183) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/plugins/src/codecall/services/tool-search.service.ts (1)
315-367: Synonym expansion configuration viaembeddingOptionsis wired but non-functional—ensure this is intentionalThe constructor builds
embeddingOptions(with a defaultsynonymExpansionobject containing{ enabled: true, replaceDefaults: false, maxExpansionsPerTerm: 5 }), yetsynonymExpansionis completely ignored duringsynonymServiceinitialization. Only the top-levelconfig.synonymExpansionis checked:if (config.synonymExpansion === false) { this.synonymService = null; } else if (this.strategy === 'tfidf') { const synonymConfig = typeof config.synonymExpansion === 'object' ? config.synonymExpansion : {}; if (synonymConfig.enabled !== false) { this.synonymService = new SynonymExpansionService(synonymConfig); } }Since the plugin passes only
{ embeddingOptions: parsedOptions.embedding, mode, includeTools }and never extractsparsedOptions.embedding.synonymExpansionto the top-level config, any user configuration atembedding.synonymExpansionsilently has no effect.Either bridge the configuration by extracting
embeddingOptions.synonymExpansiontoconfig.synonymExpansionin the plugin, or removesynonymExpansionfromCodeCallEmbeddingOptionsto avoid the appearance of a working public API knob.
🧹 Nitpick comments (7)
libs/plugins/src/codecall/services/synonym-expansion.service.ts (2)
19-307: Multi‑word synonyms in DEFAULT_SYNONYM_GROUPS are not reachable viaexpandQuery/hasExpansions
expandQueryandhasExpansionssplit on whitespace and look up single tokens, so entries like'turn on','switch on','audit trail','access right'inDEFAULT_SYNONYM_GROUPScan only ever be used if callers pass the exact phrase as a single token. For typical natural‑language queries, those phrases will be split and never match the map keys.If phrase‑level synonyms are important, consider either:
- Changing tokenization to preserve/recognize multi‑word phrases, or
- Splitting multi‑word entries into their constituent single‑word synonyms, or
- Moving phrase synonyms into a separate mechanism (e.g., rule‑based rewrites) so expectations are clear.
Right now behavior is consistently word‑level despite the comment promising phrase coverage.
341-453: Service config type here diverges from publicSynonymExpansionConfigtype incodecall.types.tsThis file exports an interface
SynonymExpansionConfigwithout anenabledflag, whilecodecall.types.tsalso exports aSynonymExpansionConfig(Zod‑inferred) that does includeenabled. The shapes are structurally compatible, but the duplicate name with slightly different fields is easy to misread.Consider unifying on a single canonical config type (e.g., import the Zod‑inferred type here, or move this interface there and re‑export), or renaming one of them to make the distinction explicit (e.g.,
SynonymExpansionRuntimeConfigvsSynonymExpansionOptions).libs/plugins/src/codecall/codecall.types.ts (4)
5-45: Filter function types and schemas look good; keep shapes in sync with runtime filter usageThe
DirectCallsFilterToolInfo/IncludeToolsFilterToolInfoshapes and correspondingDirectCallsFilterFn/IncludeToolsFilterFnplusz.customwrappers are reasonable: they give strong TS types while only enforcing “is a function” at runtime.Just be careful to keep these in lockstep with the
IncludeToolsFilterobject shape used intool-search.service.ts(name/appId/source/description/tags). If either side adds/removes fields, update both to avoid subtle drift between validated input and what the search layer actually passes.
126-157: Avoid duplicating synonymExpansion defaults in multiple places
synonymExpansionConfigSchema.default(...)encodes a default, and then the same{ enabled: true, replaceDefaults: false, maxExpansionsPerTerm: 5 }object is repeated again in:
DEFAULT_EMBEDDING_OPTIONS.synonymExpansioncodeCallEmbeddingOptionsSchema.shape.synonymExpansion.default(...)This triple‑encoding is easy to desync later.
Consider introducing a single
const DEFAULT_SYNONYM_EXPANSION = {...}and reuse it in:
synonymExpansionConfigSchema.default(DEFAULT_SYNONYM_EXPANSION)DEFAULT_EMBEDDING_OPTIONS.synonymExpansionsynonymExpansionfield.default(DEFAULT_SYNONYM_EXPANSION)That keeps behavior identical but centralizes the default.
159-208: Embedding options add public synonymExpansion config; ensure it’s documented and actually wired through
codeCallEmbeddingOptionsSchemaandDEFAULT_EMBEDDING_OPTIONSnow expose asynonymExpansionfield (false | SynonymExpansionConfigwith sensible defaults). That’s a public configuration surface for consumers of the CodeCall plugin.Two follow‑ups to verify:
- The docs for CodeCall embedding options should now describe
synonymExpansion(shape, defaults, and “TF‑IDF only” behavior).- Wherever
CodeCallEmbeddingOptionsis turned into aToolSearchServiceConfig, make sureembedding.synonymExpansionis actually plumbed into thesynonymExpansionfield onToolSearchServiceConfig(right now this file only defines the type; the wiring logic lives elsewhere).If wiring is missing, users configuring synonym expansion via plugin options will see no effect.
330-349: Two exportedSynonymExpansionConfigtypes with different definitions can confuse consumersThis file exports
type SynonymExpansionConfig = z.infer<typeof synonymExpansionConfigSchema>;, whilesynonym-expansion.service.tsexports an interface of the same name without theenabledfield.Even though TS’s structural typing makes them assignable, having two public types with the same name but different members is a recipe for confusion when imported from different modules.
Consider consolidating on one exported
SynonymExpansionConfigand reusing it from both:
- The plugin types (
codecall.types.ts)- The runtime service (
synonym-expansion.service.ts)or renaming one of them to clarify intent (e.g.,
SynonymExpansionOptionsvsSynonymExpansionRuntimeConfig).libs/plugins/src/codecall/services/tool-search.service.ts (1)
18-199: Doc comment overstates what this module alone definesThe “Universal Intent Mapping & Query Normalization” block documents
DEFAULT_SYNONYM_GROUPSas if it were defined here, but the synonym groups now live insynonym-expansion.service.ts; this file only ownsSTOP_WORDS.Not a functional issue, but to avoid confusion it may be clearer to either:
- Rephrase to say this service layer uses both the synonym groups (in the SynonymExpansionService module) and the STOP_WORDS here, or
- Move the detailed description of synonym groups next to their actual definition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/plugins/src/codecall/__tests__/synonym-expansion.service.test.ts(1 hunks)libs/plugins/src/codecall/__tests__/tool-search.service.test.ts(1 hunks)libs/plugins/src/codecall/codecall.types.ts(6 hunks)libs/plugins/src/codecall/services/index.ts(1 hunks)libs/plugins/src/codecall/services/synonym-expansion.service.ts(1 hunks)libs/plugins/src/codecall/services/tool-search.service.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/__tests__/synonym-expansion.service.test.tslibs/plugins/src/codecall/services/synonym-expansion.service.tslibs/plugins/src/codecall/services/index.tslibs/plugins/src/codecall/codecall.types.tslibs/plugins/src/codecall/services/tool-search.service.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use Jest with 95%+ coverage requirement enforced through test configuration
Include constructor validation tests and error class instanceof checks in all test suites
Do not use prefixes like PT-001 in test names - use descriptive test names instead
Test all code paths including error cases and edge conditions
Files:
libs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/__tests__/synonym-expansion.service.test.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/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/__tests__/synonym-expansion.service.test.tslibs/plugins/src/codecall/services/synonym-expansion.service.tslibs/plugins/src/codecall/services/index.tslibs/plugins/src/codecall/codecall.types.tslibs/plugins/src/codecall/services/tool-search.service.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/__tests__/synonym-expansion.service.test.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/plugins/src/codecall/services/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Applied to files:
libs/plugins/src/codecall/services/index.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/plugins/src/codecall/codecall.types.ts
🧬 Code graph analysis (4)
libs/plugins/src/codecall/__tests__/tool-search.service.test.ts (1)
libs/plugins/src/codecall/services/tool-search.service.ts (1)
ToolSearchService(300-728)
libs/plugins/src/codecall/__tests__/synonym-expansion.service.test.ts (1)
libs/plugins/src/codecall/services/synonym-expansion.service.ts (1)
SynonymExpansionService(341-454)
libs/plugins/src/codecall/codecall.types.ts (1)
libs/plugins/src/codecall/services/synonym-expansion.service.ts (1)
SynonymExpansionConfig(312-331)
libs/plugins/src/codecall/services/tool-search.service.ts (2)
libs/plugins/src/codecall/codecall.types.ts (1)
SynonymExpansionConfig(341-341)libs/plugins/src/codecall/services/synonym-expansion.service.ts (2)
SynonymExpansionConfig(312-331)SynonymExpansionService(341-454)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (6)
libs/plugins/src/codecall/services/index.ts (1)
3-8: ExposeSynonymExpansionServicevia barrel is correctRe‑exporting
./synonym-expansion.servicefrom the services index keeps the public surface discoverable and avoids legacy aliases, in line with the barrel‑file guidance.libs/plugins/src/codecall/__tests__/tool-search.service.test.ts (1)
549-702: Synonym expansion tests are comprehensive and aligned with intended behaviorThe new “Synonym expansion” suite exercises:
- Default enablement for TF‑IDF (
add user→users:create).- Specific verb mappings (
add→create,remove→delete,fetch→get).- Disabling via
synonymExpansion: false.- Custom synonym groups via
synonymExpansion.additionalSynonyms.- ML strategy not relying on the synonym service.
This gives good coverage of the new config surface and TF‑IDF vs ML behavior. No changes needed here.
libs/plugins/src/codecall/__tests__/synonym-expansion.service.test.ts (1)
3-220: SynonymExpansionService test coverage is strong and matches the implementationThe suite validates:
- Core API (
getSynonyms,expandQuery,hasExpansions,getStats).- Case‑insensitivity, bidirectionality, and
maxExpansionsPerTerm.- Config behavior (
additionalSynonyms, merge vsreplaceDefaults).- Representative default groups and “real‑world” phrases.
This gives high confidence in the current token‑based implementation. No changes needed from a testing perspective.
libs/plugins/src/codecall/services/tool-search.service.ts (3)
248-293: Public config surface for synonymExpansion on ToolSearchServiceConfig is reasonableAdding
synonymExpansion?: false | (SynonymExpansionConfig & { enabled?: boolean })with clear documentation (“TF‑IDF only; ML already semantic”) gives consumers an explicit way to toggle and tune synonym behavior.The
{ enabled?: boolean }wrapper is a nice touch to distinguish “provided config but disabled” from “no opinion, use defaults.” No changes needed here.
301-334: Internal config typing is careful; excluding synonymExpansion from Required<> is appropriateThe revised
configfield excludesincludeTools,mode, andsynonymExpansionfromRequired<>, then reintroducesmodeandincludeToolswith the desired semantics. That keepssynonymExpansionas purely constructor‑only behavior, instead of part of the internal required config, which matches how the rest of the class uses it.Implementation looks consistent.
633-672: Query expansion integration is straightforward and boundedUsing:
const effectiveQuery = this.synonymService ? this.synonymService.expandQuery(query) : query; const results = await this.vectorDB.search(effectiveQuery, ...);
- Leaves ML strategy untouched (
synonymServiceis null there).- Keeps TF‑IDF expansion bounded via
maxExpansionsPerTermin the service.- Preserves backward compatibility when synonymExpansion is disabled.
This is a clean integration point; behavior matches the tests.
… LLM understanding
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
docs/draft/docs/servers/server.mdx (1)
48-57: Excellent additions to documentation clarity! The info and logging field description tables are well-structured and make the configuration options immediately actionable. Great use of the draft docs to expand guidance.One minor inconsistency to align: Line 77 shows
LogLeveloptions as"Debug | VERBOSE | Info | Warn | Error | Off"(withVERBOSEin uppercase), but the logging table on line 206 lists them as"Debug, Verbose, Info, Warn, Error, Off"(title case). Please standardize to match your enum definition.- level?: LogLevel.Info, // Debug | VERBOSE | Info | Warn | Error | Off + level?: LogLevel.Info, // Debug | Verbose | Info | Warn | Error | OffAlso applies to: 204-210
docs/draft/docs/servers/apps.mdx (1)
1-138: Consider adding cross-references to related documentation pages.The file now covers architectural motivation, minimal setup, field reference, and best practices—all excellent. A minor enhancement would be to add brief links or "See also" pointers to:
- Auth & isolation — link to relevant sections explaining
splitByApp, auth scoping, and multi-tenant patterns- Adapters — reference the adapter/OpenAPI documentation for the example on line 105–107
- Tools & Resources — link to those respective guides to help readers understand how they fit within Apps
This would improve cross-navigation without cluttering the current content.
apps/demo/src/apps/crm/tools/users-create.tool.ts (1)
26-37: Consider more complete output examples.The examples provide good input coverage, but the output is simplified (only
idandsuccess). According to the full output schema (lines 15-25), the user object includesname,role,status,createdAt, and optionallastLoginAt.Consider showing more complete output examples to better demonstrate the tool's full behavior:
examples: [ { description: 'Create an admin user', input: { email: 'admin@company.com', name: 'Admin', role: 'admin' }, - output: { user: { id: '123' }, success: true }, + output: { + user: { + id: '123', + email: 'admin@company.com', + name: 'Admin', + role: 'admin', + status: 'pending', + createdAt: '2024-01-01T00:00:00Z' + }, + success: true + }, },libs/sdk/src/common/metadata/tool.metadata.ts (2)
137-142: Consider using a more specific type constraint instead ofany.The use of
z.ZodObject<any>violates the coding guideline to avoidanytypes. While I understand this is needed for type compatibility with various Zod object shapes, consider usingz.ZodObject<z.ZodRawShape>or a similar constrained type.As per coding guidelines: "Use strict type checking in TypeScript - avoid
anytypes without strong justification".type StructuredOutputType = | z.ZodRawShape - | z.ZodObject<any> + | z.ZodObject<z.ZodRawShape> | z.ZodArray<z.ZodType> - | z.ZodUnion<[z.ZodObject<any>, ...z.ZodObject<any>[]]> - | z.ZodDiscriminatedUnion<z.ZodObject<any>[]>; + | z.ZodUnion<[z.ZodObject<z.ZodRawShape>, ...z.ZodObject<z.ZodRawShape>[]]> + | z.ZodDiscriminatedUnion<z.ZodObject<z.ZodRawShape>[]>;
240-244: Consider exportingtoolExampleSchemafor external validation use cases.The schema is defined but not exported. If consumers need to validate tool examples independently (e.g., in adapters or plugins), they would benefit from accessing this schema directly.
-const toolExampleSchema = z.object({ +export const toolExampleSchema = z.object({ description: z.string(), input: z.record(z.string(), z.unknown()), output: z.unknown().optional(), });libs/plugins/src/codecall/__tests__/describe.tool.test.ts (1)
663-691: Consider adding a test for exactly 5 user-provided examples.The current tests cover <5 and >5 user examples, but there's no explicit test for exactly 5 user examples where no smart generation should occur.
Consider adding a test case:
it('should not add smart-generated example when exactly 5 user examples provided', async () => { const mockTools = [ createMockTool({ name: 'users:create', metadata: { examples: [ { description: 'Example 1', input: { email: 'a@a.com' } }, { description: 'Example 2', input: { email: 'b@b.com' } }, { description: 'Example 3', input: { email: 'c@c.com' } }, { description: 'Example 4', input: { email: 'd@d.com' } }, { description: 'Example 5', input: { email: 'e@e.com' } }, ], }, }), ]; const tool = createDescribeTool(mockTools); const result = await tool.execute({ toolNames: ['users:create'] }); expect(result.tools[0].usageExamples.length).toBe(5); // All should be user-provided, none smart-generated expect(result.tools[0].usageExamples[4].description).toBe('Example 5'); });libs/plugins/src/codecall/tools/describe.tool.ts (1)
98-119: Consider adding explicit type for ToolEntry parameter instead ofany.The
ToolEntry<any, any>usage on line 98 and 218 could be improved. Per coding guidelines, preferunknownfor generic type parameters without constraints.- private getInputSchema(tool: ToolEntry<any, any>): JsonSchema | null { + private getInputSchema(tool: ToolEntry<unknown, unknown>): JsonSchema | null {Also note: The empty
catchblock at line 108 silently swallows errors. Consider logging at debug level for troubleshooting:} catch { - // Fall through to rawInputSchema + // Conversion failed, fall through to rawInputSchema }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.md(1 hunks)apps/demo/src/apps/crm/tools/users-create.tool.ts(2 hunks)docs/draft/blog/external-links.mdx(1 hunks)docs/draft/docs/guides/vectoriadb.mdx(1 hunks)docs/draft/docs/plugins/codecall/api-reference.mdx(8 hunks)docs/draft/docs/servers/apps.mdx(3 hunks)docs/draft/docs/servers/server.mdx(5 hunks)docs/draft/docs/servers/tools.mdx(2 hunks)libs/plugins/src/codecall/README.md(2 hunks)libs/plugins/src/codecall/__tests__/describe.tool.test.ts(5 hunks)libs/plugins/src/codecall/__tests__/describe.utils.test.ts(1 hunks)libs/plugins/src/codecall/__tests__/tool-search.service.test.ts(1 hunks)libs/plugins/src/codecall/codecall.types.ts(6 hunks)libs/plugins/src/codecall/services/tool-search.service.ts(8 hunks)libs/plugins/src/codecall/tools/describe.schema.ts(2 hunks)libs/plugins/src/codecall/tools/describe.tool.ts(8 hunks)libs/plugins/src/codecall/utils/describe.utils.ts(2 hunks)libs/sdk/src/common/metadata/tool.metadata.ts(5 hunks)libs/sdk/src/common/tokens/tool.tokens.ts(1 hunks)libs/sdk/src/tool/README.md(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/draft/docs/guides/vectoriadb.mdx
- docs/draft/docs/plugins/codecall/api-reference.mdx
🧰 Additional context used
📓 Path-based instructions (8)
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document known limitations clearly and write comprehensive security documentation for validation libraries
Files:
libs/sdk/src/tool/README.mdREADME.mdlibs/plugins/src/codecall/README.md
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/README.mdlibs/sdk/src/common/tokens/tool.tokens.tslibs/plugins/src/codecall/__tests__/describe.utils.test.tslibs/plugins/src/codecall/tools/describe.tool.tslibs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/services/tool-search.service.tslibs/plugins/src/codecall/utils/describe.utils.tslibs/plugins/src/codecall/tools/describe.schema.tslibs/plugins/src/codecall/__tests__/describe.tool.test.tslibs/plugins/src/codecall/codecall.types.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/plugins/src/codecall/README.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/sdk/src/common/tokens/tool.tokens.tslibs/plugins/src/codecall/__tests__/describe.utils.test.tsapps/demo/src/apps/crm/tools/users-create.tool.tslibs/plugins/src/codecall/tools/describe.tool.tslibs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/services/tool-search.service.tslibs/plugins/src/codecall/utils/describe.utils.tslibs/plugins/src/codecall/tools/describe.schema.tslibs/plugins/src/codecall/__tests__/describe.tool.test.tslibs/plugins/src/codecall/codecall.types.tslibs/sdk/src/common/metadata/tool.metadata.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/common/tokens/tool.tokens.tslibs/sdk/src/common/metadata/tool.metadata.ts
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/servers/tools.mdxdocs/draft/docs/servers/apps.mdxdocs/draft/docs/servers/server.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/servers/tools.mdxdocs/draft/docs/servers/apps.mdxdocs/draft/docs/servers/server.mdxdocs/draft/blog/external-links.mdx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use Jest with 95%+ coverage requirement enforced through test configuration
Include constructor validation tests and error class instanceof checks in all test suites
Do not use prefixes like PT-001 in test names - use descriptive test names instead
Test all code paths including error cases and edge conditions
Files:
libs/plugins/src/codecall/__tests__/describe.utils.test.tslibs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/__tests__/describe.tool.test.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/crm/tools/users-create.tool.ts
🧠 Learnings (11)
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.ts : Add JSDoc examples with example tags showing basic usage and options patterns for all components
Applied to files:
libs/sdk/src/common/tokens/tool.tokens.tsdocs/draft/docs/servers/tools.mdxlibs/plugins/src/codecall/tools/describe.tool.tslibs/plugins/src/codecall/__tests__/describe.tool.test.tslibs/sdk/src/common/metadata/tool.metadata.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.tslibs/plugins/src/codecall/__tests__/tool-search.service.test.tslibs/plugins/src/codecall/__tests__/describe.tool.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.tslibs/plugins/src/codecall/__tests__/describe.tool.test.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Include constructor validation tests and error class instanceof checks in all test suites
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and edge conditions
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Do not use prefixes like PT-001 in test names - use descriptive test names instead
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.tslibs/plugins/src/codecall/__tests__/describe.tool.test.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Use Jest with 95%+ coverage requirement enforced through test configuration
Applied to files:
libs/plugins/src/codecall/__tests__/describe.utils.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/plugins/src/codecall/tools/describe.tool.tsdocs/draft/blog/external-links.mdxlibs/plugins/src/codecall/codecall.types.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Use Zod schema validation for all component inputs as a core validation strategy
Applied to files:
docs/draft/blog/external-links.mdx
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Applied to files:
docs/draft/blog/external-links.mdx
🧬 Code graph analysis (5)
libs/plugins/src/codecall/__tests__/describe.utils.test.ts (1)
libs/plugins/src/codecall/utils/describe.utils.ts (9)
detectToolIntent(40-64)generateCreateExample(384-394)generateGetExample(399-420)generateListExample(425-433)generateUpdateExample(438-464)generateDeleteExample(469-489)generateSearchExample(494-511)generateSmartExample(590-617)hasPaginationParams(290-297)
libs/plugins/src/codecall/tools/describe.tool.ts (1)
libs/plugins/src/codecall/utils/describe.utils.ts (1)
generateSmartExample(590-617)
libs/plugins/src/codecall/__tests__/tool-search.service.test.ts (1)
libs/plugins/src/codecall/services/tool-search.service.ts (1)
ToolSearchService(300-749)
libs/plugins/src/codecall/utils/describe.utils.ts (1)
libs/cli/src/templates/3rd-party-integration/src/tools/example.list.ts (1)
inputSchema(12-17)
libs/plugins/src/codecall/codecall.types.ts (1)
libs/plugins/src/codecall/services/synonym-expansion.service.ts (1)
SynonymExpansionConfig(312-331)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (49)
docs/draft/blog/external-links.mdx (1)
33-41: Format and content look good; verify packages exist.The three new Card entries follow the established pattern and align with the broader PR scope on MCP protocol extensions and ecosystem tooling. The descriptions are clear and descriptive.
Please verify that the npm packages exist at the URLs specified:
docs/draft/docs/servers/server.mdx (2)
58-89: Strong structured additions across HTTP, Sessions, and Logging sections. Each field description table now provides context (defaults, use-cases, constraints) that complements the code examples. The progression from minimal → full config → transport controls → best practices flows well.One observation: The
entryPathfield (line 120) mentions it "MUST match.well-knowndiscovery" — consider clarifying whether this refers to the MCP.well-known/mcp.jsonPRM resourcePath (since that's the contract point). This aligns with line 67's note about PRM resourcePath and will help developers debug discovery mismatches.Also applies to: 117-122, 174-178, 204-210
291-309: Excellent addition: the Best Practices section crystallizes real-world constraints. The Do's and Don'ts distill earlier guidance (e.g., "don't use server-level auth with splitByApp" echoes line 101) into a memorable reference. Line 307's note aboutenableConsolein containerized production is especially valuable for ops teams.docs/draft/docs/servers/apps.mdx (6)
7-7: Updated intro clearly establishes Apps' organizational role.The revised description effectively introduces Apps as "organizational units" with examples of contained elements (tools, resources, prompts, providers, adapters, plugins). This sets a strong foundation for the subsequent architectural sections.
9-26: "Why Apps?" section effectively motivates architectural design choices.The comparison table (App vs. Server vs. Tool) quickly clarifies domain boundaries, and the bulleted use cases (feature domains, team boundaries, multi-tenant, third-party integrations) provide concrete rationale. This directly supports the PR's emphasis on App-centric scoping and multi-tenant isolation.
28-46: Minimal App example and server registration flow are clear and actionable.The example is appropriately minimal and the follow-up registration snippet shows immediate next steps. Readers can quickly understand the pattern without unnecessary complexity.
69-95: Expanded field descriptions and subsections provide comprehensive reference.The field descriptions table (lines 71–85) is well-organized and includes key auth/isolation concepts. The "Scoping & auth" and "Dependency resolution" subsections (lines 86–95) directly address the multi-tenant and isolation semantics introduced in the PR. The guidance on provider resolution order and app-scope inheritance is helpful for architecture planning.
125-138: Best Practices section provides actionable Do/Don't guidance.The recommendations are concrete (descriptive IDs, related grouping, isolated auth, adapter usage) and their negative counterparts clarify anti-patterns. This is well-suited for a draft doc and will help users make better architectural decisions.
117-122: The<Info>component is valid and properly used.The
<Info>component is a standard Mintlify component available in this codebase (confirmed bydocs.jsonschema reference tohttps://mintlify.com/docs.json). It is already used consistently throughout the documentation and is correctly applied here within the "Remote Apps (Roadmap)" section.Regarding the suggestion to add context about when and why Remote Apps would be valuable: the current placement under a "Remote Apps (Roadmap)" heading adequately signals this as future functionality. Adding further explanation about use cases is optional and outside the scope of verification.
libs/plugins/src/codecall/codecall.types.ts (5)
5-46: LGTM! Well-structured filter type definitions.The filter function types and schemas are well-designed with clear interfaces, explicit function signatures, and appropriate runtime validation using
z.custom(). The type safety is strict without anyanytypes.
118-121: LGTM! Clean filter schema integration.The filter field correctly uses the dedicated
directCallsFilterSchemawith proper documentation of the expected signature.
291-294: LGTM! Clean filter schema integration.The includeTools field correctly uses the dedicated
includeToolsFilterSchemawith proper documentation of the expected signature.
196-208: LGTM! Appropriate union type for synonym expansion.The union type correctly allows either
falseto disable the feature or a configuration object to customize it. The documentation clearly explains when this applies (TF-IDF strategy only). However, this depends on resolving the schema mismatch issue flagged in lines 126-158.
332-332: Verify.prefault()is the intended behavior.The change from
.default()to.prefault()alters when defaults are applied. In Zod 4:
.default()short-circuits when input isundefinedand returns the default immediately.prefault()applies defaults before parsing (Zod 3 behavior)For plugin options where you want to merge user-provided partial configs with defaults,
.prefault()is typically correct. However, ensure this matches your intended behavior and that consuming code doesn't rely on the new Zod 4.default()semantics.libs/sdk/src/common/tokens/tool.tokens.ts (1)
16-16: LGTM! Clean addition of the examples metadata token.The new
examplestoken follows the established pattern and integrates seamlessly with the existing metadata structure.README.md (2)
304-334: LGTM! Enhanced documentation clarity.The updated Core Concepts descriptions provide better context and align well with the architectural changes in this PR.
637-671: Excellent documentation of the new usageExamples surface.The expanded documentation clearly explains:
- The shift from single
usageExampletousageExamplesarray- Example generation and prioritization rules
- Intent detection patterns for smart example generation
- Indexing weights for semantic search
libs/plugins/src/codecall/services/tool-search.service.ts (9)
16-16: LGTM! Required import for synonym expansion feature.
32-199: Excellent expansion of the stop words list.The comprehensive, categorized stop word list is well-documented and optimized:
- Clear categories with inline comments
- Preserves action verbs critical for tool search
- Uses
ReadonlySetfor O(1) lookup performance- Domain-aware filtering for command-line/chat interfaces
283-292: LGTM! Clear and well-documented configuration option.The
synonymExpansionconfig provides flexible control:
- Can be disabled with
false- Can be customized with config object
- Well-documented JSDoc explains behavior and defaults
- Correctly scoped to TF-IDF strategy only
307-310: LGTM! Correct type refinement for internal config.Properly excludes optional fields from the required internal config type.
313-313: LGTM! Appropriate field for optional synonym service.
317-323: LGTM! Sensible default embedding options.The default synonym expansion configuration is well-balanced:
enabled: true- reasonable default for TF-IDFreplaceDefaults: false- preserves built-in synonymsmaxExpansionsPerTerm: 5- prevents query explosion
357-366: LGTM! Correct conditional initialization of synonym service.The initialization logic properly:
- Respects explicit disabling with
false- Only enables for TF-IDF strategy
- Defaults to enabled when not explicitly disabled
- Extracts config when provided as object
602-621: LGTM! Well-implemented example indexing with proper weighting.The example indexing logic:
- Uses defensive optional chaining
- Applies appropriate 2x weight to descriptions
- Extracts both keys and string values from input
- Handles undefined/invalid examples gracefully
675-680: LGTM! Clean query expansion implementation.The synonym expansion is:
- Well-documented with inline comment and example
- Applied conditionally (only when synonymService exists)
- Used appropriately in the vector search
libs/sdk/src/tool/README.md (1)
38-118: LGTM! Clear documentation of the examples feature.The updated ToolRecord documentation:
- Clearly describes the examples array structure
- Provides a concrete example with two entries
- Shows proper usage of description, input, and output fields
libs/plugins/src/codecall/README.md (1)
637-671: Excellent comprehensive documentation of the describe contract.The updated documentation clearly covers:
- Migration from single
usageExampletousageExamplesarray- Up to 5 examples per tool with prioritization rules
- Example indexing with explicit weighting (2x for descriptions/inputs)
- Intent detection patterns in a clear table format
- User-provided examples taking priority
libs/plugins/src/codecall/__tests__/tool-search.service.test.ts (2)
549-644: LGTM! Comprehensive example indexing test coverage.The tests properly verify:
- Example descriptions are indexed and searchable
- Example input values are indexed
- Graceful handling of undefined examples
- Proper service cleanup
646-799: Excellent comprehensive synonym expansion test coverage.The test suite thoroughly covers:
- Default enabling for TF-IDF strategy
- Multiple synonym matches (add→create, remove→delete, fetch→get)
- Explicit disabling with
synonymExpansion: false- Custom synonym groups via configuration
- ML strategy not using synonym expansion
- Proper service cleanup in all cases
apps/demo/src/apps/crm/tools/users-create.tool.ts (2)
16-16: LGTM! Good addition of field documentation.
9-9: No issues found. The usage ofz.email()is valid and recommended for Zod 4.0.0, which is the version specified in the project's package.json. Zod 4 introducedz.email()as a standalone string format schema and deprecatedz.string().email(). The code is correct and follows best practices.libs/plugins/src/codecall/__tests__/describe.utils.test.ts (1)
1-409: Excellent comprehensive test suite for describe utils.This test file demonstrates best practices:
- Well-organized with logical describe blocks by intent type
- Uses parameterized tests (
it.each) for verb variations- Covers all intent types: create, delete, get, update, list, search, unknown
- Tests both name-based and description-based detection (fallback)
- Tests all generator functions with schema variations
- Tests edge cases (pagination, entity name pluralization, non-namespaced tools)
- Descriptive test names without prefixes (follows coding guidelines)
- Comprehensive coverage of the functionality
libs/sdk/src/common/metadata/tool.metadata.ts (2)
21-39: LGTM! Well-documented interface for tool examples.The
ToolExampleinterface is clearly documented with JSDoc comments explaining each field's purpose. The use ofRecord<string, unknown>forinputandunknownforoutputappropriately handles arbitrary tool inputs/outputs.
209-215: Good addition of the examples field to ToolMetadata.The
examplesfield is well-documented and aligns with theToolExampleinterface. The documentation clearly explains the purpose and precedence behavior.docs/draft/docs/servers/tools.mdx (2)
249-253: Documentation accurately reflects the new examples field.The metadata documentation and field descriptions table are consistent with the
ToolExampleinterface defined in the SDK. The optional nature and field types are correctly documented.Also applies to: 280-280
285-352: Comprehensive documentation for the new Tool Examples feature.The new section effectively demonstrates:
- Both Class Style and Function Style usage patterns
- Multiple example entries with description, input, and optional output
- Discovery behavior (2x indexing weight, max 5 examples)
- Auto-generation fallback when examples aren't provided
The
<Tip>explaining the fallback behavior is helpful for users who don't want to manually specify examples.libs/plugins/src/codecall/__tests__/describe.tool.test.ts (3)
446-452: Good structural validation for usageExamples array.The test properly validates that
usageExamplesis an array with at least one element, and checks the structure of individual example objects (description and code fields).
538-562: Thorough test for max 5 examples limit.This test verifies that even when 7 user-provided examples exist, only the first 5 are returned. The assertions on specific descriptions ensure ordering is preserved.
590-623: Important test for example filling behavior.The test at line 617 expects exactly 4 examples (3 user-provided + 1 smart-generated), which validates the "fill remaining slots" logic. This ensures the system maximizes useful examples without exceeding the limit.
libs/plugins/src/codecall/tools/describe.schema.ts (2)
56-68: Well-structured schema for usageExamples array.The schema correctly:
- Uses
.max(5)to enforce the documented limit- Includes descriptive
.describe()annotations for each field- Clearly documents the expected code format in the code field description
8-8: Breaking change: usageExample → usageExamples. The schema output type has been changed from a singleusageExampleobject to ausageExamplesarray. All consumers ofDescribeToolOutputhave been correctly updated (describe.tool.ts lines 70, 79), and no references to the old singular form remain in the codebase. However, per coding guidelines for publishable SDK libraries, ensure documentation indocs/draft/docs/is updated to reflect this API change.libs/plugins/src/codecall/tools/describe.tool.ts (3)
2-2: Good addition of Zod v4 JSON Schema conversion.The import of
toJSONSchemafromzod/v4aligns with the library context showing Zod version 4.0.0. This ensures property descriptions from.describe()are preserved in the generated JSON Schema.Also applies to: 4-4
217-241: Solid implementation of multi-example generation with proper prioritization.The
generateExamplesmethod correctly:
- Prioritizes user-provided examples (up to 5)
- Fills remaining slots with smart-generated examples
- Ensures at least 1 example is always returned
- Enforces the max 5 limit with
.slice(0, 5)Minor observation: The code formatting in generated examples (line 229-230) uses
JSON.stringify(..., null, 2)which produces multi-line output. This is good for readability.
135-136: Consistent use of Zod v4's toJSONSchema.The
toJsonSchemamethod now consistently usestoJSONSchemafromzod/v4for both direct ZodType instances and raw shapes. This ensures uniform behavior across different schema input types.Also applies to: 151-152
libs/plugins/src/codecall/utils/describe.utils.ts (4)
16-34: LGTM! Well-designed intent classification system.The
ToolIntenttype and pattern mappings are comprehensive and well-structured. The synonym groups align well with the mentioned search service patterns, providing consistent intent detection across the system.
36-64: LGTM! Robust intent detection with appropriate fallbacks.The function correctly prioritizes tool name patterns before falling back to description analysis. The dual-layer approach ensures intent is detected even when naming conventions vary.
366-584: LGTM! Intent-specific generators are well-implemented.The example generators follow consistent patterns and handle edge cases appropriately. The helper functions provide robust parameter detection with sensible fallbacks when schemas don't match expected patterns.
586-617: LGTM! Smart routing with appropriate fallbacks.The dispatcher correctly routes to intent-specific generators and handles the list intent specially by checking for pagination parameters. The fallback to
generateBasicExampleensures robustness.Note: The AI summary mentions structural duplication of these generator functions appearing twice in the diff, but I don't observe this in the provided code. If such duplication exists elsewhere, it should be removed to avoid conflicting declarations.
| // Synonym expansion configuration schema | ||
| export const synonymExpansionConfigSchema = z | ||
| .object({ | ||
| /** | ||
| * Enable/disable synonym expansion for TF-IDF search. | ||
| * When enabled, queries are expanded with synonyms to improve relevance. | ||
| * For example, "add user" will also match tools containing "create user". | ||
| * @default true | ||
| */ | ||
| enabled: z.boolean().default(true), | ||
|
|
||
| /** | ||
| * Additional synonym groups beyond the defaults. | ||
| * Each group is an array of related terms that should be treated as equivalent. | ||
| * @example [['customer', 'client', 'buyer'], ['order', 'purchase', 'transaction']] | ||
| */ | ||
| additionalSynonyms: z.array(z.array(z.string())).optional(), | ||
|
|
||
| /** | ||
| * Replace default synonyms entirely with additionalSynonyms. | ||
| * @default false | ||
| */ | ||
| replaceDefaults: z.boolean().default(false), | ||
|
|
||
| /** | ||
| * Maximum number of synonym expansions per term. | ||
| * Prevents query explosion for terms with many synonyms. | ||
| * @default 5 | ||
| */ | ||
| maxExpansionsPerTerm: z.number().positive().default(5), | ||
| }) | ||
| .default({ enabled: true, replaceDefaults: false, maxExpansionsPerTerm: 5 }); | ||
|
|
There was a problem hiding this comment.
Schema conflicts with existing SynonymExpansionConfig interface.
The synonymExpansionConfigSchema includes an enabled field that doesn't exist in the SynonymExpansionConfig interface defined in libs/plugins/src/codecall/services/synonym-expansion.service.ts (lines 311-330). Additionally, line 342 exports a type with the same name inferred from this schema, creating two conflicting SynonymExpansionConfig definitions in the same package.
Service interface has:
additionalSynonyms?: ReadonlyArray<ReadonlyArray<string>>replaceDefaults?: booleanmaxExpansionsPerTerm?: number
This schema adds:
enabled: z.boolean().default(true)← Not in interface
Either:
- Remove
enabledfrom the schema and keep only the three fields, or - Add
enabledto the service interface, or - Rename one of the types to avoid the collision
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/codex-mintlify-docs.yml (1)
74-106: Clarify misleading comment about MCP query.Lines 74–75 comment "Query Mintlify MCP for documentation best practices," but the code (lines 76–106) writes hardcoded guidelines to a markdown file—it does not query an MCP server. The comment should accurately reflect that these are pre-written guidelines.
Apply this diff to fix the misleading comment:
echo "Setting up Mintlify MCP context..." - # Query Mintlify MCP for documentation best practices - # This provides Codex with official Mintlify guidelines + # Provide hardcoded Mintlify documentation guidelines for Codex context + # (These are embedded best practices, not dynamically fetched from MCP) cat > .github/codex/mcp-context/mintlify-guidelines.md <<'GUIDELINES'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/codex-mintlify-docs.yml(3 hunks)
⏰ 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). (1)
- GitHub Check: build
| uses: openai/codex-action@v1 | ||
| with: | ||
| openai-api-key: ${{ secrets.CODEX_OPENAI_KEY }} | ||
| codex-home: ${{ steps.mcp.outputs.codex_home }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
openai/codex-action v1 parameters documentation
💡 Result:
The openai/codex-action@v1 GitHub Action accepts the following inputs and exposes these outputs:
Inputs
- openai-api-key — API key (secret). [required if using the proxy]. [1]
- responses-api-endpoint — Optional override for Responses API endpoint (e.g. Azure). [1]
- prompt — Inline prompt text (provide this OR prompt-file). [1]
- prompt-file — Path to a file containing the prompt (provide this OR prompt). [1]
- output-file — File path to write the final Codex message (optional). [1]
- working-directory — Directory passed to
codex exec --cd(defaults to repo root). [1] - sandbox — Sandbox mode:
workspace-write(default),read-only, ordanger-full-access. [1] - codex-version — Version of the
@openai/codexpackage to install. [1] - codex-args — Extra arguments forwarded to
codex exec; accepts JSON arrays (e.g. ["--flag","value"]) or shell-style strings. [1] - safety-strategy — Controls runner privileges; valid values include
drop-sudo(default),unprivileged-user,read-only,unsafe. [1] - codex-user — (used with
unprivileged-usersafety-strategy; see examples). [1]
Outputs
- final-message — Final message returned by
codex exec. [1]
Notes
- The action installs the Codex CLI and starts a Responses API proxy; Azure usage requires the full Responses API URL (including /v1/responses). [1]
- For more configuration options and examples (including sandbox and Azure guidance) see the repo README and the Codex docs. [1][2]
Sources
[1] openai/codex-action README (GitHub).
[2] OpenAI Codex developer docs (Codex SDK / action overview).
🏁 Script executed:
# First, let's examine the workflow file to understand the context
find . -name "codex-mintlify-docs.yml" -type fRepository: agentfront/frontmcp
Length of output: 106
🏁 Script executed:
# Let's also search for any action definitions in the repository
find .github -name "action.yml" -o -name "action.yaml"Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Read the workflow file around line 609 to understand the context
sed -n '600,620p' .github/workflows/codex-mintlify-docs.ymlRepository: agentfront/frontmcp
Length of output: 833
Remove codex-home parameter—it is not a valid input for openai/codex-action@v1.
The codex-home parameter passed on line 609 is not recognized by the openai/codex-action@v1 action and will be silently ignored. The official action inputs do not include this parameter. If the intention is to set a working directory for Codex, use the working-directory parameter instead.
🤖 Prompt for AI Agents
In .github/workflows/codex-mintlify-docs.yml around line 609, the step is
passing an invalid input named `codex-home` to openai/codex-action@v1; remove
the `codex-home: ${{ steps.mcp.outputs.codex_home }}` line and, if the goal was
to change the action's working directory, replace it by setting
`working-directory:` on the job/step (or use the action's supported inputs) so
only valid inputs are passed to openai/codex-action@v1.
…tter reference handling
…iterals and template literals
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/testing/src/server/test-server.ts (1)
330-349: ExportingfindAvailablePortmakes it part of the public testing API—ensure this is intentional and discoverableBy adding
exporthere,findAvailablePortbecomes a public utility oflibs/testing. If the intent is to support external callers (beyond this file):
- Confirm this is a stable API you’re comfortable supporting going forward (name, return type, and behavior).
- Re‑export it from the testing barrel (e.g.,
libs/testing/src/index.tsor equivalent) so consumers don’t have to deep‑import this file.- Consider adding/expanding the JSDoc to call out that it’s intended for test usage and relies on Node’s
netserver binding on port0.If the exposure was only meant for local reuse inside this module, you can drop
exportand instead provide higher‑level helpers onTestServer. Based on learnings, …libs/ast-guard/src/transforms/concat.transform.ts (2)
72-110: Concatenation transform logic looks solid; suggest clarifyingwalk.simplesemantics commentThe in-place rewrite from
BinaryExpressiontoCallExpressionlooks correct: capturingleft/rightby reference avoids stale copies, nested+chains will see inner transforms first (post‑order), and you carefully preserve location metadata while only deleting operator-specific fields. This should play nicely with downstream tooling and source maps.The only thing I’d tweak is the comment at lines 72–75. In
acorn-walk@8.x,simplecalls the base walker before user visitors, so children are traversed using the original shape (left/right) and your visitor runs afterward; it does not subsequently traverse children via the newargumentsarray in this pass. (gist.github.com) The current wording could mislead future refactors into relying on traversal overarguments.A small comment-only diff to better reflect what actually happens:
- // Use simple walker. Because we mutate the node type to 'CallExpression' - // and move children to 'arguments', the walker will naturally - // continue traversing the children in their new location. + // Use simple walker. acorn-walk.simple runs our visitor *after* traversing + // the original BinaryExpression children, so nested `+` expressions are + // transformed bottom‑up. After this callback, other passes will see this + // node as a CallExpression with its original children moved into `arguments`.(Optionally, if you ever want stricter typing, you could introduce minimal structural interfaces for the
BinaryExpression/CallExpressionshapes instead ofany, but that’s non‑blocking given acorn-walk’s own typings.)
154-176: Template literal transform is correct and reference-safe; minor clarifications onlyThe TemplateLiteral →
__safe_template__call rewrite looks good: you build a syntheticArrayExpressionfor quasis, keepexpressionsby reference (so earlier transforms on the inner expressions are preserved), and then move them intoargumentswhile preserving node location data by only deletingquasis/expressions. This matches the BinaryExpression strategy and should work well withacorn-walk.simple’s post‑order behavior.If you want to tighten things further (optional):
- Consider a brief comment noting that
simplevisits TemplateLiteral children before this visitor runs, so any other expression transforms (like your concatenation pass) are already applied before the template is collapsed into a CallExpression. That documents the ordering assumption you’re relying on.- As above, you could later replace
node: anywith a small structural TemplateLiteral/CallExpression type alias once you have time, but it’s not required.libs/ast-guard/src/transforms/string-extraction.transform.ts (2)
78-110: New context helpers look correct; consider light typing tighten-upThe
isObjectKey/isModuleSourceguards correctly avoid transforming object/class keys and import/export specifiers, which should significantly reduce accidental semantic changes during extraction.One small follow‑up you might consider (non‑blocking):
- If
acorn/acorn-walktypings allow it, narrownode/parentfromanyto something likeacorn.Node | nullin these helpers to better align with the strict TS guidance inlibs/**without changing runtime behavior.Given the constraints of the external typings, treating this as a best‑effort improvement rather than a requirement seems reasonable.
153-206: TemplateLiteral transform is safer now; optional symmetry with Literal guardsThe TemplateLiteral handler improvements look good:
- Restricting to static templates (no expressions, single quasi) keeps behavior aligned with “string-like” literals.
- The TaggedTemplateExpression guard avoids turning
tag\...`into invalidtag"ref"` code, fixing the previous syntax hazard.- Deleting only
quasisandexpressionswhile preservingloc/start/endis a nice touch for source-map stability before retagging the node as aLiteral.If you want to push this a bit further (optional):
- For consistency with the Literal path, you could also reuse
isObjectKey/isModuleSourcehere (based on the computedparent) so that purely static TemplateLiterals used in keys or module specifiers are treated the same way as string Literals.This would keep the mental model of “never rewrite structural strings” uniform across both literal forms.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/ast-guard/src/transforms/concat.transform.ts(2 hunks)libs/ast-guard/src/transforms/string-extraction.transform.ts(5 hunks)libs/testing/src/client/mcp-test-client.ts(1 hunks)libs/testing/src/server/test-server.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/testing/src/client/mcp-test-client.tslibs/testing/src/server/test-server.tslibs/ast-guard/src/transforms/concat.transform.tslibs/ast-guard/src/transforms/string-extraction.transform.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/testing/src/client/mcp-test-client.tslibs/testing/src/server/test-server.tslibs/ast-guard/src/transforms/concat.transform.tslibs/ast-guard/src/transforms/string-extraction.transform.ts
libs/ast-guard/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Files:
libs/ast-guard/src/transforms/concat.transform.tslibs/ast-guard/src/transforms/string-extraction.transform.ts
🧠 Learnings (5)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methods
Applied to files:
libs/testing/src/client/mcp-test-client.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/testing/src/client/mcp-test-client.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/testing/src/server/test-server.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/testing/src/server/test-server.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/ast-guard/src/transforms/string-extraction.transform.ts
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (4)
libs/testing/src/client/mcp-test-client.ts (2)
751-759: Clarification needed on roots capability change claim.The initialize() method currently only includes
sampling: {}in capabilities—there is no evidence that arootscapability was ever present in this test client's initialization payload. The codebase shows roots management is correctly handled through the notification-based approach (viaroots-list-changed-notification.handler), but this appears to be the intended architecture rather than a change from a previous state.Since
mcp-test-clientis itself a test utility library (not application code with dedicated test suites), requesting test updates may be unnecessary. The implementation is correct and follows the MCP protocol pattern for roots management through notifications.Likely an incorrect or invalid review comment.
52-52: Verify protocol version '2025-06-18' is supported by @modelcontextprotocol/sdk 1.23.0.Ensure that the protocol version update from '2024-11-05' to '2025-06-18' is compatible with the current SDK version in use. Check the SDK's official documentation or release notes for supported protocol versions.
libs/ast-guard/src/transforms/string-extraction.transform.ts (2)
66-71: Doc example stays aligned with behaviorThe updated threshold/onExtract example matches the actual API and behavior of
extractLargeStrings; no further changes needed here.
111-132: Ancestor-based Literal handling + safety guards look soundUsing
walk.ancestorand derivingparentfromancestors[ancestors.length - 2]is a solid way to get context, and the early returns forisObjectKeyandisModuleSourcecorrectly prevent extraction in structurally significant positions before any mutation happens.The threshold and accounting logic below remains unchanged and consistent with
shouldExtract, so this change should be a pure correctness win (fewer unsafe rewrites) without altering extraction counts for “normal” string literals.
…bling standard globals
…ifiers and refining session ID generation logic
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/plugins/src/codecall/codecall.types.ts (1)
126-158: FixSynonymExpansionConfigname clash and single‑source synonym-expansion defaults.This file now exports
SynonymExpansionConfig(line 342) inferred fromsynonymExpansionConfigSchema, butlibs/plugins/src/codecall/services/synonym-expansion.service.tsalready declares aninterface SynonymExpansionConfig(lines 311‑330 in that file). A type alias and an interface with the same top-level name will produce a duplicate identifier error in TypeScript rather than merging, so this is currently a compile-time blocker.In addition, the schema here introduces an
enabledflag, while the service interface only hasadditionalSynonyms,replaceDefaults, andmaxExpansionsPerTerm. Even once the name clash is resolved, having two differently-shapedSynonymExpansionConfigtypes is likely to cause subtle typing and runtime mismatches when wiring embedding options into the synonym-expansion service.I’d recommend either:
- Renaming this inferred export to something like
SynonymExpansionOptionsand keeping the serviceSynonymExpansionConfiginterface as the canonical config, or- Moving the canonical config type definition here (including
enabled) and reusing it in the service, so the schema, TS type, and service expectations all stay aligned.Separately, the default synonym-expansion config is now encoded three times: as the object schema
.default(...)(lines 126‑157), inDEFAULT_EMBEDDING_OPTIONS.synonymExpansion(line 165), and as thesynonymExpansionfield default incodeCallEmbeddingOptionsSchema(lines 204‑207). To avoid drift when defaults change, consider introducing a singleDEFAULT_SYNONYM_EXPANSIONconstant and referencing it from all three places.Finally, turning synonym expansion on by default for TF‑IDF search changes behavior and may affect performance/recall. Please double‑check that this is the desired default for the SDK and ensure it’s clearly documented (including how to set
synonymExpansion: falseto disable it).#!/bin/bash # Verify all SynonymExpansionConfig definitions and usages under libs/plugins rg -n "SynonymExpansionConfig" libs/plugins/srcAlso applies to: 165-166, 197-209, 342-347
🧹 Nitpick comments (10)
libs/ast-guard/src/presets/secure.preset.ts (1)
76-87: Confirm impact and hierarchy for newly disallowed identifiers.Tightening
disallowedIdentifierswith:
constructor,arguments,thisRegExp,SymbolWeakMap,WeakSetmeaningfully raises the bar for the SECURE preset, but also changes behavior for existing callers:
- Any code using classes or object
constructoraccess may now be rejected under SECURE.- Legitimate uses of
RegExp,Symbol, or weak collections will also be blocked.- If STRICT doesn’t already include these, SECURE might become stricter than STRICT, which would violate the intended preset hierarchy.
Two things worth double‑checking:
Preset hierarchy & compatibility
- Ensure STRICT ≥ SECURE ≥ STANDARD ≥ PERMISSIVE still holds after this change.
- If this is a breaking tightening for existing SDK users, consider how it aligns with your versioning strategy and whether docs/release notes should explicitly call it out.
Rule semantics for special tokens
- In many ASTs,
thisis represented as aThisExpression, not a normal identifier; please confirm thatDisallowedIdentifierRuleactually flagsthisas intended and that this entry isn’t a no‑op.If the goal is to harden SECURE without surprising existing consumers, you may want to (a) mirror these additions into STRICT as well, and/or (b) explicitly document that SECURE now forbids these constructs so users can choose STANDARD/PERMISSIVE when they need them. As per coding guidelines and retrieved learnings, this helps keep the STRICT > SECURE > STANDARD > PERMISSIVE hierarchy clear and predictable.
Also applies to: 91-97, 126-129
libs/ast-guard/src/presets/agentscript.preset.ts (2)
210-219: StricterUnknownGlobalRuleis good, but double‑check intended compatibility surfaceFlipping
allowStandardGlobalstofalsecloses the hole where unlisted standard globals (e.g.RegExp,Promise,Symbol, etc.) could bypass the explicit whitelist, which is a solid hardening move and matches the “whitelist‑only” comments.However, this also means all standard globals not in
allowedGlobals(e.g.console,parseInt,encodeURI, etc.) will now be rejected in AgentScript. For existing consumers, that is a behavioural breaking change for this preset.Please confirm that:
- AgentScript is intended to be in the “STRICT” end of your preset hierarchy and not a drop‑in for more permissive presets, and
- Any expected/allowed globals beyond the current list are either added here or clearly documented (and, if needed, called out in release notes / docs under
docs/draft/docs/**).If you’d like, we can sketch a small “STANDARD AgentScript” variant or a compatibility flag that keeps
allowStandardGlobals: truefor legacy callers.Based on learnings, this aligns AgentScript with the stricter side of the STRICT > SECURE > STANDARD > PERMISSIVE hierarchy, but it’s worth validating the intended level.
239-335: ExpandeddangerousIdentifierslist looks good; just be aware it overridesallowedGlobalscustomizationsThe additions of
arguments,RegExp,Promise,Symbol,Map, andSetall make sense from a safety perspective (scope leakage, ReDoS, async flooding, prototype/memory hazards).Because they live in
dangerousIdentifiersenforced viaDisallowedIdentifierRule, they will stay blocked even if a consumer tries to add them tooptions.allowedGlobals. That’s likely the right default for AgentScript, but it does mean:
- AgentScript consumers cannot selectively opt into these primitives just by customizing
allowedGlobals.- Any scenario where you do want e.g.
Map/SetorPromisewill require a different preset or additional configuration surface.If that’s intentional for this preset, nothing to change; otherwise, consider either:
- Documenting these as “always banned” in AgentScript, or
- Introducing a separate, slightly less strict preset where some of these can be toggled.
libs/ast-guard/src/presets/permissive.preset.ts (2)
15-18: Align preset documentation with actual blocked identifiersThe top‑level JSDoc is slightly out of sync with the behavior:
- “System globals (process, require, module)” omits
exports, which is now also blocked by the baseline list.- The comment in the baseline section refers to sandbox escapes via
constructor, butconstructoris explicitly not blocked in this preset (by design, per the later comment).To avoid surprises for consumers of this published
libspreset, consider tightening the wording to exactly match the identifiers we actually block (e.g., mentionexports, and describe “Function and related constructors” instead of “Function, constructor”).Also applies to: 37-37
44-78: Baseline disallowed identifiers: behavior change and hierarchy considerationsThe baseline +
DisallowedIdentifierRulewiring looks technically sound and is a meaningful hardening step, but it changes behavior of the PERMISSIVE preset:
- Scripts that previously used
Function,process,require,module,exports, or__proto__undercreatePermissivePreset()will now start failing validation.- Because this is a published
libs/ast-guardpreset, this is effectively a behavioral breaking change for existing consumers of the “permissive” profile.Two follow‑ups to consider:
Hierarchy check (STRICT > SECURE > STANDARD > PERMISSIVE):
Ensure the stricter presets all at least block this same baseline (or more). Otherwise PERMISSIVE might accidentally become stricter than STANDARD/SECURE on some identifiers. Based on learnings, the presets should form a clean monotonic hierarchy.Consumer impact / API contract:
- Confirm whether this is acceptable as a tightening of the “permissive” contract; if not, you might gate this behind a new option (e.g.,
baselineSecurity?: booleandefaulting totrue) or document it prominently as a hardening change with an appropriate version bump.- If the intent is “always‑on minimal sandbox safety,” add a brief note in higher‑level docs (not just this file) that PERMISSIVE still disallows these primitives even when no
additionalDisallowedIdentifiersare supplied.If you’d like, I can sketch a small compatibility option pattern that keeps this baseline by default but allows legacy behavior for callers that explicitly opt out.
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (2)
26-30: createTransport wiring and logging correctly handle stateless sessions; consider centralizing loggingUsing
resolveSessionIdGenerator(this.key.type, sessionId)ensures stateless HTTP no longer passes a generator intoStreamableHTTPServerTransport, and theonsessioninitializedguard avoids slicing an undefined sessionId while still distinguishing stateless sessions in logs. If you have a shared logging facility elsewhere in the SDK, consider routing theseconsole.logcalls through it for consistency and easier log management.Also applies to: 34-38
68-72: Remove unnecessaryas anycast by usingZodTypeAnyconstraintThe
toJSONSchemafunction from Zod v4 expectsT extends ZodTypeAny, but the method currently usesZodTypeand casts toany. Replace the import and constraint withZodTypeAnyto eliminate the unsafe cast:-import { ZodType } from 'zod'; +import { ZodTypeAny } from 'zod'; - async sendElicitRequest<T extends ZodType>( + async sendElicitRequest<T extends ZodTypeAny>( relatedRequestId: RequestId, message: string, requestedSchema: T, ): Promise<TypedElicitResult<T>> { await this.transport.send( rpcRequest(this.newRequestId, 'elicitation/create', { message, - requestedSchema: toJSONSchema(requestedSchema as any), + requestedSchema: toJSONSchema(requestedSchema), }),Also applies to
libs/sdk/src/transport/adapters/transport.sse.adapter.tsline 76.libs/plugins/src/codecall/codecall.types.ts (3)
5-46: Filter function types and basic validation look good; consider stricter runtime checking.The
DirectCallsFilterToolInfo/IncludeToolsFilterToolInfointerfaces and theirDirectCallsFilterFn/IncludeToolsFilterFnsignatures are clear and readable, and thez.custom(... typeof val === 'function')checks ensure callers at least pass a function. If you want stronger guarantees, consider modeling these asz.function().args(...).returns(z.boolean())so the argument/return shape is validated at runtime as well, instead of only checkingtypeof === 'function'.
319-332: Using.prefault(DEFAULT_PLUGIN_OPTIONS)for plugin defaults is appropriate; guard against drift.
codeCallPluginOptionsObjectSchema.prefault(DEFAULT_PLUGIN_OPTIONS)is a good fit for Zod 4’s pre‑parse defaulting and aligns with the hand-maintainedDEFAULT_PLUGIN_OPTIONSobject. Because defaults now live both in the schema (e.g.,.default(8)fortopK, defaults insidecodeCallEmbeddingOptionsSchema) and inDEFAULT_PLUGIN_OPTIONS, it’s easy for them to diverge as fields evolve. I’d recommend adding a small test asserting thatcodeCallPluginOptionsSchema.parse(undefined)deep‑equalsDEFAULT_PLUGIN_OPTIONSso any mismatch is caught early.
336-347: New embedding and synonym-expansion types form a clear public surface; update SDK docs accordingly.Exporting
CodeCallEmbeddingOptions,CodeCallEmbeddingOptionsInput, and the synonym-expansion config type (once the naming clash is resolved) cleanly exposes the runtime schemas to SDK consumers. Since this lives underlibs/plugins, these are effectively public API surfaces; please ensure there’s a corresponding update underdocs/draft/docs/**describing the newsynonymExpansionbehavior, its defaults, and the new filter hooks so downstream users know how to configure or disable them.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ast-guard/src/presets/agentscript.preset.ts(5 hunks)libs/ast-guard/src/presets/permissive.preset.ts(2 hunks)libs/ast-guard/src/presets/secure.preset.ts(4 hunks)libs/plugins/src/codecall/codecall.types.ts(6 hunks)libs/sdk/src/transport/adapters/__tests__/transport.streamable-http.adapter.test.ts(1 hunks)libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/sdk/src/transport/adapters/tests/transport.streamable-http.adapter.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/ast-guard/src/presets/permissive.preset.tslibs/ast-guard/src/presets/secure.preset.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/plugins/src/codecall/codecall.types.tslibs/ast-guard/src/presets/agentscript.preset.ts
libs/ast-guard/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Files:
libs/ast-guard/src/presets/permissive.preset.tslibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/presets/agentscript.preset.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/ast-guard/src/presets/permissive.preset.tslibs/ast-guard/src/presets/secure.preset.tslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/plugins/src/codecall/codecall.types.tslibs/ast-guard/src/presets/agentscript.preset.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts
🧠 Learnings (3)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/ast-guard/src/presets/permissive.preset.tslibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/presets/agentscript.preset.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Build must complete without TypeScript warnings using strict mode
Applied to files:
libs/ast-guard/src/presets/permissive.preset.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/plugins/src/codecall/codecall.types.ts
🧬 Code graph analysis (3)
libs/ast-guard/src/presets/permissive.preset.ts (6)
libs/ast-guard/src/index.ts (5)
createPermissivePreset(91-91)PresetOptions(95-95)ValidationRule(19-19)NoEvalRule(52-52)DisallowedIdentifierRule(47-47)libs/ast-guard/src/presets/index.ts (2)
createPermissivePreset(20-20)PresetOptions(14-14)libs/ast-guard/src/presets/types.ts (1)
PresetOptions(6-50)libs/ast-guard/src/interfaces.ts (1)
ValidationRule(62-77)libs/ast-guard/src/rules/no-eval.rule.ts (1)
NoEvalRule(13-89)libs/ast-guard/src/rules/disallowed-identifier.rule.ts (1)
DisallowedIdentifierRule(25-91)
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
libs/sdk/src/transport/transport.types.ts (1)
TransportType(7-7)
libs/plugins/src/codecall/codecall.types.ts (1)
libs/plugins/src/codecall/services/synonym-expansion.service.ts (1)
SynonymExpansionConfig(312-331)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (7)
libs/ast-guard/src/presets/secure.preset.ts (2)
15-27: Doc update correctly reflects constructor-based eval exploits.The added note about “constructor exploits” matches the new
constructorentry in the disallowed identifiers and keeps the preset description accurate.
31-46: Examples align withPresetOptionsusage.The added examples for
requiredFunctions/minFunctionCalls/maxFunctionCallsandallowedLoops: { allowWhile: true }accurately reflect how the options are wired into the rules and improve discoverability.libs/ast-guard/src/presets/agentscript.preset.ts (1)
113-140: AgentScript example and security‑model docs are consistent with the rulesThe example and security‑model commentary only use constructs that are actually allowed by the preset (for/of,
callTool, array methods, default loop/arrow‑function settings), and accurately describe the static‑validation → transformation → runtime pipeline. No changes needed here.Also applies to: 142-159, 171-175
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (2)
2-7: Import changes correctly align with new transport typing and Zod v4 JSON schema usageBringing in
TransportType,ZodType, andtoJSONSchemamatches how they’re used below in the helper andsendElicitRequest; the imports look consistent and type-safe.Please ensure
zod/zod/v4versions inpackage.jsonmatch the expected API surface forZodTypeandtoJSONSchema, and let CI/type-check validate this wiring.
11-22: Session ID generator helper cleanly encodes stateless vs stateful behaviorEncapsulating the “no generator for
stateless-http” rule inresolveSessionIdGeneratormakes the stateful/stateless distinction explicit and reusable, and the TransportType check is straightforward. Since this function is exported from the SDK, please also confirm there’s a brief docs/draft entry (or existing docs update) describing when callers should use it and what they should expect for eachTransportType.libs/plugins/src/codecall/codecall.types.ts (2)
118-122: Direct-callsfilterfield wiring is consistent with the shared schema.Using
directCallsFilterSchema.optional()for thefilterfield keeps the direct-calls options aligned with the dedicated filter type and centralizes validation; this looks correct and non-breaking.
291-295:includeToolsfilter wiring mirrors direct-calls filter and looks correct.Binding
includeToolstoincludeToolsFilterSchema.optional()matches the pattern used fordirectCalls.filterand keeps the plugin options surface consistent for both filter hooks; no issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/auth/demo-transparent-auth/e2e/transparent-auth.e2e.test.ts (2)
40-57: Replace non-null assertion with proper null checking.As per coding guidelines, avoid non-null assertions (!) and use proper error handling instead. If
beforeAllfails to initialize the server, this test would fail with a confusing runtime error.Apply this diff to add proper null checking:
describe('Unauthorized Access', () => { it('should return 401 for unauthorized requests', async () => { + if (!server) { + throw new Error('Test server not initialized'); + } - const response = await fetch(`${server!.info.baseUrl}/`, { + const response = await fetch(`${server.info.baseUrl}/`, { method: 'POST',
61-76: Replace non-null assertion with proper null checking.As per coding guidelines, avoid non-null assertions (!) and use proper error handling instead.
Apply this diff to add proper null checking:
describe('Protected Resource Metadata', () => { it('should expose protected resource metadata endpoint', async () => { + if (!server) { + throw new Error('Test server not initialized'); + } - const response = await fetch(`${server!.info.baseUrl}/.well-known/oauth-protected-resource`, { + const response = await fetch(`${server.info.baseUrl}/.well-known/oauth-protected-resource`, { method: 'GET',
🧹 Nitpick comments (3)
apps/auth/demo-orchestrated-auth/jest.e2e.config.ts (1)
8-8: Sequentializing E2E tests withmaxWorkers: 1Setting
maxWorkers: 1is a reasonable way to stabilize E2E runs that share state or infra, at the cost of slower execution. If runtime becomes an issue later, consider making this configurable (e.g., via an env var or Jest CLI flag) instead of hard-coding the value.apps/auth/demo-public/jest.e2e.config.ts (1)
8-8: Confirm single-worker E2E execution is desired in all environmentsAdding
maxWorkers: 1will serialize all E2E tests and can noticeably slow the suite; if this is mainly to address CI flakiness (e.g., shared ports/SSE server), consider gating it via an environment flag or adding a short comment explaining the rationale so future maintainers don’t “optimize” it away unintentionally.libs/ast-guard/src/presets/agentscript.preset.ts (1)
324-327: Consider whether blockingMapandSetis too restrictive.The "Memory Hazards" comment doesn't clearly differentiate these from
Array, which can also cause unbounded memory growth.MapandSetare commonly needed for:
- Deduplication:
[...new Set(items)]- Key-value lookups without string coercion
- Counting occurrences
If this is an intentional v1 restriction (like function declarations), consider adding a comment like
// v1 restriction - may relax in v2. Otherwise, these may be overly restrictive for practical agent code.- // Memory Hazards + // Memory Hazards (v1 restriction - revisit if agents need deduplication/lookup patterns) 'Map', 'Set',
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
apps/auth/demo-orchestrated-auth/e2e/orchestrated-auth.e2e.test.ts(1 hunks)apps/auth/demo-orchestrated-auth/jest.e2e.config.ts(1 hunks)apps/auth/demo-public/e2e/public-auth.e2e.test.ts(1 hunks)apps/auth/demo-public/jest.e2e.config.ts(1 hunks)apps/auth/demo-transparent-auth/e2e/transparent-auth.e2e.test.ts(2 hunks)apps/auth/demo-transparent-auth/jest.e2e.config.ts(2 hunks)apps/auth/demo-transparent-auth/src/main.ts(1 hunks)apps/demo-showcase/e2e/openapi.e2e.ts(0 hunks)apps/demo-showcase/e2e/prompts.e2e.ts(0 hunks)apps/demo-showcase/e2e/resources.e2e.ts(0 hunks)apps/demo-showcase/e2e/tools.e2e.ts(0 hunks)apps/demo-showcase/eslint.config.mjs(0 hunks)apps/demo-showcase/jest.config.ts(0 hunks)apps/demo-showcase/project.json(0 hunks)apps/demo-showcase/src/apps/notes/data/store.ts(0 hunks)apps/demo-showcase/src/apps/notes/index.ts(0 hunks)apps/demo-showcase/src/apps/notes/prompts/create-note-draft.prompt.ts(0 hunks)apps/demo-showcase/src/apps/notes/prompts/index.ts(0 hunks)apps/demo-showcase/src/apps/notes/prompts/summarize-notes.prompt.ts(0 hunks)apps/demo-showcase/src/apps/notes/resources/index.ts(0 hunks)apps/demo-showcase/src/apps/notes/resources/note-by-id.resource.ts(0 hunks)apps/demo-showcase/src/apps/notes/resources/notes-list.resource.ts(0 hunks)apps/demo-showcase/src/apps/notes/tools/create-note.tool.ts(0 hunks)apps/demo-showcase/src/apps/notes/tools/delete-note.tool.ts(0 hunks)apps/demo-showcase/src/apps/notes/tools/get-note.tool.ts(0 hunks)apps/demo-showcase/src/apps/notes/tools/index.ts(0 hunks)apps/demo-showcase/src/apps/notes/tools/list-notes.tool.ts(0 hunks)apps/demo-showcase/src/main.ts(0 hunks)apps/demo-showcase/tsconfig.app.json(0 hunks)apps/demo-showcase/tsconfig.json(0 hunks)apps/demo-showcase/tsconfig.spec.json(0 hunks)apps/demo-showcase/webpack.config.js(0 hunks)libs/ast-guard/src/presets/agentscript.preset.ts(7 hunks)
💤 Files with no reviewable changes (25)
- apps/demo-showcase/src/apps/notes/tools/list-notes.tool.ts
- apps/demo-showcase/src/apps/notes/prompts/index.ts
- apps/demo-showcase/e2e/openapi.e2e.ts
- apps/demo-showcase/src/apps/notes/tools/create-note.tool.ts
- apps/demo-showcase/src/apps/notes/tools/delete-note.tool.ts
- apps/demo-showcase/src/apps/notes/resources/notes-list.resource.ts
- apps/demo-showcase/webpack.config.js
- apps/demo-showcase/e2e/prompts.e2e.ts
- apps/demo-showcase/tsconfig.spec.json
- apps/demo-showcase/src/apps/notes/data/store.ts
- apps/demo-showcase/src/apps/notes/resources/index.ts
- apps/demo-showcase/src/apps/notes/resources/note-by-id.resource.ts
- apps/demo-showcase/tsconfig.json
- apps/demo-showcase/src/apps/notes/tools/get-note.tool.ts
- apps/demo-showcase/src/apps/notes/prompts/summarize-notes.prompt.ts
- apps/demo-showcase/tsconfig.app.json
- apps/demo-showcase/jest.config.ts
- apps/demo-showcase/src/apps/notes/prompts/create-note-draft.prompt.ts
- apps/demo-showcase/eslint.config.mjs
- apps/demo-showcase/e2e/tools.e2e.ts
- apps/demo-showcase/e2e/resources.e2e.ts
- apps/demo-showcase/src/apps/notes/index.ts
- apps/demo-showcase/src/main.ts
- apps/demo-showcase/src/apps/notes/tools/index.ts
- apps/demo-showcase/project.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
apps/auth/demo-public/jest.e2e.config.tsapps/auth/demo-public/e2e/public-auth.e2e.test.tsapps/auth/demo-orchestrated-auth/e2e/orchestrated-auth.e2e.test.tsapps/auth/demo-transparent-auth/src/main.tsapps/auth/demo-transparent-auth/e2e/transparent-auth.e2e.test.tslibs/ast-guard/src/presets/agentscript.preset.tsapps/auth/demo-orchestrated-auth/jest.e2e.config.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use Jest with 95%+ coverage requirement enforced through test configuration
Include constructor validation tests and error class instanceof checks in all test suites
Do not use prefixes like PT-001 in test names - use descriptive test names instead
Test all code paths including error cases and edge conditions
Files:
apps/auth/demo-public/e2e/public-auth.e2e.test.tsapps/auth/demo-orchestrated-auth/e2e/orchestrated-auth.e2e.test.tsapps/auth/demo-transparent-auth/e2e/transparent-auth.e2e.test.ts
libs/ast-guard/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Files:
libs/ast-guard/src/presets/agentscript.preset.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/ast-guard/src/presets/agentscript.preset.ts
🧠 Learnings (7)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Use Jest with 95%+ coverage requirement enforced through test configuration
Applied to files:
apps/auth/demo-public/jest.e2e.config.tsapps/auth/demo-orchestrated-auth/jest.e2e.config.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
apps/auth/demo-public/jest.e2e.config.tsapps/auth/demo-public/e2e/public-auth.e2e.test.tsapps/auth/demo-orchestrated-auth/e2e/orchestrated-auth.e2e.test.tsapps/auth/demo-transparent-auth/e2e/transparent-auth.e2e.test.tsapps/auth/demo-orchestrated-auth/jest.e2e.config.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Include constructor validation tests and error class instanceof checks in all test suites
Applied to files:
apps/auth/demo-public/jest.e2e.config.tsapps/auth/demo-orchestrated-auth/jest.e2e.config.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and edge conditions
Applied to files:
apps/auth/demo-public/jest.e2e.config.tsapps/auth/demo-orchestrated-auth/jest.e2e.config.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Do not use prefixes like PT-001 in test names - use descriptive test names instead
Applied to files:
apps/auth/demo-public/jest.e2e.config.tsapps/auth/demo-public/e2e/public-auth.e2e.test.tsapps/auth/demo-orchestrated-auth/e2e/orchestrated-auth.e2e.test.tsapps/auth/demo-transparent-auth/e2e/transparent-auth.e2e.test.tsapps/auth/demo-orchestrated-auth/jest.e2e.config.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/ast-guard/src/presets/agentscript.preset.tsapps/auth/demo-transparent-auth/jest.e2e.config.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
apps/auth/demo-transparent-auth/jest.e2e.config.ts
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (9)
apps/auth/demo-transparent-auth/src/main.ts (1)
16-19: Confirm new default IdP URL and audience match your demo tenant configurationBoth
remote.providerandexpectedAudiencenow default tohttps://sample-app.frontegg.com. For transparent auth to work smoothly in the demo, please confirm that:
- The remote IdP actually serves OIDC metadata/tokens under this base URL.
- The access tokens’
aud(or equivalent) claim matcheshttps://sample-app.frontegg.comwhenIDP_EXPECTED_AUDIENCEis not set.If that’s the case, the change looks good and keeps the defaults consistent; otherwise, you may want a more specific audience (e.g., an API identifier) or a short comment explaining the expected
audformat.apps/auth/demo-public/e2e/public-auth.e2e.test.ts (1)
83-97: Prompt Access E2E coverage in public mode looks correctEnabling this suite with checks for listing and retrieving prompts anonymously matches the intended public-mode behavior, and the test names are clear and guideline-compliant (no ID prefixes). As long as
summarize-notesandprioritize-tasksremain the canonical prompt IDs, this is a solid, focused coverage point.apps/auth/demo-orchestrated-auth/e2e/orchestrated-auth.e2e.test.ts (1)
141-173: Prompt Access tests for orchestrated auth are well-scoped and consistentRe-enabling this suite with authenticated clients and
['read']scope correctly exercises prompts listing and retrieval in orchestrated mode and keeps expectations aligned with the public-mode tests (same prompt IDs and result shape checks).apps/auth/demo-transparent-auth/jest.e2e.config.ts (2)
3-9: Configuration updates are correct and verified.All referenced paths resolve correctly:
jest.preset.jsexists at root,libs/testing/src/setup.tsis accessible, and the testMatch patterne2e/**/*.e2e.test.tssuccessfully matches thetransparent-auth.e2e.test.tstest file. The path adjustments from../../to../../../appropriately reflect the three-level nesting fromapps/auth/demo-transparent-auth/. The configuration changes are appropriate:
displayNamematches the directory nametestTimeout: 60000is reasonable for E2E auth flowsmaxWorkers: 1prevents race conditions in E2E testssetupFilesAfterEnvreferences the shared testing setup
40-46: Module mapper paths are correctly configured.All
moduleNameMapperentries use the proper../../../libs/relative paths for the directory structure. The required library directories andindex.tsfiles exist:
libs/testing/src/index.tsandlibs/testing/src/setup.ts✓libs/sdk/src/index.tswith subdirectories (auth, common, flows, etc.) ✓libs/adapters/src/index.ts✓libs/plugins/src/index.ts✓The
@frontmcp/sdk/(.*)$mapping correctly enables subpath imports from the SDK's internal modules.libs/ast-guard/src/presets/agentscript.preset.ts (4)
22-27: Documentation accurately reflects implementation changes.The updated default list in the JSDoc comment correctly includes the newly added safe standard globals.
191-196: Good security design: explicitly allowing immutable standard globals.Adding
undefined,NaN, andInfinityto the explicit allowlist is the correct approach. These are immutable primitive values that pose no security risk, and explicitly listing them (rather than relying onallowStandardGlobals: true) provides better auditability of what's permitted in the sandbox.
261-272: Well-justified security hardening additions.Each new blocked identifier addresses a specific attack vector:
arguments: Prevents scope leakage and caller information accessRegExp: ComplementsNoRegexLiteralRuleby blockingnew RegExp()constructor (ReDoS via dynamic patterns)Promise: Blocks explicit Promise manipulation whileasync/awaitsyntax remains functionalSymbol: Prevents iterator protocol manipulation and prototype poisoning via well-known symbolsThe inline comments clearly document the security rationale.
218-223: The configuration is intentional and correct. The explicit allowlist approach withallowStandardGlobals: falseis the designed security model for AgentScript. TheallowedGlobalslist already includes essential safe standard globals (Array,Object,String,Number,Date,undefined,NaN,Infinity). Globals likeBoolean,parseInt,parseFloat,isNaN, andisFiniteare not included because they are not required for AgentScript usage patterns—neither the primary consumer (libs/enclave/src/enclave.ts) nor any tests rely on these functions. This configuration aligns with the hierarchical preset security pattern and properly prevents access to dangerous globals likeRegExpandPromise.Likely an incorrect or invalid review comment.
| @@ -1,11 +1,12 @@ | |||
| /* eslint-disable */ | |||
There was a problem hiding this comment.
Replace blanket ESLint disable with specific rule exemptions.
The /* eslint-disable */ comment disables all linting rules. If linting issues exist, disable only the specific rules that are problematic rather than all rules.
Apply this approach instead:
-/* eslint-disable */
+/* eslint-disable import/no-default-export */(Adjust the specific rule as needed based on actual violations)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* eslint-disable */ | |
| /* eslint-disable import/no-default-export */ |
🤖 Prompt for AI Agents
In apps/auth/demo-transparent-auth/jest.e2e.config.ts around line 1, the file
currently uses a blanket "/* eslint-disable */" which disables all linting;
replace it by removing that comment and instead add targeted
eslint-disable-next-line or file-level exemptions for only the specific rules
causing violations (identify the actual lint errors by running ESLint, then add
precise rule names, e.g. "/* eslint-disable rule-name */" or inline disables on
the offending lines), keeping comments minimal and scoped to the exact
rules/lines so that other lint rules remain enforced.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/draft/docs/plugins/codecall/api-reference.mdx (1)
54-69: Clarify codecall:search example parameter semantics.The search examples have been updated with specific query strings. Ensure these example queries accurately reflect the search behavior—particularly that
"get user profile information"and"list all invoices"demonstrate realistic natural language usage and expected matching behavior with the embedding strategy described in lines 103-119.Consider adding a brief comment in the examples explaining why the topK values differ (5 vs 10) to help developers understand parameter tuning:
<CodeGroup> ```json Search All Apps { "tool": "codecall:search", "input": { "query": "get user profile information", - "topK": 5 + "topK": 5 // Reasonable default } }</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 81b7b35c77f5d062240de8a10d3352628958278e and 7b835b144b76995c6bd97e1f354ab4b2e553e2e2. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `CLAUDE.md` (1 hunks) * `docs/draft/blog/external-links.mdx` (1 hunks) * `docs/draft/docs/guides/enclave.mdx` (7 hunks) * `docs/draft/docs/plugins/codecall/api-reference.mdx` (9 hunks) * `docs/draft/docs/plugins/codecall/security.mdx` (4 hunks) * `docs/live/docs/plugins/codecall-plugin.mdx` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * docs/draft/docs/guides/enclave.mdx </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (3)</summary> <details> <summary>docs/**</summary> **⚙️ CodeRabbit configuration file** > `docs/**`: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation) > Files: - `docs/live/docs/plugins/codecall-plugin.mdx` - `docs/draft/blog/external-links.mdx` - `docs/draft/docs/plugins/codecall/security.mdx` - `docs/draft/docs/plugins/codecall/api-reference.mdx` </details> <details> <summary>**/*.md</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Document known limitations clearly and write comprehensive security documentation for validation libraries Files: - `CLAUDE.md` </details> <details> <summary>docs/draft/docs/**</summary> **⚙️ CodeRabbit configuration file** > `docs/draft/docs/**`: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/**. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/**. - It is fine to do structural/content feedback here (clarity, examples, etc). > Files: - `docs/draft/docs/plugins/codecall/security.mdx` - `docs/draft/docs/plugins/codecall/api-reference.mdx` </details> </details><details> <summary>🧠 Learnings (5)</summary> <details> <summary>📚 Learning: 2025-11-26T15:23:04.965Z</summary>Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with.strict()mode to reject unknown properties**Applied to files:** - `docs/draft/blog/external-links.mdx` </details> <details> <summary>📚 Learning: 2025-11-26T15:23:04.965Z</summary>Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Use Zod schema validation for all component inputs as a core validation strategy**Applied to files:** - `docs/draft/blog/external-links.mdx` </details> <details> <summary>📚 Learning: 2025-11-26T17:22:58.085Z</summary>Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level**Applied to files:** - `docs/draft/blog/external-links.mdx` - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-11-26T17:22:58.085Z</summary>Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-11-26T15:23:04.965Z</summary>Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable**Applied to files:** - `docs/draft/docs/plugins/codecall/api-reference.mdx` </details> </details> </details> <details> <summary>⏰ 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). (1)</summary> * GitHub Check: build </details> <details> <summary>🔇 Additional comments (9)</summary><blockquote> <details> <summary>docs/draft/docs/plugins/codecall/api-reference.mdx (4)</summary><blockquote> `154-168`: **Verify usageExamples array schema and max limit enforcement.** The documentation shows `usageExamples` as an array with up to 5 entries, prioritized as user-provided first, then smart-generated by intent. Verify this matches the implementation: 1. Confirm the max 5 example limit is enforced in the backend 2. Verify the prioritization logic (user-provided > smart-generated by intent type) in `describe.tool.ts` or related handler 3. Ensure all frontend examples in this doc correctly represent `usageExamples` as an array --- `824-824`: **No action needed on import path.** The import from `enclave-vm` at line 824 is correct and consistent across the codebase. `enclave-vm` is listed as a dependency in `libs/plugins/package.json` (version ^1.0.0), and all other files in the repository that import from this package use the same `enclave-vm` path. There is no `@frontmcp/enclave` package in use or referenced anywhere in the repository, so this is not a breaking change or migration concern. --- `297-349`: **Error response structure is consistent with implementation.** All error status codes (`syntax_error`, `illegal_access`, `runtime_error`, `tool_error`, `timeout`) and their corresponding object structures match the backend implementation in `libs/plugins/src/codecall/tools/execute.tool.ts` and schema definitions in `libs/plugins/src/codecall/tools/execute.schema.ts`. The documented examples accurately reflect the actual error responses: syntax errors include location details, illegal access includes kind/message, runtime errors include source/name/message, tool errors include toolName/message, and timeouts include only a message field. --- `174-246`: **All validation points are correct—no changes needed.** The codecall:describe expanded response example (lines 182–246) is accurate: 1. **User definition reference** (`#/definitions/User`): Valid JSON Schema `$ref` format for internal type references in outputSchema. 2. **Annotation values** (`readOnlyHint: true`): Confirmed in the codebase—`readOnlyHint` is an optional boolean field in the `ToolAnnotations` interface (libs/sdk/src/common/metadata/tool.metadata.ts, line 53), and the handler correctly extracts annotations from tool metadata. 3. **usageExamples format**: Matches the schema—each example contains `description` and `code` fields, with a maximum of 5 examples enforced by both the schema (libs/plugins/src/codecall/tools/describe.schema.ts, line 67) and the handler implementation (libs/plugins/src/codecall/tools/describe.tool.ts, line 240). The documentation example accurately reflects the actual schema and handler behavior. </blockquote></details> <details> <summary>docs/draft/blog/external-links.mdx (2)</summary><blockquote> `33-41`: **Well-formatted additions with consistent styling.** The new external link cards are properly formatted, maintain consistency with existing cards in the file, and integrate seamlessly into the Quick Links documentation. The descriptions are clear and provide helpful context for each package. --- `33-41`: **All three npm package links are broken—the packages do not exist on npm.** Web searches confirm that `enclave-vm`, `ast-guard`, and `vectoriadb` are not published packages on the npm registry. These external links will return 404 errors when users try to access them. Verify whether these are: - Intentional placeholder names for draft documentation - Misspelled package names (corrected versions: check `enclave`, `secureworker`, `shift-validator`, `ast-verify`, `js-x-ray`, or `vectra` as alternatives) - Packages scheduled for future release that should not yet be published in documentation Update the links to point to real, publicly available npm packages before merging to production documentation. <details> <summary>⛔ Skipped due to learnings</summary>Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries</details> </blockquote></details> <details> <summary>docs/live/docs/plugins/codecall-plugin.mdx (1)</summary><blockquote> `652-652`: **Clarify automation status for docs/live edits.** Per coding guidelines, `docs/live/docs/**` files are marked as "automation-only." Please confirm whether these manual URL updates should be applied here or if a build/documentation generation process should be updated to produce these links automatically. As a follow-up, verify the updated GitHub links are valid and accessible. Also applies to: 655-655 </blockquote></details> <details> <summary>CLAUDE.md (1)</summary><blockquote> `23-24`: **GitHub URL is valid and consolidation is complete.** The link to https://github.com/agentfront/enclave is publicly accessible and active. The consolidation of the three libraries into a single reference note is complete with no dangling references in CLAUDE.md—the only remaining mentions are the consolidation note itself and an appropriate build command context (`nx test ast-guard`). Documentation is consistent and clearly guides developers to the enclave monorepo. </blockquote></details> <details> <summary>docs/draft/docs/plugins/codecall/security.mdx (1)</summary><blockquote> `1247-1247`: **Verify Security Audit link is valid.** The link to the Security Audit document points to https://github.com/agentfront/enclave/blob/main/libs/ast-guard/docs/SECURITY-AUDIT.md. Confirm this URL is accessible and the file exists in the referenced repository location. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
New Features
Enhancements
Configuration
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.