feat(sdk): Extend tool outputSchema and Add prettier #52
Conversation
WalkthroughRefactors many demo tools to return flat primitives/tuples; expands SDK tool typing and parsed-output pipeline; adds a comprehensive MCP error system and ErrorHandler; replaces direct cache access with CacheStore-backed cache; removes ReportHoursTool; and introduces a new json-schema-to-zod-v3 library with converters, handlers, security, tests, and packaging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Handler as MCP Handler
participant Flow as Call-Tool Flow
participant Registry
participant Tool as ToolInstance
participant Cache as CacheStore
Client->>Handler: CallToolRequest
Handler->>Flow: runFlowForOutput(...)
Flow->>Registry: findTool(by name/fullName)
Registry-->>Flow: ToolEntry
Flow->>Cache: read(hash{tool,input})
alt cache hit & valid
Cache-->>Flow: cachedRawOutput
Flow->>Tool: safeParseOutput(cachedRawOutput)
Tool-->>Flow: parsedResult
else cache miss or invalid
Flow->>Tool: execute(input)
Tool-->>Flow: rawOutput
Flow->>Cache: write(hash, rawOutput, ttl)
Flow->>Tool: safeParseOutput(rawOutput)
Tool-->>Flow: parsedResult
end
Flow-->>Handler: CallToolResult (parsedResult)
Handler-->>Client: Response or formatted MCP error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Focus areas that need careful review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/plugins/src/cache/cache.plugin.ts (2)
69-72: Fix cache miss logic to avoid persisting undefined values.Lines 69-72 attempt to persist the cached value for sliding window TTL refresh without first checking if the value exists. On a cache miss,
cachedwill beundefinedornull, and this will be persisted to the cache store. This is likely unintended behavior.Apply this diff to check for value existence before persisting:
- if (cache == true || (cache.ttl && cache.slideWindow)) { + if (cached && (cache == true || (cache.ttl && cache.slideWindow))) { const ttl = cache === true ? this.defaultTTL : cache.ttl ?? this.defaultTTL; await redis.setValue(hash, cached, ttl); }
21-21: Initialize defaultTTL field in constructor.The
defaultTTLfield is declared on line 21 but never initialized in the constructor (lines 50-56). However, it's used in lines 70 and 92, which will result inundefinedat runtime and cause incorrect TTL behavior.Apply this diff to initialize the field:
constructor(options: CachePluginOptions = CachePlugin.defaultOptions) { super(); this.options = { defaultTTL: 60 * 60 * 24, ...options, }; + this.defaultTTL = this.options.defaultTTL; }
🧹 Nitpick comments (4)
libs/plugins/src/cache/cache.plugin.ts (2)
65-65: Consider renaming variable for clarity.The variable
redisis misleading since the cache store can be either a Redis provider or a memory provider depending on configuration (see lines 16, 31). Consider renaming tocacheStoreorstorefor accuracy.Apply this diff:
- const redis = this.get(CacheStoreToken); + const cacheStore = this.get(CacheStoreToken);Then update subsequent references accordingly.
Also applies to: 90-90
75-75: Remove console.log statements from SDK code.Console.log statements should not be present in production SDK library code. Consider either removing them or replacing with a proper logger that can be configured by consumers.
Apply this diff to remove the console.log statements:
if (cached) { - console.log('return from cache', { cached }); ctx.respond({ ...cached, ___cached__: true, }); }- console.log('willWriteCache', { cache }); const ttl = cache === true ? this.defaultTTL : cache.ttl ?? this.defaultTTL;Also applies to: 91-91
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (1)
63-67: Double-check intended semantics ofsiteScopeddetection
const siteScoped = meta.site?.siteScoped || siteId !== undefined;means any call that provides asiteIdwill be treated as site-scoped, even ifmeta.site?.siteScopedis explicitlyfalse. If you ever need tools that carry asiteIdfield but should not be treated as site-scoped, consider using nullish coalescing instead:- const siteScoped = meta.site?.siteScoped || siteId !== undefined; + const siteScoped = meta.site?.siteScoped ?? (siteId !== undefined);If you never plan to set
siteScoped: falseexplicitly and only usetrue/undefined, the current logic is fine; please just confirm which behavior you want.apps/demo/src/apps/employee-time/tools/list-employees.tool.ts (1)
14-52: Output schema and paging are correct; consider deduping pagination logicThe explicit
outputSchemamatches whatexecutereturns (total, enricheditemswithemployeeId/sites, and apageobject with boundedoffset/limit), and it lines up withEmployeeDirectoryProvider.listEmployeesbehavior. The extra site lookup per employee vialistSitesForEmployeeis clear and correct for the demo.You do recompute the
limitclamp here with the same logic as inEmployeeDirectoryProvider.listEmployees. If that logic ever changes, you’ll need to update it in two places; optionally consider having the provider return the effectiveoffset/limit(or a shared helper) and using that to populatepage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.prettierrc(1 hunks)apps/demo/src/apps/calculator/tools/abs.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/add.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/ceil.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/divide.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/exp.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/floor.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/max.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/min.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/modulo.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/multiply.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/pow.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/round.tool.ts(2 hunks)apps/demo/src/apps/calculator/tools/sqrt.tool.ts(1 hunks)apps/demo/src/apps/calculator/tools/subtract.tool.ts(1 hunks)apps/demo/src/apps/employee-time/index.ts(1 hunks)apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts(4 hunks)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts(3 hunks)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts(2 hunks)apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts(2 hunks)apps/demo/src/apps/employee-time/tools/get-employee-details.tool.ts(3 hunks)apps/demo/src/apps/employee-time/tools/list-employees.tool.ts(2 hunks)apps/demo/src/apps/employee-time/tools/register-exit.tool.ts(2 hunks)apps/demo/src/apps/employee-time/tools/report-hours.tool.ts(0 hunks)apps/demo/src/apps/expenses/index.ts(1 hunks)apps/demo/src/apps/expenses/tools/add.tool.ts(1 hunks)apps/demo/src/apps/expenses/tools/create-expense.tool.ts(2 hunks)apps/demo/src/main.ts(1 hunks)docs/blog/11-2025/introducing-frontmcp.mdx(12 hunks)docs/blog/11-2025/mcp-run-out-of-socket.mdx(10 hunks)docs/blog/external-links.mdx(1 hunks)docs/blog/index.mdx(3 hunks)docs/blog/style.css(1 hunks)docs/docs.json(7 hunks)docs/snippets/card.jsx(1 hunks)libs/plugins/src/cache/cache.plugin.ts(5 hunks)libs/plugins/src/cache/cache.symbol.ts(1 hunks)libs/plugins/src/cache/cache.types.ts(1 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)libs/sdk/src/common/metadata/tool.metadata.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/demo/src/apps/employee-time/tools/report-hours.tool.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/cache/cache.symbol.tslibs/plugins/src/cache/cache.types.tslibs/plugins/src/cache/cache.plugin.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/tool.decorator.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/calculator/tools/modulo.tool.tsapps/demo/src/apps/calculator/tools/divide.tool.tsapps/demo/src/apps/calculator/tools/exp.tool.tsapps/demo/src/apps/employee-time/index.tsapps/demo/src/main.tsapps/demo/src/apps/calculator/tools/min.tool.tsapps/demo/src/apps/calculator/tools/max.tool.tsapps/demo/src/apps/calculator/tools/floor.tool.tsapps/demo/src/apps/calculator/tools/add.tool.tsapps/demo/src/apps/expenses/tools/create-expense.tool.tsapps/demo/src/apps/expenses/index.tsapps/demo/src/apps/employee-time/providers/employee-directory.provider.tsapps/demo/src/apps/calculator/tools/subtract.tool.tsapps/demo/src/apps/employee-time/plugins/site-authorization.plugin.tsapps/demo/src/apps/calculator/tools/round.tool.tsapps/demo/src/apps/employee-time/tools/register-exit.tool.tsapps/demo/src/apps/expenses/tools/add.tool.tsapps/demo/src/apps/calculator/tools/sqrt.tool.tsapps/demo/src/apps/employee-time/tools/get-employee-details.tool.tsapps/demo/src/apps/calculator/tools/ceil.tool.tsapps/demo/src/apps/employee-time/tools/admin-add-entry.tool.tsapps/demo/src/apps/employee-time/tools/admin-add-exit.tool.tsapps/demo/src/apps/calculator/tools/abs.tool.tsapps/demo/src/apps/calculator/tools/multiply.tool.tsapps/demo/src/apps/calculator/tools/pow.tool.tsapps/demo/src/apps/employee-time/tools/list-employees.tool.ts
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/blog/11-2025/mcp-run-out-of-socket.mdxdocs/blog/style.cssdocs/docs.jsondocs/snippets/card.jsxdocs/blog/index.mdxdocs/blog/11-2025/introducing-frontmcp.mdxdocs/blog/external-links.mdx
docs/docs.json
⚙️ CodeRabbit configuration file
docs/docs.json: Mintlify navigation configuration. Important structure: - dropdowns[].dropdown == "Documentation":
- Contains a "versions" array.
- Latest version entry: "version": "vX.Y (latest)", "default": true, with
paths like "docs/getting-started/welcome" and "updates" (NO "v/X.Y" prefix).- Older versions: "version": "v0.2", "version": "v0.1", etc., with paths
like "docs/v/0.2/getting-started/welcome".- dropdowns[].dropdown == "Blog":
- Contains blog groups (e.g. "blog/november-2025/introducing-frontmcp").
Review guidelines: - Do NOT suggest editing the Blog dropdown unless the PR explicitly adds/changes blogs. - Ensure latest version uses non-versioned "docs/..." paths, and older versions
use "docs/v/{minor}/..." paths.- If you see changes that try to route archived versions away from docs/v/**,
flag that as incorrect.
Files:
docs/docs.json
🧬 Code graph analysis (17)
libs/plugins/src/cache/cache.types.ts (1)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
EmployeeRedisProvider(11-88)
apps/demo/src/apps/employee-time/index.ts (2)
apps/demo/src/apps/employee-time/providers/redis.provider.ts (1)
createEmployeeRedisProvider(90-96)apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
createEmployeeDirectoryProvider(64-70)
libs/plugins/src/cache/cache.plugin.ts (1)
libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)
apps/demo/src/apps/expenses/tools/create-expense.tool.ts (2)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)apps/demo/src/apps/expenses/tools/add.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/expenses/index.ts (2)
apps/demo/src/apps/employee-time/index.ts (1)
App(14-34)apps/demo/src/apps/calculator/index.ts (1)
App(19-50)
apps/demo/src/apps/calculator/tools/subtract.tool.ts (4)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (3)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (1)
Tool(8-57)apps/demo/src/apps/employee-time/utils/keys.ts (2)
sessionsKey(22-24)openKey(14-16)
apps/demo/src/apps/expenses/tools/add.tool.ts (1)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/calculator/tools/sqrt.tool.ts (3)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/employee-time/tools/get-employee-details.tool.ts (2)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (1)
Tool(8-57)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (2)
apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (1)
Tool(8-57)apps/demo/src/apps/employee-time/tools/get-employee-details.tool.ts (1)
Tool(9-84)
apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (2)
apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)apps/demo/src/apps/employee-time/utils/keys.ts (2)
sessionsKey(22-24)openKey(14-16)
apps/demo/src/apps/calculator/tools/abs.tool.ts (2)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/calculator/tools/multiply.tool.ts (5)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/pow.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/subtract.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/calculator/tools/pow.tool.ts (6)
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/multiply.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/round.tool.ts (1)
Tool(4-22)apps/demo/src/apps/calculator/tools/sqrt.tool.ts (1)
Tool(4-14)
apps/demo/src/apps/employee-time/tools/list-employees.tool.ts (1)
apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
EmployeeDirectoryProvider(5-62)
libs/sdk/src/common/decorators/tool.decorator.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (5)
ToolMetadata(138-186)ImageOutputSchema(87-87)AudioOutputSchema(94-94)ResourceOutputSchema(101-101)ResourceLinkOutputSchema(108-108)apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)
🪛 ESLint
libs/sdk/src/common/metadata/tool.metadata.ts
[error] 15-15: An empty interface declaration allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowInterfaces' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 15-15: An empty interface is equivalent to {}.
(@typescript-eslint/no-empty-interface)
libs/sdk/src/common/decorators/tool.decorator.ts
[error] 188-188: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 195-195: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 202-202: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 218-218: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 221-221: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
🪛 LanguageTool
docs/blog/11-2025/mcp-run-out-of-socket.mdx
[style] ~130-~130: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...with separate auth and base paths. - Sometimes expose an app as a standalone MCP serve...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/blog/external-links.mdx
[uncategorized] ~12-~12: The official name of this software platform is spelled with a capital “H”.
Context: ...={'GitHub Repo'} horizontal arrow icon="github"> FrontMCP source code on Github </Ca...
(GITHUB)
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...con="github"> FrontMCP source code on Github <Card href={'/updates'} title={...
(GITHUB)
🔇 Additional comments (31)
libs/plugins/src/cache/cache.types.ts (1)
1-1: LGTM!The import statement formatting aligns with Prettier conventions (spaces inside braces). This is a non-semantic formatting change with no impact on the API or runtime behavior.
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (4)
1-1: SDK import usage looks clean and minimalImporting only
DynamicPlugin,Plugin,ToolHook, andFlowCtxOfkeeps this plugin’s dependency surface tight and is consistent with how the rest of the file uses the SDK.
5-9: Tool metadata extension remains clear and flexibleThe optional
sitemetadata shape is well-structured and keeps all site-related flags grouped, which makes it easy for tools to opt into site-scoping and admin requirements without breaking existing metadata.
25-29: Option merging with nullish coalescing preserves explicitfalseUsing
opts.demoAllowAllIfNoClaims ?? trueandopts.siteIdFieldName ?? 'siteId'is correct here: explicitfalseand empty strings are preserved, whileundefinedfalls back to sensible defaults.
57-58: Authorization hook placement and priority are appropriateRunning
validateSiteAccessas aWill('execute')hook with high priority ensures site authorization is enforced early in the tool pipeline, which is a sensible choice for auth in this demo plugin.libs/plugins/src/cache/cache.symbol.ts (1)
1-4: Formatting-only change; cache token export remains correct.Imports and the
CacheStoreTokentypedSymbolare unchanged in behavior and remain suitable for DI lookups..prettierrc (1)
23-26: Prettier override reformatting only.Switching the YAML
filespattern list to multi-line doesn’t change behavior; config remains valid.apps/demo/src/apps/employee-time/providers/employee-directory.provider.ts (1)
3-56: Employee directory tweaks are purely structural.Reformatted imports, expanded
listEmployeesoptions type, and the parenthesized filter parameter leave behavior unchanged; case-insensitive search, pagination, and Redis access remain the same.docs/blog/11-2025/mcp-run-out-of-socket.mdx (1)
7-485: MDX/JSX structure and formatting look sound.The refactor to JSX-style image blocks, cards, steps, and code fences preserves content, keeps components well-formed, and maintains
alttext and link targets. No structural or readability issues detected.docs/snippets/card.jsx (1)
1-101: NewBlogCardlayout is coherent and matches light/dark image needs.The consolidated card component cleanly handles optional dark-mode imagery, wraps content in a single anchor for clickability, and exposes a clear content/footer structure that aligns with the updated blog styles.
apps/demo/src/apps/calculator/tools/ceil.tool.ts (1)
8-13: CeilTool now returns a primitive number consistent with the schema.Using
outputSchema: 'number'and returningMath.ceil(input.x)aligns this tool with the simplified numeric outputs used by the other calculator tools.apps/demo/src/apps/expenses/tools/create-expense.tool.ts (1)
1-27: CreateExpenseTool schemas and return shape are consistent.The documented
idinput, simple{ ok: string }output, cache settings, andrequiredRoles: ['Admin']all line up with theexecuteimplementation and make the tool easy to understand in the demo context.apps/demo/src/apps/employee-time/index.ts (1)
1-34: LGTM! Formatting improvements and intentional tool removal.The import formatting adjustments and removal of ReportHoursTool align with the PR objectives. The app structure and provider/plugin configurations remain correct.
apps/demo/src/apps/calculator/tools/floor.tool.ts (1)
8-8: LGTM! Consistent with calculator tool refactoring pattern.The outputSchema and return value changes align with the systematic refactoring across all calculator tools to return primitive numbers directly.
Also applies to: 12-12
apps/demo/src/apps/calculator/tools/modulo.tool.ts (1)
8-8: LGTM! Consistent refactoring pattern applied.The changes follow the same primitive output pattern as other calculator tools. The modulo logic remains correct.
Also applies to: 12-12
docs/blog/index.mdx (1)
10-10: LGTM! Documentation updates look good.The import formatting, new blog post entry, and light/dark image support are all properly implemented. The content is clear and the metadata is complete.
Also applies to: 24-34, 55-57
apps/demo/src/apps/calculator/tools/exp.tool.ts (1)
8-8: LGTM! Follows the consistent refactoring pattern.The changes align with the calculator tool API simplification. The exponential calculation logic is correct.
Also applies to: 12-12
apps/demo/src/apps/calculator/tools/max.tool.ts (1)
8-8: LGTM! Consistent with the systematic refactoring.The outputSchema and return value changes follow the established pattern across calculator tools. The max calculation logic remains correct.
Also applies to: 12-12
apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
1-2: LGTM! Formatting and API refactoring applied consistently.The import formatting, schema spacing, and primitive return pattern are all properly implemented and consistent with the other calculator tools reviewed.
Also applies to: 7-8, 12-12
apps/demo/src/apps/calculator/tools/min.tool.ts (1)
8-8: Code change is correct and fully supported by the SDK.The primitive outputSchema value
'number'is officially supported by the SDK. ThePrimitiveOutputTypeis explicitly defined inlibs/sdk/src/common/metadata/tool.metadata.tsand includes'string' | 'number' | 'date' | 'boolean'. All 14+ existing calculator tools already use this exact pattern consistently.The min.tool.ts change matches the established codebase convention—no breaking changes or incompatibilities exist.
apps/demo/src/main.ts (1)
1-15: LGTM! Clean formatting improvements.The formatting changes consistently apply proper spacing in imports, decorator options, and exports. These align with the prettier configuration added in this PR.
apps/demo/src/apps/calculator/tools/sqrt.tool.ts (1)
8-12: LGTM! Consistent primitive output schema.The shift from
{ result: number }to primitivenumbersimplifies the tool's API and aligns with the pattern applied consistently across calculator tools (abs, add, multiply, divide, etc.).apps/demo/src/apps/calculator/tools/add.tool.ts (1)
8-12: LGTM! Consistent with the API refactor.The primitive output schema and direct number return align with the broader refactor across calculator tools.
apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
8-12: LGTM! Consistent refactor.The primitive output schema follows the same pattern as other calculator tools.
apps/demo/src/apps/calculator/tools/pow.tool.ts (1)
7-12: LGTM! Consistent API simplification.The formatting improvements and primitive output schema align with the broader calculator tools refactor.
docs/blog/11-2025/introducing-frontmcp.mdx (1)
1-321: LGTM! Documentation formatting improvements.The formatting changes throughout this documentation file improve readability and align with the prettier configuration. Code examples now follow consistent spacing patterns.
apps/demo/src/apps/expenses/tools/add.tool.ts (1)
8-8: Both formats are officially supported by the SDK; the inconsistency is a style choice, not an error.The SDK type system explicitly supports both string literals (
'number') and Zod schemas (z.number()) as validoutputSchemavalues. The calculator tools use string literals while the expenses and employee-time tools use Zod schemas—both approaches are correct according to the SDK's__PrimitiveOutputTypedefinition, which includes both formats.The expenses tool's use of
z.number()is actually the more type-safe approach and aligns with structured output patterns in the employee-time tools. If consistency across demo tools is desired, consider standardizing on one approach (preferably Zod schemas for type safety), but the current code is not technically incorrect.apps/demo/src/apps/calculator/tools/round.tool.ts (1)
11-21: Flattened output shape looks correct; verify callers updated
outputSchema: 'number'now aligns withexecutereturning a barenumberin both branches (with/withoutdigits), which is consistent with the PR’s move away from{ result: ... }wrappers. Just make sure any consumers that previously expected{ result: number }have been updated to handle the primitive return value.docs/blog/style.css (1)
2-80: Blog card layout and dark-mode styles look consistentThe width constraints, image container styling, light/dark image toggling, and mobile typography adjustments all look coherent and should work well with the Tailwind-based layout hinted in the comments.
docs/docs.json (1)
10-310: Docs navigation structure matches versioning guidelinesThe updated
favicon, contextual options, and especially theversionsarray look good:v0.3 (latest)uses non-versioneddocs/...paths,v0.2/v0.1correctly usedocs/v/{minor}/..., and the new"main (draft)"section is hidden and scoped underdraft/docs/...so it won’t interfere with the live tree. The Blog dropdown changes are purely formatting.apps/demo/src/apps/employee-time/tools/register-exit.tool.ts (1)
1-47: Register-exit logic aligns with admin-add-exit and new flat outputThe open-session validation, per-day splitting via
splitSessionByDay,hoursKey/sessionsKeyupdates, and cleanup of theopenKeymirror the pattern inAdminAddExitTool, withendtaken fromDate.now(). The newoutputSchemaandreturn { employeeId, endedAt: end, durationMs: total }keep the runtime shape and schema in sync and align with the PR’s flattened outputs.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/common/entries/tool.entry.ts (1)
14-16: Update CHANGELOG.md to document breaking change in ToolCallExtra type.The addition of
authInfo: AuthInfoto the exportedToolCallExtratype is a breaking change for SDK consumers who extendToolEntry. This change is not documented in CHANGELOG.md. When upgrading from one version to another, breaking changes must be clearly listed in the changelog.Any external code implementing
ToolEntry.create()will now require theauthInfofield in theirToolCallExtraobjects. Additionally, per the coding guidelines, public API changes should include corresponding documentation updates indocs/draft/docs/**.
🧹 Nitpick comments (1)
libs/sdk/src/tool/flows/tools-list.flow.ts (1)
155-157: Remove unnecessaryas anycast ontool.metadatafor type safety consistency.Verification confirms that
tool.metadatais properly typed asToolMetadatathrough theToolEntryclass hierarchy. TheinputJsonSchemaproperty is defined in theToolMetadatainterface as an optional field (line 165), making theas anycast unnecessary and inconsistent with how other metadata properties (name,description,annotations) are accessed on the same line.Apply this diff:
- inputSchema: - (tool.metadata as any).inputJsonSchema ?? - (zodToJsonSchema(z.object(tool.metadata.inputSchema) as any) as any), + inputSchema: + tool.metadata.inputJsonSchema ?? + (zodToJsonSchema(z.object(tool.metadata.inputSchema) as any) as any),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/demo/src/apps/expenses/index.ts(1 hunks)apps/demo/src/apps/expenses/tools/get-expense-fun.tool.ts(1 hunks)docs/blog/external-links.mdx(1 hunks)docs/draft/docs/servers/tools.mdx(1 hunks)libs/adapters/src/openapi/openapi.tool.ts(2 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)libs/sdk/src/common/entries/tool.entry.ts(2 hunks)libs/sdk/src/common/metadata/tool.metadata.ts(4 hunks)libs/sdk/src/common/tokens/tool.tokens.ts(1 hunks)libs/sdk/src/tool/flows/tools-list.flow.ts(1 hunks)libs/sdk/src/tool/tool.instance.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/demo/src/apps/expenses/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/sdk/src/tool/tool.instance.tslibs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/adapters/src/openapi/openapi.tool.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/common/tokens/tool.tokens.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.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/blog/external-links.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/apps/expenses/tools/get-expense-fun.tool.ts
🧬 Code graph analysis (2)
libs/sdk/src/common/decorators/tool.decorator.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (5)
ToolMetadata(138-186)ImageOutputSchema(87-87)AudioOutputSchema(94-94)ResourceOutputSchema(101-101)ResourceLinkOutputSchema(108-108)apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)
libs/sdk/src/common/tokens/tool.tokens.ts (1)
libs/sdk/src/common/metadata/tool.metadata.ts (1)
ToolMetadata(138-186)
🪛 ESLint
libs/sdk/src/common/metadata/tool.metadata.ts
[error] 15-15: An empty interface declaration allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowInterfaces' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 15-15: An empty interface is equivalent to {}.
(@typescript-eslint/no-empty-interface)
libs/sdk/src/common/decorators/tool.decorator.ts
[error] 35-35: Constraining the generic type In to unknown does nothing and is unnecessary.
(@typescript-eslint/no-unnecessary-type-constraint)
[error] 35-35: Constraining the generic type Out to unknown does nothing and is unnecessary.
(@typescript-eslint/no-unnecessary-type-constraint)
[error] 191-191: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 198-198: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 205-205: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 221-221: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 224-224: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
🪛 LanguageTool
docs/blog/external-links.mdx
[uncategorized] ~12-~12: The official name of this software platform is spelled with a capital “H”.
Context: ...={'GitHub Repo'} horizontal arrow icon="github"> FrontMCP source code on GitHub </Ca...
(GITHUB)
🔇 Additional comments (13)
docs/blog/external-links.mdx (1)
9-20: LGTM! Formatting improvements applied consistently.The Card components are now consistently formatted with single-quoted attributes and explicit closing tags, aligning with the Prettier formatting changes in this PR. The content is clear and well-organized.
Note: The static analysis hint flagging a capitalization issue on line 12 is a false positive—"GitHub" is already correctly capitalized in both the title attribute and body text.
apps/demo/src/apps/expenses/tools/get-expense-fun.tool.ts (1)
3-3: LGTM! Import path typo corrected.The import path has been correctly fixed from "../provders" to "../providers", resolving what would have been a runtime import error.
libs/sdk/src/tool/tool.instance.ts (1)
20-33: ExposeinputJsonSchemafrom metadata (aligned with ToolMetadata).Wiring
this.inputJsonSchema = record.metadata.inputJsonSchemakeeps the instance surface consistent with the newToolMetadata.inputJsonSchemafield and doesn’t alter validation behavior; looks good.libs/sdk/src/common/decorators/tool.decorator.ts (1)
71-169: New input/output inference helpers andTooloverloads look consistent.The
ToolInputOf/ToolOutputOfhelpers, constrained__OutputSchema, and the twoTooloverloads (with and withoutoutputSchema) line up withToolMetadataand the demo usages (primitive literals, Zod objects, arrays), and they now correctly allow primitive/array outputs while enforcing rich compile-time checks onexecuteparameter/return types. No further issues from a typing or API-surface perspective.Also applies to: 237-269
docs/draft/docs/servers/tools.mdx (1)
56-76: Docs metadata snippet correctly renamed toinputJsonSchema.Renaming the metadata field to
inputJsonSchema?: JSONSchema7matches the updatedToolMetadatainterface and the token/instance changes, so the docs remain in sync with the SDK.libs/sdk/src/common/tokens/tool.tokens.ts (1)
5-19: Token rename toinputJsonSchemaand newextendedToolMetadatalook correct.Using
tokenFactory.meta('inputJsonSchema')aligns this token withToolMetadata.inputJsonSchemaand the updated docs, and exportingextendedToolMetadatamatches its usage intool.decorator.tsfor storing extra metadata. Just confirm that any remaining references to the oldrawInputSchematoken were updated and thatExtendFrontMcpToolMetadatais properly imported in this file so theRawMetadataShape<...>annotation compiles.libs/adapters/src/openapi/openapi.tool.ts (1)
12-19: OpenAPI tool now correctly populatesinputJsonSchema.Passing
inputJsonSchema: oTool.inputSchemaalongside the derived ZodinputSchemakeeps the generated tool metadata consistent with the SDK’sinputJsonSchemafield and tokens, without changing runtime behavior.libs/sdk/src/common/entries/tool.entry.ts (1)
1-2: LGTM! File header added for clarity.libs/sdk/src/common/metadata/tool.metadata.ts (5)
70-133: LGTM! Comprehensive output type system.The new output typing system provides excellent coverage for primitives, media types (image/audio), resources, and structured data. The exported schemas (ImageOutputSchema, AudioOutputSchema, etc.) give consumers clear contract definitions.
138-186: LGTM! Well-designed generic metadata interface.The generic ToolMetadata interface with In/Out type parameters provides strong typing while maintaining backward compatibility through sensible defaults. The dual representation (inputSchema as Zod, inputJsonSchema as JSONSchema7) appropriately serves both validation and serialization needs.
197-221: Past issue resolved: runtime schema now accepts primitives.The previous review flagged that
outputSchemarequiredz.instanceof(Object), which would reject primitive outputs. This has been fixed! The newtoolOutputSchema(lines 218-221) properly accepts:
- Primitive literals (
'string','number', etc.)- Special content types (
'image','audio', etc.)- Zod schema instances (objects, arrays, unions)
- Raw shapes
- Arrays of any of the above
This aligns with the ToolOutputType typings and enables the primitive outputs used throughout the PR.
222-234: LGTM! Runtime schema correctly implements the type system.The
frontMcpToolMetadataSchemaproperly validates the ToolMetadata interface. Key aspects:
- Line 229 uses the new flexible
toolOutputSchemathat accepts primitives and complex types- The
.passthrough()at line 234 enables extensibility viaExtendFrontMcpToolMetadata- The
satisfiesconstraint ensures type/runtime alignmentThe permissive
z.any()forinputJsonSchema(line 228) is reasonable since JSON Schema has its own validation semantics.
15-15: Static analysis false positive: empty interface is intentional.The empty
ExtendFrontMcpToolMetadatainterface is an intentional declaration-merging pattern that allows consumers to extend the tool metadata interface viadeclare global. This is a standard TypeScript extensibility pattern.The ESLint warnings can be safely ignored, or you could add an inline comment to suppress them:
+ // eslint-disable-next-line @typescript-eslint/no-empty-interface, @typescript-eslint/no-empty-object-type interface ExtendFrontMcpToolMetadata {}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/sdk/src/tool/tool.instance.ts (3)
45-52: Consider stronger typing for metadata schema properties.The type casts to
anyon lines 49 and 52 reduce type safety. While this provides flexibility, it makes it easier to accidentally access non-existent properties or assign incorrect values.Consider defining explicit types for these properties in ToolMetadata or using type guards to validate the structure at runtime.
Apply this pattern to improve type safety:
- this.inputJsonSchema = (record.metadata as any).inputJsonSchema; + this.inputJsonSchema = record.metadata.inputJsonSchema;And ensure ToolMetadata properly declares these properties (which it appears to do based on the relevant snippets).
147-159: Consider validating array/scalar mismatch more strictly.Line 149 silently wraps non-array values in an array when the descriptor expects an array. This could mask bugs where a tool returns a single value instead of the expected array.
Consider logging a warning or throwing an error when there's a type mismatch:
if (Array.isArray(descriptor)) { - // Multiple content items; expect raw to be an array of the same length. - const values = Array.isArray(raw) ? raw : [raw]; + // Multiple content items; expect raw to be an array of the same length. + if (!Array.isArray(raw)) { + console.warn(`Expected array output but received ${typeof raw}; wrapping in array`); + } + const values = Array.isArray(raw) ? raw : [raw];
266-277: Silent content dropping may hide errors.Lines 275-276 return an empty array when content doesn't match the expected type. This silently discards potentially valuable diagnostic information.
Consider logging a warning when content is dropped:
if (value && typeof value === 'object' && (value as any).type === expectedType) { return [value as T]; } - // If the value is null/undefined or doesn't match, we just skip it. + // If the value is null/undefined or doesn't match, we just skip it. + if (value != null) { + console.warn(`Expected content type '${expectedType}' but received:`, value); + } return [];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/tool/tool.instance.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/tool/tool.instance.ts
🧬 Code graph analysis (2)
libs/sdk/src/common/entries/tool.entry.ts (1)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)
libs/sdk/src/tool/tool.instance.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/scope/scope.instance.ts (1)
hooks(124-126)libs/sdk/src/common/entries/tool.entry.ts (1)
ParsedToolResult(18-22)
🔇 Additional comments (6)
libs/sdk/src/tool/tool.instance.ts (2)
57-65: LGTM!The hook filtering and registration logic is clear and correctly implemented. The use of arrow functions and explicit flow filtering improves readability.
293-344: Excellent sanitization implementation.The
sanitizeForJsonfunction comprehensively handles edge cases including circular references, BigInt, Date, Error, Map, and Set conversions. The use of WeakSet to track seen objects is an efficient approach to preventing circular reference errors.libs/sdk/src/common/entries/tool.entry.ts (4)
17-22: Well-designed interface for MCP compliance.The
ParsedToolResultinterface appropriately structures tool outputs for MCP compatibility with required content blocks and optional structured data. The optionalisErrorflag enables proper error handling.
31-35: Type looseness provides flexibility at the cost of safety.The
anytype forinputJsonSchema(line 33) and optionaloutputSchema(line 35) provide flexibility for various schema formats but reduce compile-time safety. This is likely intentional to support different JSON schema representations, but be aware that this means:
- Invalid schema assignments won't be caught at compile time
- Tools without output schemas may produce harder-to-validate results
Verify that this level of type flexibility aligns with the SDK's design goals and that runtime validation compensates for the lack of compile-time checks.
37-44: LGTM!The
getOutputSchema()accessor is well-documented and provides a clean interface for accessing the tool's output schema declaration. The documentation clearly explains its purpose for tools/list.
46-55: Abstract method design promotes proper separation of concerns.The
parseOutput()abstract method appropriately delegates implementation details to concrete classes while establishing a clear contract. The documentation effectively explains its purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/sdk/src/common/decorators/tool.decorator.ts (1)
35-38: Remove unnecessary type constraints.The
extends unknownconstraints are redundant and trigger the ESLint rule. Simply remove them.Apply this diff:
-export type FrontMcpToolExecuteHandler<In extends unknown, Out extends unknown> = ( +export type FrontMcpToolExecuteHandler<In = unknown, Out = unknown> = ( input: In, ctx: ToolContext<In, Out>, ) => Out | Promise<Out>;
🧹 Nitpick comments (3)
libs/plugins/src/cache/cache.plugin.ts (3)
56-76: Cache read path: verify behavior with non‑object outputs and cache metadata shapeThe switch to
cacheStore.getValueandcacheStore.setValuewith TTL resolution viathis.options.defaultTTLis sound, but there are a couple of edge cases worth checking:
ctx.respond({ ...cached, ___cached__: true })assumescachedis an object. If tool outputs can now be primitives (as implied by the broader PR), this will silently drop the value and only return{ ___cached__: true }. If primitives are possible, consider:
- wrapping the value (e.g.
{ value: cached, ___cached__: true }), or- storing/reading a stable envelope object in the cache.
- The sliding‑TTL condition
if (cache == true || (cache.ttl && cache.slideWindow))is a bit subtle:
- For a boolean
cache === true, you always slide withthis.options.defaultTTL.- For an object config, sliding only happens when
cache.ttlis truthy andcache.slideWindowis true; defaultTTL is not considered in the condition.- If you intend “slide when slideWindow is true regardless of whether ttl is explicit or defaulted”, this condition should be adjusted.
If your contract guarantees
ctx.output/cachedis always a plain object and thatcacheis eithertrueor a well‑typed config object, then this is fine as‑is; otherwise a small adjustment would make it more robust.
79-91: Cache write path: duplicated TTL logic and potential helper extractionThe write hook’s switch to
cacheStore.setValueand TTL resolution:const ttl = cache === true ? this.options.defaultTTL : cache.ttl ?? this.options.defaultTTL; await cacheStore.setValue(hash, ctx.output, ttl);is consistent with the read path and correctly uses nullish coalescing so
ttl: 0is preserved.Given the identical TTL expression in both
willReadCacheandwillWriteCache, consider extracting a small helper (e.g.resolveTtl(cache, this.options.defaultTTL)) to keep behavior in sync if it’s tweaked later and to centralize the semantics ofcache === truevs object configs.
94-105: hashObject is stable for plain JSON, but collisions and non‑JSON inputs are possibleThe hashing strategy (sorted keys + recursive concatenation of values) is simple and deterministic for typical JSON‑like inputs:
const keys = Object.keys(obj).sort(); const values = keys.map((key) => obj[key]);Given this is used for cache keys, that’s probably sufficient, but be aware:
- It doesn’t distinguish well between some shapes (e.g., different structures that stringify to the same concatenation).
- Cyclic or non‑JSON values (Dates, functions, etc.) would either recurse oddly or throw.
If tool inputs are guaranteed to be JSON‑serializable and relatively small, this is acceptable. If you expect more complex structures, consider a more robust hash (e.g., stable JSON stringify + a real hash function) in the future.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts(4 hunks)libs/plugins/src/cache/cache.plugin.ts(3 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/cache/cache.plugin.tslibs/sdk/src/common/decorators/tool.decorator.ts
🧬 Code graph analysis (2)
libs/plugins/src/cache/cache.plugin.ts (1)
libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)
libs/sdk/src/common/decorators/tool.decorator.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (5)
ToolMetadata(138-186)ImageOutputSchema(87-87)AudioOutputSchema(94-94)ResourceOutputSchema(101-101)ResourceLinkOutputSchema(108-108)apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)
🪛 ESLint
libs/sdk/src/common/decorators/tool.decorator.ts
[error] 35-35: Constraining the generic type In to unknown does nothing and is unnecessary.
(@typescript-eslint/no-unnecessary-type-constraint)
[error] 35-35: Constraining the generic type Out to unknown does nothing and is unnecessary.
(@typescript-eslint/no-unnecessary-type-constraint)
🔇 Additional comments (4)
libs/plugins/src/cache/cache.plugin.ts (1)
1-17: Cache store token & default provider wiring look correctUsing
CacheStoreTokenfor both the default memory provider and dynamic providers keeps the DI surface clean and ensures the plugin can transparently swap cache backends. The addedCachePluginOptionsimport matches the generic onDynamicPlugin<CachePluginOptions>and the comment on the default provider clarifies override behavior.No functional issues in this hunk.
libs/sdk/src/common/decorators/tool.decorator.ts (3)
190-229: Nice fix on the helper types!The branded error helpers now correctly use
unknowninstead of{}for the "OK" branches, which satisfies theno-empty-object-typelint rule while preserving the correct intersection behavior.
75-128: Well-structured type inference!The output type inference machinery comprehensively handles primitive literals, MCP content types, Zod schemas, and arrays. The logic correctly maps each schema form to its corresponding TypeScript type.
237-269: Solid module augmentation design!The two decorator overloads are well-structured:
- Strict typing when
outputSchemais provided- Flexible typing when omitted
The overload order is correct (most specific first), and the branded error types will provide helpful compile-time feedback to developers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/sdk/src/common/metadata/tool.metadata.ts (1)
11-16: Add eslint-disable comment for intentional empty interface.The empty
ExtendFrontMcpToolMetadatainterface is a standard module-augmentation pattern that allows consumers to extend tool metadata. However, it triggers ESLint warnings.Apply this diff to suppress the expected warnings:
declare global { /** * Declarative metadata extends to the an McpTool decorator. */ + // eslint-disable-next-line @typescript-eslint/no-empty-object-type, @typescript-eslint/no-empty-interface interface ExtendFrontMcpToolMetadata {} }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CODE_OF_CONDUCT.md(1 hunks)libs/adapters/src/openapi/openapi.tool.ts(1 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/common/metadata/tool.metadata.ts(3 hunks)libs/sdk/src/common/tokens/tool.tokens.ts(1 hunks)libs/sdk/src/tool/tool.instance.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/common/tokens/tool.tokens.ts
- libs/adapters/src/openapi/openapi.tool.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/tool/tool.instance.tslibs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/common/decorators/tool.decorator.ts
🧬 Code graph analysis (3)
libs/sdk/src/tool/tool.instance.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/scope/scope.instance.ts (1)
hooks(124-126)libs/sdk/src/common/entries/tool.entry.ts (1)
ParsedToolResult(18-22)
libs/sdk/src/common/entries/tool.entry.ts (1)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)
libs/sdk/src/common/decorators/tool.decorator.ts (5)
libs/sdk/src/common/metadata/tool.metadata.ts (5)
ToolMetadata(138-186)ImageOutputSchema(87-87)AudioOutputSchema(94-94)ResourceOutputSchema(101-101)ResourceLinkOutputSchema(108-108)apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/divide.tool.ts (1)
Tool(4-14)apps/demo/src/apps/calculator/tools/round.tool.ts (1)
Tool(4-22)apps/demo/src/apps/employee-time/tools/admin-add-exit.tool.ts (1)
Tool(8-57)
🪛 ESLint
libs/sdk/src/common/metadata/tool.metadata.ts
[error] 15-15: An empty interface declaration allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowInterfaces' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 15-15: An empty interface is equivalent to {}.
(@typescript-eslint/no-empty-interface)
🪛 LanguageTool
CODE_OF_CONDUCT.md
[style] ~25-~25: Try using a synonym here to strengthen your wording.
Context: ...ind - Trolling, insulting or derogatory comments, and personal or political attacks - Pu...
(COMMENT_REMARK)
🪛 markdownlint-cli2 (0.18.1)
CODE_OF_CONDUCT.md
49-49: Bare URL used
(MD034, no-bare-urls)
93-93: Bare URL used
(MD034, no-bare-urls)
100-100: Bare URL used
(MD034, no-bare-urls)
101-101: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (26)
CODE_OF_CONDUCT.md (1)
1-101: LGTM — good addition to the repository.The addition of a Contributor Covenant Code of Conduct is a best practice for community-driven projects. The document is well-structured, follows the standard template, and establishes clear community expectations and enforcement guidelines. Once the bare URL formatting is corrected, this will be ready.
libs/sdk/src/common/metadata/tool.metadata.ts (6)
1-9: LGTM!The imports are well-organized and all necessary for the expanded type inference system and MCP content schemas.
18-68: LGTM!The
ToolAnnotationsinterface and corresponding runtime schema are well-structured. The use ofsatisfies RawZodShape<ToolAnnotations>ensures compile-time alignment, and.passthrough()correctly allows additional properties.
70-132: LGTM!The output type hierarchy is comprehensive and well-structured, supporting primitives, MCP content types (image, audio, resource, resource_link), and structured data. The exported schemas correctly reference MCP SDK types.
138-186: LGTM!The
ToolMetadatainterface is correctly parameterized with the new generic constraints (In = ToolInputType,Out extends ToolOutputType), enabling rich compile-time type inference for both inputs and outputs.
190-203: LGTM!The runtime literal schemas correctly mirror the TypeScript type definitions, providing proper runtime validation for primitive and special content type literals.
205-234: Runtime schema now correctly accepts primitive outputs!The
frontMcpToolMetadataSchemahas been successfully updated to accept the fullToolOutputTypesurface, including:
- Primitive literals (
'string','number','boolean','date')- Special content types (
'image','audio','resource','resource_link')- Zod schema instances
- Raw shapes
- Arrays of any of the above
This resolves the critical runtime parsing issue flagged in previous reviews where primitive outputs would have been rejected.
libs/sdk/src/common/decorators/tool.decorator.ts (6)
1-33: LGTM!The decorator implementation correctly validates metadata using the updated runtime schema and properly registers it via Reflect API.
35-61: LGTM!The handler type and
frontMcpToolfunction now correctly support primitive outputs viaToolOutputOf<T>. The use of default type parameters (= unknown) instead of constraints (extends unknown) is appropriate.
63-128: LGTM!The type inference helpers are comprehensive and correctly handle:
- Input inference from raw shapes or Zod objects
- Output inference for primitives, MCP content types, Zod schemas, and structured data
- Array/tuple outputs
The implementation enables rich compile-time type safety.
130-168: LGTM!The local type constraints correctly mirror the exported types and are properly used to constrain decorator generics while maintaining type safety.
170-229: LGTM!The constructor reflection helpers and branded error types are well-designed. The "OK" branches correctly return
unknowninstead of{}, satisfying the ESLint rule while maintaining proper intersection behavior.
237-269: LGTM!The dual decorator overloads provide an excellent developer experience:
- When
outputSchemais provided → strict type checking with rich error messages- When
outputSchemais omitted → flexible typingThe use of branded error types (
__MustParam,__MustReturn) will provide clear feedback when there are type mismatches.libs/sdk/src/common/entries/tool.entry.ts (3)
1-22: LGTM!The
ParsedToolResultinterface provides a clean abstraction for tool outputs that aligns with MCP's content model. The use ofContentBlockfrom the MCP SDK ensures protocol compatibility.
24-44: LGTM!The
ToolEntryclass correctly uses the new generic constraints and provides thegetOutputSchema()accessor for exposing declared output schemas to consumers.
46-56: LGTM!The abstract methods
create()andparseOutput()establish a clear contract for concrete tool implementations to convert typed outputs into MCP-compatible content.libs/sdk/src/tool/tool.instance.ts (10)
1-36: LGTM!The imports comprehensively support the new output parsing functionality, including Zod primitive types and MCP content models. The class signature correctly uses the new generic constraints.
37-55: LGTM!The constructor correctly handles both raw shapes and Zod objects for
inputSchema, and preserves the rawoutputSchemaform for later parsing. This flexibility is essential for the decorator's type inference.
57-77: LGTM!The initialization logic correctly filters and registers hooks. The
getOutputSchema()override properly exposes the raw metadata output schema.
79-99: LGTM!The
create()method correctly instantiates tool contexts for both class-based and function-based tools.
101-127: LGTM!The
parseOutput()override provides a clean entry point that delegates to the helper orchestratorbuildParsedToolResult().
133-173: LGTM with minor note.The
buildParsedToolResult()orchestrator handles three cases well:
- No descriptor → best-effort JSON
- Array descriptor → multiple content items
- Single descriptor → single content item
Note: When the descriptor is an array but the raw value isn't, line 149 wraps it as
[raw]. This is a reasonable defensive fallback for type mismatches, though ideally the decorator's type checking prevents this at compile time.
175-242: LGTM!The
parseSingleValue()helper comprehensively handles all output schema types:
- Primitive literals → text content
- MCP content types → typed content blocks
- Zod primitives → text content
- Structured types → JSON with
structuredContentThe fallback to JSON for unknown types is reasonable and prevents crashes.
244-277: LGTM!The content builder helpers are straightforward and robust:
- Primitive conversion with null handling
- JSON formatting with fallback
- Content array normalization with type checking
The
filter(Boolean)on line 268 correctly removes falsy values.
279-344: Excellent sanitization implementation!The
sanitizeForJson()function is comprehensive and production-ready:
- Handles all non-JSON types (functions, symbols, BigInt, Date, Error, Map, Set)
- Prevents circular reference errors with WeakSet tracking
- Round-trips through JSON for guaranteed safety
- Provides sensible fallback
This ensures
structuredContentis always MCP-compliant.
119-127: LGTM!The
FunctionToolContextwrapper correctly delegates execution to function-based tools.
There was a problem hiding this comment.
Actionable comments posted: 8
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/cache/cache.plugin.ts (1)
112-123: Fix hash collision vulnerability in cache key generation.The current
hashObjectimplementation uses simple string concatenation, which can produce identical hashes for different input objects. For example:
{a: "1", b: "2"}and{a: "12", b: ""}both produce hash"122"{x: "ab", y: "c"}and{x: "a", y: "bc"}both produce hash"abc"This could cause the cache to return incorrect data for different tool inputs, leading to data corruption or unexpected behavior.
Consider using a proper hashing algorithm. Apply this diff to use a delimiter-based approach or a crypto hash:
function hashObject(obj: any) { const keys = Object.keys(obj).sort(); - const values = keys.map((key) => obj[key]); - return values.reduce((acc, val) => { + return keys.reduce((acc, key) => { + const val = obj[key]; + acc += key + ':'; if (typeof val === 'object' && val !== null) { acc += hashObject(val); } else { - acc += val; + acc += String(val) + ';'; } return acc; }, ''); }Or better yet, use a standard hashing library like Node's
cryptomodule:import { createHash } from 'crypto'; function hashObject(obj: any): string { return createHash('sha256') .update(JSON.stringify(obj, Object.keys(obj).sort())) .digest('hex'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CODE_OF_CONDUCT.md(1 hunks)apps/demo/src/apps/expenses/index.ts(1 hunks)apps/demo/src/apps/expenses/tools/add.tool.ts(1 hunks)apps/demo/src/main.ts(1 hunks)libs/cli/project.json(1 hunks)libs/plugins/src/cache/cache.plugin.ts(3 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/common/interfaces/internal/registry.interface.ts(1 hunks)libs/sdk/src/common/interfaces/tool.interface.ts(4 hunks)libs/sdk/src/common/metadata/tool.metadata.ts(5 hunks)libs/sdk/src/tool/flows/call-tool.flow.ts(6 hunks)libs/sdk/src/tool/tool.instance.ts(4 hunks)libs/sdk/src/tool/tool.registry.ts(14 hunks)libs/sdk/src/tool/tool.utils.ts(6 hunks)libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- libs/sdk/src/common/interfaces/internal/registry.interface.ts
- libs/sdk/src/tool/tool.registry.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/common/decorators/tool.decorator.ts
- apps/demo/src/apps/expenses/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/cli/project.jsonlibs/plugins/src/cache/cache.plugin.tslibs/sdk/src/transport/mcp-handlers/call-tool-request.handler.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/interfaces/tool.interface.tslibs/sdk/src/tool/tool.utils.tslibs/sdk/src/tool/tool.instance.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/main.tsapps/demo/src/apps/expenses/tools/add.tool.ts
🧬 Code graph analysis (9)
libs/cli/project.json (1)
libs/cli/src/cli.ts (2)
runInspector(476-479)runCreate(648-715)
libs/plugins/src/cache/cache.plugin.ts (3)
apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts (1)
Plugin(18-87)apps/demo/src/apps/expenses/plugins/authorization.plugin.ts (1)
Plugin(21-48)libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)
libs/sdk/src/tool/flows/call-tool.flow.ts (1)
libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)
apps/demo/src/apps/expenses/tools/add.tool.ts (1)
apps/demo/src/apps/calculator/tools/add.tool.ts (1)
Tool(4-14)
libs/sdk/src/common/entries/tool.entry.ts (2)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)
libs/sdk/src/common/metadata/tool.metadata.ts (1)
libs/sdk/src/common/interfaces/tool.interface.ts (2)
output(103-105)output(107-110)
libs/sdk/src/common/interfaces/tool.interface.ts (2)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)
libs/sdk/src/tool/tool.utils.ts (4)
libs/sdk/src/common/interfaces/tool.interface.ts (1)
ToolType(11-11)libs/sdk/src/common/metadata/tool.metadata.ts (1)
ToolMetadata(138-186)libs/sdk/src/common/tokens/tool.tokens.ts (2)
extendedToolMetadata(19-19)FrontMcpToolTokens(5-17)libs/sdk/src/common/entries/tool.entry.ts (1)
ParsedToolResult(18-18)
libs/sdk/src/tool/tool.instance.ts (5)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/scope/scope.instance.ts (1)
hooks(124-126)libs/sdk/src/common/entries/tool.entry.ts (3)
ToolCallArgs(13-13)ToolCallExtra(14-16)ParsedToolResult(18-18)libs/sdk/src/tool/tool.utils.ts (1)
buildParsedToolResult(185-267)
🪛 ESLint
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts
[error] 10-16: Unnecessary try/catch wrapper.
(no-useless-catch)
libs/sdk/src/common/metadata/tool.metadata.ts
[error] 15-15: An empty interface declaration allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowInterfaces' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
[error] 15-15: An empty interface is equivalent to {}.
(@typescript-eslint/no-empty-interface)
🪛 LanguageTool
CODE_OF_CONDUCT.md
[style] ~25-~25: Try using a synonym here to strengthen your wording.
Context: ...nd - Trolling, insulting, or derogatory comments, and personal or political attacks - Pu...
(COMMENT_REMARK)
🔇 Additional comments (28)
CODE_OF_CONDUCT.md (1)
49-49: Markdown linting issues resolved.All bare URLs and the email address have been properly wrapped in angle brackets, resolving the previous MD034 violations. The formatting now passes markdown linting standards.
Also applies to: 93-93, 100-101
libs/cli/project.json (1)
34-42: All verification checks passed. The Nx target configuration is correct.The CLI properly parses and routes the "inspector" argument to the
runInspector()function. The command is defined in the Command type, documented in help text, handled by a case statement in the argument parser, and correctly invokes the MCP Inspector. The Nx target configuration is sound.apps/demo/src/apps/expenses/tools/add.tool.ts (3)
1-2: LGTM!The imports are correct and include all necessary dependencies.
4-9: LGTM!The tool decorator configuration is correct. The
outputSchemaas a tuple['string', 'number']properly matches the execute method's return type.
11-14: Code is correct; tuple return is intentional design for SDK reference.The implementation correctly matches its
outputSchema: ['string', 'number']and executes properly. Analysis of all 23 demo tools reveals thatexpenses/add.tool.tsis the only tool returning a tuple—all calculator tools return primitives and all employee-time tools return objects. This pattern indicates the tuple return is intentional to demonstrate differentoutputSchemacapabilities to SDK users.libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts (1)
4-6: LGTM! Destructuring improves readability.The inline parameter destructuring makes it immediately clear which properties from
McpHandlerOptionsare used by this handler.apps/demo/src/main.ts (4)
1-4: LGTM! Import formatting normalized.The import formatting changes are consistent with Prettier standards.
7-12: LGTM! Decorator formatting improved.The formatting updates to the
info,logging, andhttpconfiguration objects improve consistency.
19-19: LGTM! Class declaration compacted.The one-line class declaration is cleaner while maintaining the same functionality as a decorator target.
13-17: Configuration is correct and aligns with SDK expectations.The auth configuration matches the SDK's
RemoteAuthOptionsinterface. The fieldstype,name, andbaseUrlare all required and correctly specified. The sample URL format (https://sample-app.frontegg.com) follows the documented pattern for Frontegg integration. For a demo application, this minimal configuration is appropriate—additional OIDC credentials (client ID, secret) can be provided as optional fields or loaded from environment variables.libs/plugins/src/cache/cache.plugin.ts (5)
1-5: LGTM! Imports support the abstraction refactoring.The new imports for
FlowCtxOf,CachePluginOptions, andCacheStoreTokenproperly support the refactored cache implementation using the store abstraction pattern.
78-84: Excellent cache validation logic!The schema validation check using
tool.safeParseOutput(cached)is a robust approach to invalidate stale cache entries when the tool's output schema changes. This prevents serving incorrectly-shaped cached data and maintains data integrity.
97-109: LGTM! Cache write logic is clean and consistent.The cache persistence logic correctly uses the
CacheStoreTokenabstraction and handles TTL resolution consistently with the read path.
58-62: State access pattern is intentional and correct—no action needed.The inconsistency you identified is by design. The
FlowStateAPI exposes two access patterns:
- Direct property access (
flowCtx.state.toolContext) returnsundefinedif the property doesn't exist- Required access (
flowCtx.state.required.toolContext) throws if the property is undefined/nullIn
willReadCache, the Will hook executes before tool execution, sotoolContextandtoolmay not exist yet—hence direct access with validation guards is appropriate. InwillWriteCache, the Did hook executes after execution, guaranteeing these properties exist—hence the required accessor is correct. This pattern is documented in the state type definition and properly leverages the SDK's state management contract.
88-88: No issue found - code is correct.The state object is implemented as a Proxy with a set trap (lines 77-80 in libs/sdk/src/common/interfaces/internal/flow.utils.ts) that explicitly handles direct property assignment:
(data as any)[prop] = value.
rawOutputis defined in the state schema (line 25 in libs/sdk/src/tool/flows/call-tool.flow.ts) as a direct property (rawOutput: z.any().optional()), and the StateAccess type exposes all schema properties for direct access. Both direct assignment (flowCtx.state.rawOutput = cached) and the.set()method work equally—they access the same underlying data object through different code paths.libs/sdk/src/tool/flows/call-tool.flow.ts (1)
60-75: Flow refactor around input parsing and tool context looks consistentUsing
CallToolRequestSchema+ctxinparseInput, resolving tools byfullNameorname, and drivingtoolContextviatool.parseInput(input)andtoolContext.execute(toolContext.input)lines up cleanly with the new metadata/ToolEntry surface. The hook wiring increateToolCallContextalso looks coherent with the class-based tool model.Also applies to: 77-97, 100-116, 134-149, 151-161
libs/sdk/src/common/interfaces/tool.interface.ts (1)
20-27: ToolContext generic surface and ctor wiring look sound; update docs accordinglyThe new
ToolContext<InSchema, OutSchema, In, Out>signature and constructor wiring (includinglogger.child('tool:${this.toolId}'), metadata, and histories) align with the updatedToolMetadataandToolEntrygenerics and should give consumers better typed input/output handling without changing runtime behavior.Given this is part of the public SDK under
libs/sdk, please ensure the developer docs underdocs/draft/docs/**are updated to describe the new generics and theToolCtorArgsshape, rather than editingdocs/docs/**directly.Also applies to: 28-69, 86-112
libs/sdk/src/common/metadata/tool.metadata.ts (2)
70-133: Output schema types and runtime validation now cover primitives and structured outputsThe new
ToolSingleOutputType/ToolOutputTypefamily plustoolSingleOutputSchema/toolOutputSchemacorrectly admit primitive literals ('string' | 'number' | 'date' | 'boolean'), special content types ('image' | 'audio' | 'resource' | 'resource_link'), arbitrary Zod schemas, and raw shapes (record of Zod types), including array forms. This addresses the earlier limitation where only object schemas were accepted and should unblock tools using primitive outputs like'number'.Also applies to: 138-171, 197-221
11-16: Remove "Also applies to: 222-234" reference—it's inaccurateThe code review correctly identifies that line 15's empty interface
ExtendFrontMcpToolMetadataneeds ESLint suppression comments. This pattern is intentional and actively used for declaration merging (verified inlibs/plugins/src/cache/cache.types.ts,apps/demo/src/apps/employee-time/plugins/site-authorization.plugin.ts, andapps/demo/src/apps/expenses/plugins/authorization.plugin.ts).However, lines 222–234 do not contain a second empty interface. Line 233 only uses
ExtendFrontMcpToolMetadatain a type constraint (satisfies RawZodShape<ToolMetadata, ExtendFrontMcpToolMetadata>), not a definition. The proposed fix at line 15 is correct, but the "Also applies to" reference should be removed.Likely an incorrect or invalid review comment.
libs/sdk/src/tool/tool.instance.ts (9)
1-25: LGTM!The new imports are appropriate for the enhanced type system and parsing functionality introduced in this refactor.
37-66: LGTM!The constructor correctly initializes the tool instance with support for both Zod objects and raw shapes, preserves raw schemas as documented, and properly filters hooks for registration.
68-78: LGTM!The new
getOutputSchema()method correctly exposes the raw output schema from metadata for downstream MCP JSON Schema generation.
80-100: LGTM!The
createmethod correctly instantiates tool contexts with proper generic typing based on the tool kind.
102-105: LGTM!The
parseInputmethod correctly validates input arguments against the Zod schema.
106-121: LGTM!The
parseOutputmethod correctly delegates tobuildParsedToolResultto convert raw tool outputs into MCP-compliant results with content blocks and optional structured data.
123-130: LGTM!The
safeParseOutputmethod provides appropriate error handling by wrapping the parsing logic in a try-catch and returning aSafeParseReturnType.
139-146: LGTM!The constructor and execute method are correctly implemented for function-based tool contexts.
27-32: Clarify documentation intent for new ToolInstance methods.The new public methods (
getOutputSchema,parseInput,parseOutput,safeParseOutput) are present in the code but not documented indocs/draft/docs/servers/tools.mdx. However, these appear to be framework/internal utilities (override methods with MCP-specific logic) rather than public API for tool authors. The existing tools.mdx teaches the tool authoring pattern (@tool, tool()), not ToolInstance internals.Please confirm: are these methods intended as public API that tool authors should use and thus require documentation, or are they internal framework utilities?
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
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/cache/cache.plugin.ts (1)
112-123: Critical: Hash function has collision vulnerabilities.The
hashObjectfunction only concatenates values without including their corresponding keys, leading to hash collisions. For example:
{a: "1", b: "2"}and{a: "12"}both hash to"12"{x: 1, y: 2}and{z: 1, w: 2}both hash to"12"This can cause incorrect cache hits where different tool inputs incorrectly match cached results.
Apply this diff to include keys in the hash:
function hashObject(obj: any) { const keys = Object.keys(obj).sort(); - const values = keys.map((key) => obj[key]); - return values.reduce((acc, val) => { + return keys.reduce((acc, key) => { + acc += key + ':'; + const val = obj[key]; if (typeof val === 'object' && val !== null) { acc += hashObject(val); } else { acc += val; } + acc += ';'; return acc; - }, ''); + }, ''); }Alternatively, consider using a well-tested hashing library like
object-hashorfast-json-stable-stringifycombined with a cryptographic hash for production-grade collision resistance.
♻️ Duplicate comments (2)
libs/plugins/src/cache/cache.plugin.ts (1)
73-76: Use strict equality for boolean comparison.Line 73 uses loose equality (
cache == true) which could match truthy values like1or"true". Use strict equality (cache === true) to ensure type-safe comparison with boolean values.Apply this diff:
- if (cache === true || (cache.ttl && cache.slideWindow)) { + if (cache === true || (cache.ttl && cache.slideWindow)) {libs/sdk/src/tool/flows/call-tool.flow.ts (1)
231-262: Re‑evaluate hard failure whenrawOutput === undefinedinfinalizeThe
finalizestage currently throwsToolExecutionErrorifrawOutputisundefined(Lines 241–244). Combined with thetoolpresence guard, this can:
- Break side‑effect‑only tools that intentionally return
undefined, and- Potentially mask earlier, more specific failures if
finalizeruns on an error path.A softer approach would be to either:
- Allow
undefinedto flow intotool.safeParseOutput(rawOutput)and let that logic (andInvalidOutputError) decide validity, or- Only enforce a non‑undefined
rawOutputwhen you knowexecutecompleted successfully (e.g., via an explicit success flag in state).The earlier bot review on this file already raised this point; I’d align with that guidance here.
🧹 Nitpick comments (18)
libs/json-schema-to-zod-v3/eslint.config.mjs (1)
1-22: Prefer static import (or.default) forjsonc-eslint-parserinstead of rawawait importUsing
languageOptions.parser: await import('jsonc-eslint-parser')will assign the module namespace object toparser, whereas ESLint expects the actual parser export. This may work or fail depending on the module format and interop, and also relies on top‑levelawaitsupport in the ESLint/runtime combo.To make this robust and clearer, I’d recommend a static default import (or at least
.defaulton the dynamic import):-import baseConfig from '../../eslint.config.mjs'; +import baseConfig from '../../eslint.config.mjs'; +import jsoncParser from 'jsonc-eslint-parser'; export default [ ...baseConfig, { files: ['**/*.json'], rules: { '@nx/dependency-checks': [ 'error', { ignoredFiles: ['{projectRoot}/eslint.config.{js,cjs,mjs,ts,cts,mts}'], }, ], }, languageOptions: { - parser: await import('jsonc-eslint-parser'), + parser: jsoncParser, }, }, { ignores: ['**/out-tsc'], }, ];If you really want to keep the dynamic import, at minimum:
- parser: await import('jsonc-eslint-parser'), + parser: (await import('jsonc-eslint-parser')).default,libs/sdk/src/common/types/options/logging.options.ts (1)
35-42: ClarifyLoggingConfigTypesemantics and docs alignmentDeriving
LoggingConfigTypefromz.infer<typeof loggingOptionsSchema>(and omittingtransports) means:
levelandenableConsoleare required at the type level (since they have defaults).prefixremains optional.- Callers can’t set
transportsonLoggingConfigType, only onLoggingOptions.This looks intentional and consistent, but it slightly differs from the looser
LoggingOptionsshape. Consider adding a short doc comment toLoggingConfigTypeexplaining that it represents the normalized config (after schema defaults), and please ensure the public docs underdocs/draft/docs/**reflect the newLogLevelvalues and theprefix/enableConsolebehavior. As per coding guidelines.libs/sdk/src/transport/adapters/transport.sse.adapter.ts (1)
50-60: Tighten Zod typing instead of casting requestedSchema toanyCasting
requestedSchematoanyhere works but throws away the type guarantees you set up withT extends ZodObject<any>, and theZodObjectimport fromnode_modules/zod/v3/types.cjsis brittle for a published SDK.If zod-to-json-schema in this repo is wired against Zod v3 via the regular
zodentry, a more robust pattern would be:- import { ZodObject } from 'node_modules/zod/v3/types.cjs'; + import type { ZodObject } from 'zod'; async sendElicitRequest<T extends ZodObject<any>>( relatedRequestId: RequestId, message: string, requestedSchema: T, ): Promise<TypedElicitResult<T>> { await this.transport.send( rpcRequest(this.newRequestId, 'elicitation/create', { message, - requestedSchema: zodToJsonSchema(requestedSchema as any), + requestedSchema: zodToJsonSchema(requestedSchema), }), ); }This keeps the call sites type‑safe and avoids hard‑coding a
node_modules/...path into the published bundle. Iftscor zod-to-json-schema typings still complain, consider isolating theanycast in a small internal helper instead of repeating it at adapters.libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
52-61: Avoid repeatingas anycasts around zodToJsonSchemaSimilar to the SSE adapter, this
requestedSchema as anyweakens the otherwise good typing onT extends ZodObject<any>and duplicates an unsafe pattern in multiple adapters.If zod-to-json-schema’s typings allow it, prefer:
- requestedSchema: zodToJsonSchema(requestedSchema as any), + requestedSchema: zodToJsonSchema(requestedSchema),Otherwise, consider a small internal utility (e.g.
toJsonSchema(schema: ZodObject<any>)) that performs the cast once in one place and keeps adapter code strongly typed.libs/json-schema-to-zod-v3/README.md (1)
437-455: Minor README polish: fenced code language and version labelTwo small doc tweaks will help keep things tidy:
- The architecture tree block is missing a fenced language, which trips markdownlint; you can mark it as plain text:
-``` +```text src/ ├── index.ts # Public API ... -``` +```
- The changelog currently labels the initial release as
1.0.0, while other parts of the repo (e.g. dependencies in libs/adapters/package.json) point atjson-schema-to-zod-v3version0.3.1. It’s worth aligning the README’s version naming with whatever you actually publish to npm to avoid confusion.Also applies to: 541-545
libs/sdk/src/errors/ERROR_HANDLING.md (1)
18-34: Add a language to the architecture code fence for markdownlintThe error-class hierarchy diagram is inside a raw fenced block:
McpError (abstract)
├── PublicMcpError ...
...Adding a language (e.g.
text) will satisfy MD040 and make the intent clearer:-``` +```text McpError (abstract) ... -``` +```The rest of the document aligns well with the current
McpErrorAPI and formatting behavior.libs/sdk/src/errors/error-handler.ts (1)
28-114: ErrorHandler implementation is solid; consider a few small refinementsThe overall design (centralized formatting, optional transformer, logging, and shouldStopExecution) looks good and matches the docs. A few details you may want to tighten:
- Prefer direct property access over bracket notation in the constructor for clarity and type safety:
- constructor(options: ErrorHandlerOptions = {}) { - this.isDevelopment = options.isDevelopment ?? process.env['NODE_ENV'] !== 'production'; - this['logger'] = options.logger; - this.errorTransformer = options.errorTransformer; - } + constructor(options: ErrorHandlerOptions = {}) { + this.isDevelopment = options.isDevelopment ?? process.env['NODE_ENV'] !== 'production'; + this.logger = options.logger; + this.errorTransformer = options.errorTransformer; + }
- Since this file already imports from
./mcp.error, using a dynamicrequirefortoMcpErroris a bit awkward and untyped. If there’s no circular dependency, a static import keeps things cleaner:-import { McpError, formatMcpErrorResponse } from './mcp.error'; +import { McpError, formatMcpErrorResponse, toMcpError } from './mcp.error'; ... - // Default transformation - const { toMcpError } = require('./mcp.error'); - return toMcpError(error); + // Default transformation + return toMcpError(error);
wrapcurrently drops the originalthiswhen callingfn, which can break unbound instance methods. You can preservethisand be a bit more robust for async functions by switching to a non‑arrow wrapper and checking for thenables:- wrap<T extends (...args: any[]) => any>(fn: T): T { - return ((...args: any[]) => { - try { - const result = fn(...args); - if (result instanceof Promise) { - return result.catch((error) => { - throw this.transformToMcpError(error); - }); - } - return result; - } catch (error) { - throw this.transformToMcpError(error); - } - }) as T; - } + wrap<T extends (...args: any[]) => any>(fn: T): T { + return (function (this: any, ...args: any[]) { + try { + const result = fn.apply(this, args); + if (result && typeof (result as any).then === 'function') { + return (result as Promise<unknown>).catch((error) => { + throw this.transformToMcpError(error); + }); + } + return result; + } catch (error) { + throw this.transformToMcpError(error); + } + }).bind(this) as T; + }These are all non‑breaking cleanups; the current behavior is already coherent with the documented error handling model.
libs/json-schema-to-zod-v3/src/handlers/refinement/special.ts (2)
7-7: Avoid shadowing globalhasOwnPropertyby aliasing the importBiome is flagging the imported
hasOwnPropertyhelper because it shares a restricted global name, which can be confusing. You can keep using the helper but alias it at import and call sites.Suggested change:
-import { deepEqual, hasOwnProperty } from '../../utils'; +import { deepEqual, hasOwnProperty as hasOwn } from '../../utils'; @@ - return required.every((prop) => hasOwnProperty(value, prop)); + return required.every((prop) => hasOwn(value, prop));Please re-run the linter/biome after this change to confirm the warning disappears.
Also applies to: 108-114
90-98: ProtoRequiredHandler drops all existing schema constraints
ProtoRequiredHandler.applyreplaces the incomingzodSchemawithz.any().refine(...), so any previous constraints (type narrowing, property shapes, additionalProperties, etc.) are ignored for schemas that hit this branch. Also,validateRequiredreturnstruefor non-object values and arrays, meaning those values are accepted even if the original JSON Schema might have rejected them.If the intent is to augment the existing schema with safe
__proto__handling rather than fully replace it, consider refining or intersecting with the originalzodSchemainstead:- return z - .any() - .refine((value) => this.validateRequired(value, schema.required!), { message: 'Missing required properties' }); + return zodSchema.refine( + (value) => this.validateRequired(value, schema.required!), + { message: 'Missing required properties' }, + );If you do intend to override all other constraints for this edge case, a short comment explaining that trade-off would help future readers.
Also applies to: 108-114
package.json (1)
19-19: Consider using caret prefix for json-schema-to-zod-v3 version.The version is pinned to exactly
0.3.1without a caret prefix, while other dependencies use caret ranges. For internal packages or when you need strict version control this is appropriate, but if this is an external dependency, consider^0.3.1to allow patch updates.libs/adapters/src/openapi/openapi.tool.ts (2)
19-19: Use const for queryParams - it's never reassigned.After destructuring from
prepareUrl,queryParamsis only read (line 42) but never reassigned.Apply this diff:
- let { urlPath, headers, queryParams } = prepareUrl(oTool, input); + const { urlPath, headers, queryParams } = prepareUrl(oTool, input); let requestBodyData: any = undefined;Note: Keep
urlPathandheadersas destructured from the object. TheurlPathis not reassigned in this scope (it's only modified insideprepareUrl).Based on static analysis hints.
50-50: Use const for result - it's never reassigned.The
resultvariable is initialized and then only its propertyresult.datais modified, butresultitself is never reassigned.Apply this diff:
- let result = { data }; + const result = { data };Based on static analysis hints.
libs/json-schema-to-zod-v3/src/converter.ts (1)
60-94: Primitive handler ordering for arrays matches intent; minor doc mismatch elsewhereThe
primitiveHandlersordering here (TupleHandler/ItemsHandler before MinItems/MaxItems) correctly matches the comment that base array types are established before min/max constraints are applied.Note that
ItemsHandler’s own inline comment (inhandlers/primitive/array.ts, around Lines 114–116) still says Min/Max run before it, which now contradicts this definition. Consider updating that comment to avoid confusion for future maintainers.libs/json-schema-to-zod-v3/src/handlers/refinement/complex.ts (2)
5-9: Prefer a relative import ofconvertJsonSchemaToZodover importing the package name from inside srcHere you import from the published package name:
import { convertJsonSchemaToZod } from 'json-schema-to-zod-v3';Inside the library’s own
srctree this can be brittle:
- It relies on TS path mapping or package resolution being configured for internal builds/tests, and
- It may create a more complex circular dependency via
index.tsthan a direct relative import fromconverter.ts.For consistency with
converter.tsand to keep the dependency graph simpler, I’d suggest switching to a relative import:-import { convertJsonSchemaToZod } from 'json-schema-to-zod-v3'; +import { convertJsonSchemaToZod } from '../../converter';Behavior remains the same, but the module linkage is clearer and less dependent on external tooling configuration.
153-253: ObjectPropertiesHandler:additionalPropertiesobject schemas are currently treated as “allow anything”The
ObjectPropertiesHandlercorrectly handlesadditionalProperties: falseby producing a closedz.object(shape), but any non‑false value (including an object schema) goes through theelsebranch as.passthrough().That means JSON Schemas like:
{ "type": "object", "properties": { "known": { "type": "string" } }, "additionalProperties": { "type": "number" } }will allow arbitrary extra keys of any type, not just numbers. If you intend to support the “additionalProperties as schema” pattern, a future enhancement could either:
- Build a
ZodRecordfor unknown keys usingconvertJsonSchemaToZod(additionalProperties), or- Add a refinement path that validates extra props against that schema.
Not a blocker, but worth documenting as an intentional limitation if you don’t plan to implement it yet.
libs/sdk/src/errors/mcp.error.ts (1)
28-37: GuardError.captureStackTracefor non‑Node runtimesIf this SDK is ever used in browser/edge runtimes,
Error.captureStackTracemay be undefined, which will throw at construction time. Guarding the call keeps the API portable without changing behavior on Node.constructor(message: string, errorId?: string) { super(message); this.name = this.constructor.name; this.errorId = errorId || this.generateErrorId(); - Error.captureStackTrace(this, this.constructor); + const anyError = Error as any; + if (typeof anyError.captureStackTrace === 'function') { + anyError.captureStackTrace(this, this.constructor); + } }libs/sdk/src/tool/tool.instance.ts (2)
80-100:createimplementation aligns with kind‑specific contextsThe factory logic for
ToolKind.CLASS_TOKENvsToolKind.FUNCTION, and the reuse ofToolCtorArgs<In>for both, looks sound and matches theFunctionToolContextsignature.You might optionally consider adding an
assertUnreachable/default branch to guard futureToolKindadditions so that an unsupported kind fails fast rather than returningundefined.switch (this.record.kind) { case ToolKind.CLASS_TOKEN: return new this.record.provide(toolCtorArgs); case ToolKind.FUNCTION: return new FunctionToolContext<InSchema, OutSchema, In, Out>(this.record, toolCtorArgs); + default: { + // Helps catch future enum extensions at runtime + throw new Error(`Unsupported tool kind: ${String(this.record.kind)}`); + } }
117-121: AlignsafeParseOutputtyping with actual behavior
safeParseOutputcurrently returnsz.SafeParseReturnType<Out, ParsedToolResult>, but the implementation:
- Never uses Zod to validate
raw.- Catches any thrown error from
buildParsedToolResultand returns it aserror, which may not be aZodError<Out>.This means the static type suggests Zod‑style errors while callers may see arbitrary errors, which can be surprising.
Consider defining a simpler, honest result type instead of reusing Zod’s:
- override safeParseOutput(raw: Out | Partial<Out> | any): z.SafeParseReturnType<Out, ParsedToolResult> { - const descriptor = this.outputSchema as any; - try { - return { success: true, data: buildParsedToolResult(descriptor, raw) }; - } catch (error: any) { - return { success: false, error }; - } - } + override safeParseOutput( + raw: Out | Partial<Out> | any, + ): { success: true; data: ParsedToolResult } | { success: false; error: unknown } { + const descriptor = this.outputSchema as any; + try { + return { success: true, data: buildParsedToolResult(descriptor, raw) }; + } catch (error) { + return { success: false, error }; + } + }This avoids promising a
ZodErrorshape while still giving consumers a familiarsuccess/errorpattern.Also applies to: 123-129
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (44)
apps/demo/src/apps/expenses/tools/create-expense.tool.ts(2 hunks)libs/adapters/package.json(1 hunks)libs/adapters/src/openapi/openapi.tool.ts(6 hunks)libs/cli/src/cli.ts(20 hunks)libs/json-schema-to-zod-v3/.spec.swcrc(1 hunks)libs/json-schema-to-zod-v3/README.md(1 hunks)libs/json-schema-to-zod-v3/eslint.config.mjs(1 hunks)libs/json-schema-to-zod-v3/examples.ts(1 hunks)libs/json-schema-to-zod-v3/jest.config.ts(1 hunks)libs/json-schema-to-zod-v3/package.json(1 hunks)libs/json-schema-to-zod-v3/project.json(1 hunks)libs/json-schema-to-zod-v3/src/converter.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/basic.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/number.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/object.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/refinement/complex.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/refinement/special.ts(1 hunks)libs/json-schema-to-zod-v3/src/index.ts(1 hunks)libs/json-schema-to-zod-v3/src/types.ts(1 hunks)libs/json-schema-to-zod-v3/src/utils.ts(1 hunks)libs/json-schema-to-zod-v3/tsconfig.json(1 hunks)libs/json-schema-to-zod-v3/tsconfig.lib.json(1 hunks)libs/json-schema-to-zod-v3/tsconfig.spec.json(1 hunks)libs/plugins/src/cache/cache.plugin.ts(3 hunks)libs/sdk/package.json(1 hunks)libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/common/metadata/prompt.metadata.ts(2 hunks)libs/sdk/src/common/types/options/logging.options.ts(3 hunks)libs/sdk/src/errors/ERROR_HANDLING.md(1 hunks)libs/sdk/src/errors/__tests__/mcp.error.test.ts(1 hunks)libs/sdk/src/errors/error-handler.ts(1 hunks)libs/sdk/src/errors/index.ts(1 hunks)libs/sdk/src/errors/mcp.error.ts(1 hunks)libs/sdk/src/tool/flows/call-tool.flow.ts(5 hunks)libs/sdk/src/tool/flows/tools-list.flow.ts(5 hunks)libs/sdk/src/tool/tool.instance.ts(4 hunks)libs/sdk/src/transport/adapters/transport.sse.adapter.ts(1 hunks)libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts(1 hunks)libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts(1 hunks)package.json(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- tsconfig.base.json
- libs/json-schema-to-zod-v3/tsconfig.json
- libs/json-schema-to-zod-v3/examples.ts
- libs/json-schema-to-zod-v3/tsconfig.lib.json
- libs/json-schema-to-zod-v3/tsconfig.spec.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/demo/src/apps/expenses/tools/create-expense.tool.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/json-schema-to-zod-v3/jest.config.tslibs/sdk/src/errors/index.tslibs/json-schema-to-zod-v3/package.jsonlibs/sdk/src/errors/error-handler.tslibs/sdk/src/errors/__tests__/mcp.error.test.tslibs/json-schema-to-zod-v3/src/utils.tslibs/adapters/package.jsonlibs/json-schema-to-zod-v3/src/handlers/primitive/number.tslibs/json-schema-to-zod-v3/src/handlers/primitive/string.tslibs/json-schema-to-zod-v3/src/converter.tslibs/json-schema-to-zod-v3/src/handlers/primitive/object.tslibs/sdk/src/transport/mcp-handlers/call-tool-request.handler.tslibs/sdk/src/common/metadata/prompt.metadata.tslibs/json-schema-to-zod-v3/eslint.config.mjslibs/sdk/src/transport/adapters/transport.streamable-http.adapter.tslibs/json-schema-to-zod-v3/src/handlers/primitive/basic.tslibs/json-schema-to-zod-v3/project.jsonlibs/json-schema-to-zod-v3/src/handlers/refinement/complex.tslibs/json-schema-to-zod-v3/src/index.tslibs/plugins/src/cache/cache.plugin.tslibs/json-schema-to-zod-v3/src/handlers/refinement/composition.tslibs/json-schema-to-zod-v3/src/handlers/refinement/special.tslibs/cli/src/cli.tslibs/sdk/src/errors/mcp.error.tslibs/sdk/package.jsonlibs/sdk/src/tool/flows/tools-list.flow.tslibs/json-schema-to-zod-v3/src/types.tslibs/sdk/src/transport/adapters/transport.sse.adapter.tslibs/json-schema-to-zod-v3/src/handlers/primitive/array.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/common/types/options/logging.options.tslibs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/tool/tool.instance.tslibs/adapters/src/openapi/openapi.tool.tslibs/json-schema-to-zod-v3/README.mdlibs/sdk/src/errors/ERROR_HANDLING.md
🧬 Code graph analysis (21)
libs/sdk/src/errors/error-handler.ts (1)
libs/sdk/src/errors/mcp.error.ts (3)
formatMcpErrorResponse(278-281)toMcpError(54-81)toMcpError(263-273)
libs/sdk/src/errors/__tests__/mcp.error.test.ts (1)
libs/sdk/src/errors/mcp.error.ts (6)
ToolNotFoundError(130-134)InvalidInputError(139-156)InvalidOutputError(161-172)ToolExecutionError(186-200)isPublicError(256-258)formatMcpErrorResponse(278-281)
libs/json-schema-to-zod-v3/src/utils.ts (1)
libs/json-schema-to-zod-v3/src/index.ts (3)
deepEqual(47-47)createUniqueItemsValidator(47-47)isValidWithSchema(47-47)
libs/json-schema-to-zod-v3/src/handlers/primitive/number.ts (1)
libs/json-schema-to-zod-v3/src/types.ts (3)
SchemaHandler(88-95)TypeRegistry(75-83)JSONSchemaObject(15-65)
libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts (1)
libs/json-schema-to-zod-v3/src/types.ts (3)
SchemaHandler(88-95)TypeRegistry(75-83)JSONSchemaObject(15-65)
libs/json-schema-to-zod-v3/src/converter.ts (9)
libs/json-schema-to-zod-v3/src/types.ts (4)
SchemaHandler(88-95)RefinementHandler(100-108)JSONSchema(10-10)TypeRegistry(75-83)libs/json-schema-to-zod-v3/src/handlers/primitive/basic.ts (3)
ConstHandler(49-85)EnumHandler(91-159)TypeHandler(13-43)libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts (4)
ImplicitStringHandler(12-30)MinLengthHandler(36-60)MaxLengthHandler(66-90)PatternHandler(96-114)libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts (5)
ImplicitArrayHandler(15-36)TupleHandler(146-192)ItemsHandler(86-140)MinItemsHandler(42-58)MaxItemsHandler(64-80)libs/json-schema-to-zod-v3/src/handlers/primitive/object.ts (4)
ImplicitObjectHandler(35-52)MaxPropertiesHandler(58-79)MinPropertiesHandler(85-106)PropertiesHandler(12-29)libs/json-schema-to-zod-v3/src/handlers/primitive/number.ts (5)
MinimumHandler(12-31)MaximumHandler(37-56)ExclusiveMinimumHandler(62-87)ExclusiveMaximumHandler(93-118)MultipleOfHandler(125-163)libs/json-schema-to-zod-v3/src/handlers/refinement/special.ts (5)
ProtoRequiredHandler(80-115)EnumComplexHandler(44-74)ConstComplexHandler(13-38)DefaultHandler(121-146)MetadataHandler(152-170)libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (4)
AllOfHandler(17-43)AnyOfHandler(49-83)OneOfHandler(89-131)NotHandler(137-160)libs/json-schema-to-zod-v3/src/handlers/refinement/complex.ts (4)
PrefixItemsHandler(41-102)ObjectPropertiesHandler(157-254)ContainsHandler(108-151)UniqueItemsHandler(14-35)
libs/json-schema-to-zod-v3/src/handlers/primitive/object.ts (1)
libs/json-schema-to-zod-v3/src/types.ts (3)
SchemaHandler(88-95)TypeRegistry(75-83)JSONSchemaObject(15-65)
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts (1)
libs/sdk/src/errors/mcp.error.ts (1)
formatMcpErrorResponse(278-281)
libs/json-schema-to-zod-v3/src/handlers/primitive/basic.ts (1)
libs/json-schema-to-zod-v3/src/types.ts (3)
SchemaHandler(88-95)TypeRegistry(75-83)JSONSchemaObject(15-65)
libs/json-schema-to-zod-v3/src/handlers/refinement/complex.ts (3)
libs/json-schema-to-zod-v3/src/types.ts (2)
RefinementHandler(100-108)JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/utils.ts (3)
createUniqueItemsValidator(51-67)isValidWithSchema(81-83)propertyExists(124-126)libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)
libs/json-schema-to-zod-v3/src/index.ts (2)
libs/json-schema-to-zod-v3/src/types.ts (1)
JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)
libs/plugins/src/cache/cache.plugin.ts (2)
libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)apps/demo/src/apps/expenses/provders/redis.provider.ts (1)
RedisProvider(12-66)
libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (3)
libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)libs/json-schema-to-zod-v3/src/types.ts (2)
RefinementHandler(100-108)JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/utils.ts (1)
isValidWithSchema(81-83)
libs/json-schema-to-zod-v3/src/handlers/refinement/special.ts (2)
libs/json-schema-to-zod-v3/src/types.ts (2)
RefinementHandler(100-108)JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/utils.ts (2)
deepEqual(19-37)hasOwnProperty(112-114)
libs/sdk/src/errors/mcp.error.ts (1)
libs/sdk/src/errors/index.ts (15)
McpError(3-3)PublicMcpError(4-4)InternalMcpError(5-5)ToolNotFoundError(6-6)InvalidInputError(7-7)InvalidOutputError(8-8)InvalidMethodError(9-9)ToolExecutionError(10-10)RateLimitError(11-11)QuotaExceededError(12-12)UnauthorizedError(13-13)GenericServerError(14-14)isPublicError(15-15)toMcpError(16-16)formatMcpErrorResponse(17-17)
libs/sdk/src/tool/flows/tools-list.flow.ts (2)
libs/sdk/src/errors/mcp.error.ts (2)
InvalidInputError(139-156)InvalidMethodError(177-181)libs/sdk/src/scope/scope.instance.ts (2)
apps(136-138)tools(140-142)
libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts (2)
libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)libs/json-schema-to-zod-v3/src/types.ts (3)
SchemaHandler(88-95)TypeRegistry(75-83)JSONSchemaObject(15-65)
libs/sdk/src/tool/flows/call-tool.flow.ts (2)
libs/sdk/src/errors/mcp.error.ts (5)
InvalidInputError(139-156)InvalidMethodError(177-181)ToolNotFoundError(130-134)ToolExecutionError(186-200)InvalidOutputError(161-172)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)
libs/sdk/src/common/entries/tool.entry.ts (6)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
AdminAddEntryTool(19-37)apps/demo/src/apps/expenses/tools/add.tool.ts (1)
AddTool(10-16)libs/adapters/src/openapi/openapi.adapter.ts (1)
tool(48-50)
libs/sdk/src/tool/tool.instance.ts (6)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/scope/scope.instance.ts (1)
hooks(124-126)libs/sdk/src/common/interfaces/tool.interface.ts (3)
input(86-88)input(90-97)ToolCtorArgs(20-26)libs/sdk/src/common/entries/tool.entry.ts (3)
ToolCallArgs(13-13)ToolCallExtra(14-16)ParsedToolResult(18-18)libs/sdk/src/tool/tool.utils.ts (1)
buildParsedToolResult(185-267)
libs/adapters/src/openapi/openapi.tool.ts (2)
libs/adapters/src/openapi/openapi.types.ts (1)
OpenApiAdapterOptions(65-65)libs/adapters/src/openapi/openapi.adapter.ts (1)
fetch(24-45)
🪛 ast-grep (0.40.0)
libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts
[warning] 109-109: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(schema.pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
libs/json-schema-to-zod-v3/src/utils.ts
[error] 112-112: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
libs/json-schema-to-zod-v3/src/handlers/refinement/special.ts
[error] 7-7: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 ESLint
libs/sdk/src/errors/mcp.error.ts
[error] 54-54: Type boolean trivially inferred from a boolean literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 93-93: Type string trivially inferred from a string literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 93-93: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 113-113: Type string trivially inferred from a string literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 142-142: Type string trivially inferred from a string literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 216-216: Type string trivially inferred from a string literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
[error] 225-225: Type string trivially inferred from a string literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
libs/adapters/src/openapi/openapi.tool.ts
[error] 19-19: 'urlPath' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 19-19: 'queryParams' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 50-50: 'result' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 LanguageTool
libs/json-schema-to-zod-v3/README.md
[style] ~521-~521: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## Support If ...
(FEEL_FREE_TO_STYLE_ME)
🪛 markdownlint-cli2 (0.18.1)
libs/json-schema-to-zod-v3/README.md
437-437: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
libs/sdk/src/errors/ERROR_HANDLING.md
437-437: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (59)
libs/sdk/src/common/metadata/prompt.metadata.ts (2)
63-63: LGTM: Documentation clarity improved.The wording changes improve readability and grammatical correctness ("Clients can use this" is more direct, and "as a 'hint'" is grammatically preferable to "like a 'hint'").
88-90: LGTM: Export formatting normalized.Minor formatting fix to ensure consistent export block closure.
libs/sdk/src/common/types/options/logging.options.ts (2)
1-4: Imports and RawZodShape usage look consistentThe imports and use of
RawZodShapeforloggingOptionsSchemaare coherent with the surrounding types and ensure the schema stays aligned withLoggingOptions. No issues from this change.
24-33: Confirmtransportsschema type matchesLogTransportType[]
LoggingOptions.transportsis now declared asLogTransportType[], while the schema usesz.array(annotatedFrontMcpLoggerSchema). This is fine as long asz.infer<typeof annotatedFrontMcpLoggerSchema>is assignable toLogTransportType(and vice versa for practical usage).If these ever drift apart,
RawZodShape<LoggingOptions>may stop compiling or the runtime shape may differ from the declared options type. Worth double‑checking thatLogTransportTypeis effectively the inferred type ofannotatedFrontMcpLoggerSchema.libs/cli/src/cli.ts (5)
7-11: LGTM! Import additions are appropriate.The added imports (
promises as fsp,ChildProcess,getSelfVersion) are used correctly throughout the file and follow standard Node.js patterns.
438-438: Good fix: avoids variable shadowing.Renaming the destructured property from
oktooksprevents shadowing the outerokvariable (line 404) that tracks overall doctor check status. This improves code clarity.
69-69: LGTM! Formatting changes are consistent.The formatting changes throughout the file (spacing around braces, object literal alignment, etc.) are consistent with Prettier formatting standards and do not affect functionality.
Also applies to: 147-149, 156-156, 220-220, 318-328, 331-332, 361-367, 376-376, 386-386, 417-417, 524-524, 636-636, 646-646, 658-660, 714-714
520-520: LGTM! Console messages are clear.The updated console messages accurately describe the package.json operations and provide helpful feedback to users.
Also applies to: 566-566
507-507: Zod version 3.25.76 is valid.The version exists on npm and is a legitimate release in the 3.25.x sequence. No changes are required.
libs/plugins/src/cache/cache.plugin.ts (4)
1-5: LGTM!The imports correctly reflect the new CacheStoreToken abstraction and associated types for the refactored cache storage backend.
43-54: LGTM!The public
optionsproperty and staticdefaultOptionsprovide a clean API for configuration. The default TTL of 24 hours is reasonable for cache expiration.
97-109: LGTM!The write-through caching logic correctly uses the CacheStoreToken abstraction and handles TTL appropriately. Using
flowCtx.state.required.toolContextis safe here since the execute hook always has this context.
78-84: Code is correct—safeParseOutputis always available.Verification confirms that
safeParseOutputis an abstract method in theToolbase class (libs/sdk/src/common/entries/tool.entry.tsline 70), with a concrete implementation inToolInstance(libs/sdk/src/tool/tool.instance.tsline 123). Since it's abstract, all tool implementations must provide it. The return type is Zod'sSafeParseReturnType, a discriminated union where.erroris safely accessed—undefined when parsing succeeds (falsy) and a ZodError when parsing fails (truthy). The defensive check in the cache plugin is well-implemented.libs/json-schema-to-zod-v3/.spec.swcrc (1)
1-22: SWC test config looks consistent and safeThe SWC options (TS syntax, ES2017 target, decorators, ES module output, source maps) are standard for Jest + SWC in this repo and shouldn’t introduce surprises.
libs/adapters/package.json (1)
34-39: Verify json-schema-to-zod-v3 version alignmentAdding
"json-schema-to-zod-v3": "0.3.1"here makes sense, but please double‑check that the json-schema-to-zod-v3 package you publish (libs/json-schema-to-zod-v3/package.json) uses the same version to avoid accidental dual versions or confusing docs/changelogs.libs/json-schema-to-zod-v3/jest.config.ts (1)
1-21: Jest + SWC configuration looks appropriateLoading
.spec.swcrcand passing it explicitly to@swc/jestwhile disablingswcrclookup is a clean setup for this lib’s TS tests; no issues from an SDK/public-surface perspective.libs/sdk/package.json (1)
37-49: SDK dependency wiring for Zod and json-schema-to-zod-v3 looks consistentDeclaring
zodas a peer dependency and addingjson-schema-to-zod-v3+ updatedzod-to-json-schemaaligns with the new schema-conversion flow. No issues from the SDK packaging perspective.Please double-check that:
- The published SDK README / docs mention
zodas a required peer dependency.- The installed Zod version in consuming projects satisfies both this peer range and
json-schema-to-zod-v3’s expectations.libs/json-schema-to-zod-v3/package.json (1)
1-51: Newjson-schema-to-zod-v3package.json looks structurally correctManifest (name/version, main/types/exports) is consistent with the Nx build output and with how other libs in this repo are packaged. The Zod peer range
^3.0.0is compatible with the SDK’s stricter^3.25.76.When you cut a release, please verify that:
nx build json-schema-to-zod-v3producesdist/src/index.{js,d.ts}andnpm publishuses this manifest as intended.- The published package correctly declares Zod as a peer (no duplicate Zod installs in a sample consumer).
libs/json-schema-to-zod-v3/project.json (1)
1-32: Nx build/publish targets forjson-schema-to-zod-v3look aligned with the package layoutThe
build-tsc,build, andpublishtargets are consistent with the package.json paths (./dist/src/index.js) and the intended dist directory.Before publishing, please run the
build-tsc+buildtargets and confirm:
distcontains the compiledsrctree and the strippedpackage.json.npm publish libs/json-schema-to-zod-v3/dist ...works as expected from a clean checkout.libs/json-schema-to-zod-v3/src/handlers/primitive/basic.ts (2)
1-43: LGTM! TypeHandler correctly manages type constraints.The implementation properly disables types not in the allowed set and applies integer constraints to numbers when needed.
45-85: LGTM! ConstHandler correctly handles literal values.The handler appropriately creates literal schemas for primitive types and sets complex types (arrays/objects) to
undefinedto delegate handling to refinement logic.libs/sdk/src/errors/index.ts (1)
1-21: LGTM! Clean error module public API.The exports are well-organized and provide a consolidated entry point for all error-related functionality.
libs/sdk/src/errors/__tests__/mcp.error.test.ts (1)
1-216: LGTM! Comprehensive test coverage for MCP error handling.The test suite thoroughly covers:
- Public vs internal error behavior
- Error formatting in development vs production
- Error metadata and ID generation
- Error utilities and inheritance
- Environment-based behavior
The tests properly restore environment variables and validate all critical error handling paths.
libs/json-schema-to-zod-v3/src/handlers/primitive/number.ts (5)
8-31: LGTM! Minimum constraint correctly applied.The handler appropriately applies the inclusive lower bound to number types.
33-56: LGTM! Maximum constraint correctly applied.The handler appropriately applies the inclusive upper bound to number types.
58-87: LGTM! Exclusive minimum properly handles both forms.The handler correctly supports numeric values and gracefully disables the number type for unsupported boolean forms from older JSON Schema drafts.
89-118: LGTM! Exclusive maximum properly handles both forms.The handler correctly supports numeric values and gracefully disables the number type for unsupported boolean forms from older JSON Schema drafts.
120-163: LGTM! MultipleOf handler uses robust floating-point-safe validation.The tolerance-based approach correctly handles floating-point precision issues, with appropriate edge case handling for zero values.
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts (1)
10-17: LGTM! Proper error handling with MCP error formatting.The try/catch block now serves a clear purpose by catching errors and formatting them as MCP error responses. The error logging via
scope.logger.erroris appropriate for an SDK library.This resolves the previous review concerns about the useless try/catch wrapper and console.log.
libs/json-schema-to-zod-v3/src/index.ts (1)
1-117: LGTM! Well-documented public API with correct implementation.The
jsonSchemaObjectToZodRawShapefunction properly:
- Converts JSON Schema properties to Zod schemas
- Handles required vs optional fields correctly
- Provides comprehensive documentation with examples
The re-exports create a clean, well-organized public API surface.
libs/adapters/src/openapi/openapi.tool.ts (2)
1-5: LGTM! Updated import aligns with new json-schema-to-zod-v3 library.The import path change is consistent with the broader architectural shift documented in package.json.
15-15: LGTM! Type-safe property access improvement.Using
inputSchema.shapeinstead ofinputSchema as anyprovides better type safety.package.json (1)
11-11: Based on my verification, I've gathered comprehensive information about the SDK upgrade and its impact. Let me search for more specific migration guidance:Verify @modelcontextprotocol/sdk 1.22.0 compatibility before merging.
Breaking changes confirmed in 1.22.0: SEP-1319 (request payload decoupling, passthrough iteration removal) and additional schema/type changes. The codebase imports types and schemas from the SDK across multiple files. SSE transport is deprecated in favor of Streamable HTTP, and while the codebase uses both, the legacy SSEServerTransport should be phased out.
Run the project's test suite to confirm runtime compatibility with the new SDK version before merging. Specifically verify:
- Request/response types still match expected schema shapes
- Tool name format compliance (already appears correct)
- Deprecated SSE transport still functions or requires migration to StreamableHTTPServerTransport
libs/json-schema-to-zod-v3/src/utils.ts (5)
19-37: deepEqual implementation looks correct for JSON-like dataHandles primitives, arrays, and plain objects as expected for JSON Schema use cases; behavior for Maps/Dates/etc. is intentionally out of scope.
51-67: createUniqueItemsValidator matches JSON SchemauniqueItemssemanticsUses deepEqual over a seen list to detect duplicates; quadratic complexity is acceptable for typical schema sizes.
81-83: isValidWithSchema is a clean wrapper around schema validationUsing
safeParse().successis an idiomatic way to expose a boolean validation helper.Please double-check that all call sites only need a boolean and do not rely on detailed Zod error information.
96-98: isPlainObject is appropriate for this converter’s needsThe check for non-null, non-array objects is sufficient for JSON Schema objects.
116-125: propertyExists helper is consistent and complementshasOwnUsing
Object.getOwnPropertyDescriptoris a robust way to check for defined own properties, including non-enumerable ones.Ensure callers understand this includes non-enumerable properties and behaves slightly differently from the
inoperator.libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts (3)
12-30: Implicit string inference aligns with JSON Schema behaviorInferring a string type when string-specific keywords are present and no explicit
typeis declared is a reasonable and spec-aligned default, while respectingtypes.string === false.
36-60: MinLength handler is Unicode‑aware and guardedGrapheme-aware length via
Array.from(value).lengthand thetypes.string === falseandinstanceof z.ZodStringguards make this safe and predictable.Please verify that any upstream handlers always leave
types.stringas eitherfalseor aZodString, not unions, where you still want this refinement applied.
66-90: MaxLength handler mirrors MinLength with consistent semanticsSame grapheme-aware approach and guarding as
MinLengthHandler; behavior is consistent and clear.libs/sdk/src/tool/flows/tools-list.flow.ts (5)
9-41: Input/output/state schemas and plan wiring look consistentThe
inputSchema,outputSchema, andstateSchemaalign with MCP list-tools request/result types and the flow plan stages are clearly defined.Please confirm that any callers of
'tools:list-tools'are updated to send{ request, ctx }matchingListToolsRequestSchema.
72-95: Structured input parsing and method validation are solidWrapping the Zod parse in an
InvalidInputErrorand then enforcingmethod === 'tools/list'withInvalidMethodErrorgives clear, typed errors and good logging.Make sure tests cover:
- Malformed payloads (bad shape / missing
request).- Wrong
methodvalues.- Optional/absent
paramsandcursor.
97-125: Tool discovery stage is robustly logged and guardedCollecting tools via
this.scope.tools.getTools(), tagging them withowner.id, and storing them in state inside a try/catch with informative logs is a clean pattern.If
scope.apps.getApps()is only used for logging, consider later whether you want to include app names in logs or remove that call for clarity.
127-162: Name conflict resolution strategy is clear and deterministicCounting base names (
metadata.id ?? metadata.name), logging conflicts, and prefixing withappName:when needed provides a predictablefinalNamewithout surprising renames.It may be worth adding a test that verifies:
- Multiple tools with the same
metadata.idacross apps get distinctfinalNames.- Tools with unique base names remain unprefixed.
164-191: Tool descriptor construction & JSON Schema conversion look correctMapping to the MCP response items and choosing
rawInputSchemawhen available, otherwise deriving a JSON Schema viazodToJsonSchema(z.object(tool.metadata.inputSchema)), is a sensible approach and the stage has good logging and error handling.Please verify:
- All tools either provide
rawInputSchemaor aninputSchemashape compatible withz.object(...).- The resulting schema matches what MCP clients expect (e.g. parameters object schema, not the entire tool wrapper).
libs/json-schema-to-zod-v3/src/types.ts (3)
7-65: JSONSchema and JSONSchemaObject definitions are broadly compatible with Draft 7+The core/type/string/number/array/object/composition keywords and the index signature provide a practical balance between type safety and flexibility for this converter.
70-83: TypeRegistry design matches the handler patternUsing
z.ZodTypeAny | falseto distinguish “explicitly disabled” from “not yet initialized” works well with the primitive handlers.Confirm that any future primitive (e.g. integer-specific) either reuses
numberor extendsTypeRegistryin a backward-compatible way.
85-108: Handler interfaces are clear and consistent
SchemaHandler.apply(types, schema)andRefinementHandler.apply(zodSchema, schema)provide a clean contract for primitive and refinement stages.libs/json-schema-to-zod-v3/src/handlers/primitive/object.ts (3)
12-29: PropertiesHandler sets up a sensible base object typeInitializing
types.objecttoz.object({}).passthrough()when object-defining keywords are present, while respectingtypes.object === false, is a good primitive-stage default.
58-79: MaxProperties handler cleanly enforces property upper boundRefining on
Object.keys(obj).length <= maxPropertieswith a clear error message and reusing an existing object type or a passthrough base is straightforward and maintainable.
85-106: MinProperties handler mirrors MaxProperties correctlySymmetric refinement for the minimum required properties with similar guarding and messaging keeps behavior predictable.
libs/json-schema-to-zod-v3/src/converter.ts (1)
162-237: convertJsonSchemaToZod pipeline looks solid; ensure public API is documentedThe four‑phase flow (primitive handlers → collect allowed types → union/any/never → refinements) is consistent and appears to handle common JSON Schema patterns well (including boolean schemas and type unions). From an SDK perspective this is now a core public surface.
Per the
libs/**guideline, please double‑check thatconvertJsonSchemaToZod(and any new exports from this package) are reflected indocs/draft/docs/**rather than only in code comments, so consumers have a clear contract for this API. If that’s already covered in this PR, ignore this note.libs/sdk/src/tool/tool.instance.ts (6)
4-17: Imports and new dependencies look consistentThe expanded imports (ToolInput/Output types, ParsedToolResult, zod, MCP CallToolRequest, and buildParsedToolResult) align with the new schema‑driven parsing flow and tool utils. No issues here.
Also applies to: 20-25
27-32: Generic parameters and defaults onToolInstanceare now coherent
OutSchemacorrectly defaults toToolOutputType, andIn/Outderive fromToolInputOf<InSchema>/ToolOutputOf<OutSchema>, matching the metadata/decorator types. This resolves the earlier mismatch in output typing.
41-54: Constructor wiring forfullName, scope, and IO schemas looks goodUsing
owner.id + ':' + this.nameforfullNameshould make tool identification unambiguous, and wiringscope/hooksvia the active provider scope is consistent with the rest of the SDK. PersistingrawInputSchemaandoutputSchemadirectly from metadata is appropriate for later manifest/descriptor conversion; see separate comment about input validation behavior.
58-64: Hook registration restricted to tool flows is appropriateFiltering hooks to only
tools:call-toolandtools:list-toolsbefore registration keeps the tool instance focused on relevant flows and avoids over‑registering unrelated hooks. The async initialization pattern here looks fine.
72-78:getOutputSchemaoverride cleanly exposes raw output descriptorReturning the stored
outputSchemadirectly fromgetOutputSchemamatches the new descriptor‑based design and will be useful for tools/list responses and other manifest generation. No changes needed.
133-145:FunctionToolContextgenerics and execution wiring look correctThe generic parameters mirror
ToolInstance(InSchema/OutSchema/In/Out),Outnow correctly usesToolOutputOf<OutSchema>, andexecutesimply delegates torecord.provide(input, this). This matches the functional tool pattern and the previous review feedback.
libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/sdk/src/errors/mcp.error.ts (1)
278-280: Fix remainingno-inferrable-typeslint violation onisDevelopmentESLint is still flagging the
isDevelopmentparameter because its type is trivially inferred from the default literal. This will continue to fail lint/CI until updated, similar to the earlier fixes in this file.You can remove the explicit type:
-export function formatMcpErrorResponse(error: any, isDevelopment: boolean = process.env['NODE_ENV'] !== 'production') { +export function formatMcpErrorResponse(error: any, isDevelopment = process.env['NODE_ENV'] !== 'production') {This stays fully typed (boolean inferred from the default) and satisfies
@typescript-eslint/no-inferrable-types.libs/sdk/src/tool/tool.instance.ts (1)
46-50: NormalizeinputSchemato a raw shape before callingz.object(...)Right now
inputSchemais assigned fromrecord.metadata.inputSchemaunchanged, butparseInputthen doesz.object(this.inputSchema). If a tool declaresinputSchemaas az.object({...})instead of a raw shape, this will end up asz.object(z.object(...))and throw at runtime, contradicting the comment that both forms are supported. Normalizing Zod objects to their.shapeon construction fixes this.- const schema: any = record.metadata.inputSchema; - // Support both Zod objects and raw ZodRawShape - this.inputSchema = schema ? schema : {}; + const schema: any = record.metadata.inputSchema; + // Support both Zod objects (`z.object({...})`) and raw ZodRawShape + this.inputSchema = + schema instanceof z.ZodObject ? schema.shape : schema ?? {};(This keeps
parseInputunchanged but guaranteesthis.inputSchemais always a raw shape.)Also applies to: 102-105
🧹 Nitpick comments (8)
libs/sdk/src/errors/mcp.error.ts (5)
28-37: GuardError.captureStackTracefor non-V8 runtimes
Error.captureStackTraceis V8-specific and will throw in some non-Node/non-Chrome environments. Since this lives underlibs/sdk, consider guarding the call so the SDK remains usable in broader JS runtimes.Example:
protected constructor(message: string, errorId?: string) { super(message); this.name = this.constructor.name; this.errorId = errorId || this.generateErrorId(); if (typeof (Error as any).captureStackTrace === 'function') { (Error as any).captureStackTrace(this, this.constructor); } }If this SDK is guaranteed to be Node/V8-only, please confirm that assumption is acceptable for consumers.
2-2: Verify Node‑specific dependencies in a publishable SDKThis module relies on Node globals/APIs:
import { randomBytes } from 'crypto';process.env['NODE_ENV']informatMcpErrorResponse- Optional stack exposure via
this.stackIf
libs/sdkis intended to run in browsers or edge runtimes (or be bundled for them), these can cause build/runtime issues without polyfills. Two options:
- Confirm the SDK is strictly Node‑only and document that expectation, or
- Abstract environment concerns (e.g., inject an
errorIdgenerator and anisDevelopmentflag, and avoid direct Node imports in the core API).Given this is a public SDK surface, it’s worth validating the target runtime expectations.
Also applies to: 78-79, 278-280
128-133: Fix JSDoc description forToolNotFoundErrorThe comment says “Tool didn't find an error”, but the class represents a missing tool.
Consider aligning the docstring for clarity, e.g.:
/** * Tool not found error */ export class ToolNotFoundError extends PublicMcpError { // ... }
233-247: Consider deduplicating internal message formatting for wrapped errors
ToolExecutionError.getInternalMessageandGenericServerError.getInternalMessageshare the same pattern of appending the original error stack. You could centralize this behavior inMcpError(e.g., a protected helper or shared subclass) to keep things DRY and make future error types easier to maintain.Not required for correctness, just a maintainability improvement.
51-81: Optionally export a shared MCP error response type
McpError.toMcpErrorandformatMcpErrorResponseboth shape the same response object but don’t share an explicit return type. For consumers oflibs/sdk, an exported alias would make the public API clearer and avoid ad‑hoc typing downstream.For example:
export type McpErrorResponse = ReturnType<McpError['toMcpError']>; export abstract class McpError extends Error { // ... toMcpError(isDevelopment = false): McpErrorResponse { /* ... */ } } export function formatMcpErrorResponse( error: any, isDevelopment = process.env['NODE_ENV'] !== 'production', ): McpErrorResponse { const mcpError = toMcpError(error); return mcpError.toMcpError(isDevelopment); }This is purely a type-level improvement; behavior stays the same.
Also applies to: 278-280
libs/sdk/src/common/entries/tool.entry.ts (1)
18-25: Consider tighteningparseInput’s return type to the inferred input shape
ToolEntrynow correctly derivesIn/OutfromInSchema/OutSchemaviaToolInputOf/ToolOutputOf, andsafeParseOutputis parameterized onOut, which matches what it parses. To fully benefit from that inference, you may wantparseInputto returnInrather thanCallToolRequest['params']['arguments'], so downstream code can see the schema‑derived input type instead of the raw MCP argument type.Also applies to: 51-70
libs/sdk/src/common/decorators/tool.decorator.ts (2)
37-42: Handler/frontMcpTool generics now support primitive and structured outputsUsing
InSchema/OutSchemaplusToolInputOf/ToolOutputOfinFrontMcpToolExecuteHandlerandfrontMcpToolcorrectly relaxes the earlier “object‑only” output constraint so function tools can return primitives and tuples. You might consider typingctxasToolContext<InSchema, OutSchema, In, Out>instead of relying on the defaultIn/Outgenerics, but the current form is still coherent with the rest of the type wiring.Also applies to: 47-49
73-170: New input/output inference and decorator overloads are well-structured; watch for type duplication driftThe helper types (
__AsZodObj,ToolInputOf,__InferFromSingleSchema,__InferFromArraySchema,ToolOutputOf) plus the branded error helpers andTooloverloads give a strong, schema‑driven typing story and clear diagnostics for mismatchedexecutesignatures. The only minor concern is that__ToolSingleOutputType/__OutputSchemalocally re‑encode the allowed output forms that also exist inToolOutputType/ToolSingleOutputType; if those evolve, these unions must be kept in sync, so consider deriving them from the shared metadata types to avoid future drift.Also applies to: 191-238, 242-270
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/json-schema-to-zod-v3/src/__tests__/composition.spec.ts(1 hunks)libs/json-schema-to-zod-v3/src/__tests__/global.spec.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts(1 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/common/interfaces/tool.interface.ts(4 hunks)libs/sdk/src/errors/mcp.error.ts(1 hunks)libs/sdk/src/tool/tool.instance.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/json-schema-to-zod-v3/src/__tests__/global.spec.tslibs/json-schema-to-zod-v3/src/__tests__/composition.spec.tslibs/json-schema-to-zod-v3/src/handlers/refinement/composition.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/errors/mcp.error.tslibs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/common/interfaces/tool.interface.tslibs/sdk/src/tool/tool.instance.ts
🧠 Learnings (1)
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/errors/mcp.error.ts
🧬 Code graph analysis (7)
libs/json-schema-to-zod-v3/src/__tests__/composition.spec.ts (1)
libs/json-schema-to-zod-v3/src/types.ts (1)
JSONSchema(10-10)
libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (3)
libs/json-schema-to-zod-v3/src/types.ts (2)
RefinementHandler(100-108)JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)libs/json-schema-to-zod-v3/src/utils.ts (1)
isValidWithSchema(81-83)
libs/sdk/src/common/decorators/tool.decorator.ts (5)
libs/sdk/src/common/metadata/tool.metadata.ts (7)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)ImageOutputSchema(87-87)AudioOutputSchema(94-94)ResourceOutputSchema(101-101)ResourceLinkOutputSchema(108-108)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)apps/demo/src/apps/expenses/tools/create-expense.tool.ts (1)
Tool(4-27)apps/demo/src/apps/expenses/tools/add.tool.ts (1)
Tool(4-15)apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)
libs/sdk/src/errors/mcp.error.ts (1)
libs/sdk/src/errors/index.ts (15)
McpError(3-3)PublicMcpError(4-4)InternalMcpError(5-5)ToolNotFoundError(6-6)InvalidInputError(7-7)InvalidOutputError(8-8)InvalidMethodError(9-9)ToolExecutionError(10-10)RateLimitError(11-11)QuotaExceededError(12-12)UnauthorizedError(13-13)GenericServerError(14-14)isPublicError(15-15)toMcpError(16-16)formatMcpErrorResponse(17-17)
libs/sdk/src/common/entries/tool.entry.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)
libs/sdk/src/common/interfaces/tool.interface.ts (2)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)
libs/sdk/src/tool/tool.instance.ts (5)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/common/interfaces/tool.interface.ts (3)
input(86-88)input(90-97)ToolCtorArgs(20-26)libs/sdk/src/common/entries/tool.entry.ts (3)
ToolCallArgs(13-13)ToolCallExtra(14-16)ParsedToolResult(18-18)libs/sdk/src/tool/tool.utils.ts (1)
buildParsedToolResult(185-267)
🔇 Additional comments (8)
libs/json-schema-to-zod-v3/src/__tests__/global.spec.ts (1)
1-336: LGTM! Comprehensive test coverage for JSON Schema to Zod conversion.The test suite effectively covers:
- Nested object handling with required fields and type validation
- Real-world scenarios (user registration, API responses, product catalogs, OpenAPI parameters)
- Edge cases including empty schemas, empty enums, and implicit type detection
All test assertions use
safeParseappropriately and validate both success and failure cases.libs/json-schema-to-zod-v3/src/__tests__/composition.spec.ts (1)
1-130: LGTM! Thorough test coverage for composition keywords.The test suite validates all JSON Schema composition operators:
allOf: Intersection semantics with multiple schemasanyOf: Union semantics with at least one matchoneOf: Exactly-one-match enforcement, including multi-match rejectionnot: Negation with and without additional constraintsEach test includes both valid and invalid cases to ensure correct behavior.
libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (4)
15-41: LGTM! Correct implementation ofallOfintersection semantics.The handler properly converts each subschema and reduces them into intersections, ensuring values satisfy all schemas in the array.
47-81: LGTM! Correct implementation ofanyOfunion semantics.The handler appropriately creates a union of all subschemas (with special handling for single-schema case) and intersects with the existing schema.
87-129: LGTM! Correct implementation ofoneOfexactly-one-match semantics.The refine logic properly counts matching schemas and enforces that exactly one must match, with early exit optimization when multiple matches are detected.
135-158: LGTM! Correct implementation ofnotnegation semantics.The handler properly inverts the validation using
isValidWithSchema, ensuring values do not match the negated schema.libs/sdk/src/common/interfaces/tool.interface.ts (1)
11-33: ToolContext generic wiring and constructor look consistent with new schema-based typesThe new
ToolContext<InSchema, OutSchema, In, Out>defaults viaToolInputOf/ToolOutputOfline up with the updatedToolEntry/ToolInstanceand decorator helpers, and the constructor now cleanly threads those types plus a child logger into the context. I don’t see issues here; just make sure existingToolContextsubclasses (especially in apps/demo) still compile cleanly against the stricter generics.Also applies to: 59-69
libs/sdk/src/tool/tool.instance.ts (1)
72-80: Output parsing andParsedToolResultintegration look goodThe new
getOutputSchema,parseOutput, andsafeParseOutputmethods correctly preserve the rawoutputSchemadescriptor and funnel tool results throughbuildParsedToolResultinto aParsedToolResult, while exposing a Zod‑shapedsafeParseOutputinterface. Behavior looks consistent with the new output typing and MCP result shape.Also applies to: 117-129
libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
libs/json-schema-to-zod-v3/src/utils.ts (1)
100-114: FixhasOwnPropertyshadowing and align docstring with actual behaviorThe function still:
- Shadows the well-known global
hasOwnProperty, which Biome flags as an error, and- Has a docstring that says “Safely gets a property” while the implementation returns a boolean (existence check), not the property value.
Since this utility is internal to the package (not re-exported from
index.ts), you can safely rename it and update its description to avoid the shadowing and confusion.Suggested change:
-/** - * Safely gets a property from an object using Object.hasOwnProperty - * This is more robust than simple property access for special properties - * - * @param obj - Object to get property from - * @param prop - Property name - * @returns True if property exists on object - * - * @example - * hasOwnProperty({ a: 1 }, 'a') // true - * hasOwnProperty({ a: 1 }, 'b') // false - */ -export function hasOwnProperty(obj: any, prop: string): boolean { - return Object.prototype.hasOwnProperty.call(obj, prop); -} +/** + * Safely checks if an object has an own (non-inherited) property + * + * @param obj - Object to check + * @param prop - Property name + * @returns True if the property exists directly on the object + * + * @example + * hasOwn({ a: 1 }, 'a') // true + * hasOwn({ a: 1 }, 'b') // false + */ +export function hasOwn(obj: any, prop: string): boolean { + return Object.prototype.hasOwnProperty.call(obj, prop); +}and update internal imports/usages (e.g., in
libs/json-schema-to-zod-v3/src/handlers/refinement/special.ts) to callhasOwninstead.This should also clear the Biome
noShadowRestrictedNameserror for this SDK library.
🧹 Nitpick comments (5)
libs/json-schema-to-zod-v3/src/utils.ts (1)
96-97: Consider stricter plain-object detection if non-JSON objects are expectedCurrent
isPlainObjectwill treat instances likenew Date()or other custom prototypes as “plain objects.” If you anticipate such values, you could optionally tighten this to check the prototype, e.g.Object.getPrototypeOf(value) === Object.prototype, to avoid misclassifying non-POJOs. If this library only ever sees JSON data, the current implementation is fine.libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (1)
47-81: Avoid repeated conversions inAnyOfHandlerfor better efficiencyFunctionally this is fine, but
convertJsonSchemaToZodis called multiple times for the same subschemas whenanyOf.length > 1. You could precompute the array once and build the union from it:- // Build union of all anyOf schemas - const anyOfSchema = - schema.anyOf.length === 1 - ? convertJsonSchemaToZod(schema.anyOf[0]) - : z.union([ - convertJsonSchemaToZod(schema.anyOf[0]), - convertJsonSchemaToZod(schema.anyOf[1]), - ...schema.anyOf.slice(2).map((s) => convertJsonSchemaToZod(s)), - ] as any); + const anyOfSchemas = schema.anyOf.map((s) => convertJsonSchemaToZod(s)); + const anyOfSchema = + anyOfSchemas.length === 1 + ? anyOfSchemas[0] + : z.union(anyOfSchemas as any);This keeps behavior the same while avoiding redundant conversions for larger
anyOfarrays.libs/sdk/src/tool/tool.instance.ts (2)
46-48: Consider more robust Zod object detection.The
instanceof z.ZodObjectcheck on line 48 addresses the previous issue, but could fail if the Zod library is bundled separately or the schema comes from a different Zod instance. A more defensive approach would check for the.shapeproperty or inspect._def.typeName.Example refactor:
- this.inputSchema = schema instanceof z.ZodObject ? schema.shape : schema ?? {}; + this.inputSchema = + (schema instanceof z.ZodObject || schema?._def?.typeName === 'ZodObject') + ? schema.shape + : schema ?? {};
103-106: Consider caching the input schema for performance.The method recreates
z.object(this.inputSchema)on each call. For tools that are called frequently, consider caching the constructed schema as a private class property initialized in the constructor.Example refactor:
// In constructor: this.inputSchemaValidator = z.object(this.inputSchema); // In parseInput: override parseInput(input: CallToolRequest['params']): CallToolRequest['params']['arguments'] { return this.inputSchemaValidator.parse(input.arguments); }libs/plugins/src/cache/cache.plugin.ts (1)
13-17: Unify default TTL configuration and avoid magic numbersThe default TTL is currently wired in three places:
- Default provider uses a hard-coded
60 * 60 * 24.dynamicProviders('memory')usesoptions.defaultTTL(which may beundefinedif not supplied).- The constructor merges
{ defaultTTL: 60 * 60 * 24, ...options }intothis.options.This makes it easy for defaults to drift and, in the
dynamicProviderscase, for the memory provider to be instantiated without an explicit TTL when none is provided.Consider introducing a single constant and using it consistently, while also ensuring
defaultOptionsincludes the TTL:-import CacheMemoryProvider from './providers/cache-memory.provider'; +import CacheMemoryProvider from './providers/cache-memory.provider'; + +const DEFAULT_CACHE_TTL = 60 * 60 * 24; // 24h ... - { - // this is a default provider for cache, will be overridden if dynamicProviders based on config - name: 'cache:memory', - provide: CacheStoreToken, - useValue: new CacheMemoryProvider(60 * 60 * 24), - }, + { + // this is a default provider for cache, will be overridden if dynamicProviders based on config + name: 'cache:memory', + provide: CacheStoreToken, + useValue: new CacheMemoryProvider(DEFAULT_CACHE_TTL), + }, ... - case 'memory': + case 'memory': providers.push({ name: 'cache:memory', provide: CacheStoreToken, - useValue: new CacheMemoryProvider(options.defaultTTL), + useValue: new CacheMemoryProvider(options.defaultTTL ?? DEFAULT_CACHE_TTL), }); ... - static defaultOptions: CachePluginOptions = { - type: 'memory', - }; + static defaultOptions: CachePluginOptions = { + type: 'memory', + defaultTTL: DEFAULT_CACHE_TTL, + }; ... constructor(options: CachePluginOptions = CachePlugin.defaultOptions) { super(); this.options = { - defaultTTL: 60 * 60 * 24, + defaultTTL: DEFAULT_CACHE_TTL, ...options, }; }Since
CachePluginOptionsis part of the public SDK surface underlibs/**, also double‑check that the docs/draft docs for the cache plugin configuration mentiondefaultTTLand its default value so consumers know what to expect.Also applies to: 32-37, 43-52
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/basic.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/object.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/refinement/complex.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts(1 hunks)libs/json-schema-to-zod-v3/src/utils.ts(1 hunks)libs/plugins/src/cache/cache.plugin.ts(2 hunks)libs/sdk/src/tool/tool.instance.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/json-schema-to-zod-v3/src/handlers/refinement/complex.ts
- libs/json-schema-to-zod-v3/src/handlers/primitive/array.ts
- libs/json-schema-to-zod-v3/src/handlers/primitive/object.ts
- libs/json-schema-to-zod-v3/src/handlers/primitive/basic.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/json-schema-to-zod-v3/src/utils.tslibs/plugins/src/cache/cache.plugin.tslibs/json-schema-to-zod-v3/src/handlers/refinement/composition.tslibs/sdk/src/tool/tool.instance.ts
🧬 Code graph analysis (4)
libs/json-schema-to-zod-v3/src/utils.ts (1)
libs/json-schema-to-zod-v3/src/index.ts (3)
deepEqual(47-47)createUniqueItemsValidator(47-47)isValidWithSchema(47-47)
libs/plugins/src/cache/cache.plugin.ts (1)
libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)
libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (3)
libs/json-schema-to-zod-v3/src/types.ts (2)
RefinementHandler(100-108)JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)libs/json-schema-to-zod-v3/src/utils.ts (1)
isValidWithSchema(81-83)
libs/sdk/src/tool/tool.instance.ts (4)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/common/entries/tool.entry.ts (3)
ToolCallArgs(13-13)ToolCallExtra(14-16)ParsedToolResult(18-18)libs/sdk/src/tool/tool.utils.ts (1)
buildParsedToolResult(185-267)
🪛 Biome (2.1.2)
libs/json-schema-to-zod-v3/src/utils.ts
[error] 112-112: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (10)
libs/json-schema-to-zod-v3/src/utils.ts (2)
19-37: Deep equality, uniqueness validator, and schema validation helper look correct for JSON-style dataThe control flow and recursion for
deepEqual, the O(n²) but acceptable uniqueness check increateUniqueItemsValidator, and the simpleisValidWithSchemawrapper are all consistent and readable for the intended JSON-schema/Zod use cases.Also applies to: 51-67, 81-83
116-125:propertyExistswrapper is clear and consistent with intended usageThis helper is a straightforward and readable wrapper around
Object.getOwnPropertyDescriptorfor own-property existence checks; no issues from my side.libs/json-schema-to-zod-v3/src/handlers/refinement/composition.ts (4)
6-10: Correct relative import forconvertJsonSchemaToZodImporting
convertJsonSchemaToZodfrom../../converterfixes the earlier ambientdeclare/self-import problem and ensures a real runtime binding while keeping the circular dependency safe (only used insideapply()).
15-40:AllOfHandler.applyintersection logic is straightforward and guardedThe early return on missing/empty
schema.allOfand the reduction of converted subschemas into a chain ofz.intersectioncalls starting from the existingzodSchemamatch the intended “must satisfy all” semantics and keep the implementation easy to follow.
87-129:OneOfHandlerrefinement correctly enforces “exactly one” semanticsThe
validCountloop with early exit when more than one subschema matches, combined with the finalvalidCount === 1check, implements the expectedoneOfbehavior cleanly and without unnecessary extra passes.
135-157:NotHandlerrefine correctly inverts the subschemaGuarding on missing
schema.not, converting it once, and then refining the base schema with!isValidWithSchema(notSchema, value)provides a clear and maintainable implementation of JSON Schema’snotkeyword.libs/sdk/src/tool/tool.instance.ts (4)
3-25: LGTM: Imports properly support the enhanced type system.The new imports for
ToolInputType,ToolOutputType,ParsedToolResult,ToolInputOf,ToolOutputOf, Zod validation, andbuildParsedToolResultcorrectly support the expanded input/output schema handling and parsing capabilities.
27-32: Verified: Previous type parameter issues resolved.The class declaration now correctly defaults
OutSchematoToolOutputType(previously wasToolInputType), and theIn/Outtype parameters properly use object wrappers that match theToolInputOfandToolOutputOfhelper signatures.
81-101: LGTM: Properly typed context creation.The
create()method correctly returns typedToolContextinstances with proper generic parameter propagation for both CLASS_TOKEN and FUNCTION tool kinds.
134-147: Verified: FunctionToolContext type parameters corrected.The generic parameters now correctly use
ToolOutputOf<{ outputSchema: OutSchema }>for theOuttype (previously incorrectly usedToolInputOf), aligning with the parentToolInstancepattern.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
libs/json-schema-to-zod-v3/README.md (2)
542-559: Add a language to the architecture fenced code block to satisfy markdownlint (MD040).The tree snippet under “Architecture” uses a bare triple‑backtick fence. Markdownlint expects a language identifier.
You can fix this by marking it as plain text:
-``` +```text src/ ├── index.ts # Public API ... -``` +```
625-627: Optional wording tweak in “Contributing” section.The sentence “Please feel free to submit a Pull Request.” is fine but slightly verbose. If you want a tighter, more formal tone you could use, for example, “Contributions are welcome—submit a pull request.” This is purely stylistic; no need to change unless you care about the tone.
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (1)
93-100: Avoid tests that rely on real timing; they may be flaky and one test has no real assertion.
- The “should timeout on slow patterns” test only asserts
expect(true).toBe(true), so it will always pass even ifcreateSafeRegExpregresses.- The performance tests assert
elapsed < 100andelapsed < 50ms, which can be brittle on loaded or slower CI runners even if the implementation is correct.Consider tightening these tests to focus on behaviour rather than wall‑clock timing. For example:
- For the slow‑pattern case, assert that
createSafeRegExpreturns eithernullor aRegExpwithout hanging and drop the timing assertion:- it('should timeout on slow patterns', () => { - // This pattern might be slow on certain inputs - const regex = createSafeRegExp('^(a|a)*$', undefined, 1); - - // The regex might be created but will timeout during testing - // Just verify it doesn't hang - expect(true).toBe(true); - }); + it('should handle slow patterns without hanging', () => { + const regex = createSafeRegExp('^(a|a)*$', undefined, 1); + // No hang = pass; we just assert the call returns promptly. + expect(regex === null || regex instanceof RegExp).toBe(true); + });
- For perf tests, either:
- Switch to a much looser upper bound (e.g., 500–1000ms), or
- Drop the time assertions and instead assert that the loops complete and return expected safety flags.
Please verify in your CI environment whether strict time bounds are actually stable.Also applies to: 217-242
libs/json-schema-to-zod-v3/src/index.ts (1)
150-164: Default export wrappers miximportandrequire; consider simplifying or documenting the intent.The default export re‑wraps
createUniqueItemsValidatorandisValidWithSchemausingrequire('./utils')inside an ES‑module‑style file. This works if your build targets CommonJS, but it’s a bit unconventional and might cause friction if you ever ship ESM builds.If you don’t strictly need lazy loading here, you could simplify to direct imports and avoid
require:-import { z } from 'zod'; -import { convertJsonSchemaToZod } from './converter'; -import { JSONSchemaObject } from './types'; +import { z } from 'zod'; +import { convertJsonSchemaToZod } from './converter'; +import { JSONSchemaObject } from './types'; +import { createUniqueItemsValidator, isValidWithSchema } from './utils'; ... export default { convertJsonSchemaToZod, jsonSchemaObjectToZodRawShape, - createUniqueItemsValidator: () => { - const { createUniqueItemsValidator: fn } = require('./utils'); - return fn(); - }, - isValidWithSchema: (schema: z.ZodTypeAny, value: any) => { - const { isValidWithSchema: fn } = require('./utils'); - return fn(schema, value); - }, + createUniqueItemsValidator, + isValidWithSchema, };Please verify this matches your bundling/module‑format constraints (CJS vs ESM) before changing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/json-schema-to-zod-v3/README.md(1 hunks)libs/json-schema-to-zod-v3/SECURITY.md(1 hunks)libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts(1 hunks)libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts(1 hunks)libs/json-schema-to-zod-v3/src/index.ts(1 hunks)libs/json-schema-to-zod-v3/src/security.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/json-schema-to-zod-v3/src/handlers/primitive/string.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/json-schema-to-zod-v3/src/security.tslibs/json-schema-to-zod-v3/src/index.tslibs/json-schema-to-zod-v3/README.mdlibs/json-schema-to-zod-v3/SECURITY.mdlibs/json-schema-to-zod-v3/src/__tests__/security.spec.ts
🧬 Code graph analysis (3)
libs/json-schema-to-zod-v3/src/security.ts (1)
libs/json-schema-to-zod-v3/src/index.ts (8)
PatternValidationResult(64-64)validatePattern(55-55)createSafeRegExp(56-56)PatternSecurityConfig(65-65)DEFAULT_SECURITY_CONFIG(60-60)setSecurityConfig(58-58)getSecurityConfig(59-59)createSafePatternValidator(57-57)
libs/json-schema-to-zod-v3/src/index.ts (2)
libs/json-schema-to-zod-v3/src/types.ts (1)
JSONSchemaObject(15-65)libs/json-schema-to-zod-v3/src/converter.ts (1)
convertJsonSchemaToZod(162-238)
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (1)
libs/json-schema-to-zod-v3/src/security.ts (6)
setSecurityConfig(227-229)DEFAULT_SECURITY_CONFIG(198-205)validatePattern(70-117)createSafeRegExp(139-175)createSafePatternValidator(250-276)getSecurityConfig(234-236)
🪛 ast-grep (0.40.0)
libs/json-schema-to-zod-v3/src/security.ts
[warning] 104-104: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 152-152: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 ESLint
libs/json-schema-to-zod-v3/src/security.ts
[error] 142-142: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts
[error] 155-155: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.
(@typescript-eslint/ban-ts-comment)
🪛 LanguageTool
libs/json-schema-to-zod-v3/README.md
[style] ~627-~627: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## Support If ...
(FEEL_FREE_TO_STYLE_ME)
libs/json-schema-to-zod-v3/SECURITY.md
[style] ~246-~246: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...schemas (may cause stack overflow) -
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
libs/json-schema-to-zod-v3/README.md
542-542: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
libs/json-schema-to-zod-v3/src/security.ts (1)
103-107: Static-analysis ReDoS warning is mitigated byvalidatePattern; no change needed beyond config wiring.Tools warn about
new RegExp(pattern)andnew RegExp(pattern, flags)being constructed from variable input, but here:
createSafeRegExpcallsvalidatePatternfirst to reject dangerous patterns and syntax errors before constructing the RegExp.- After config wiring (see previous comment), these calls remain guarded and are part of the protection logic itself.
No further change is necessary specifically for the ReDoS warning, beyond ensuring that
validatePatterncorrectly enforces the thresholds and that config is wired as intended.Also applies to: 152-154
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
libs/json-schema-to-zod-v3/src/security.ts (2)
64-111: PatternSecurityConfig is not honored in validatePattern (length/quantifier thresholds + enableProtection).
validatePatterncurrently hard-codesMAX_PATTERN_LENGTHandMAX_QUANTIFIERand ignoresglobalConfig, sosetSecurityConfig({ maxPatternLength, maxQuantifier, enableProtection })has no effect on validation. That’s a behavioral mismatch for a public API inlibs/**that exposesPatternSecurityConfig.Consider wiring the global config through
validatePatternso callers can actually tune protection:-export function validatePattern(pattern: string): PatternValidationResult { - // Check pattern length first - if (pattern.length > MAX_PATTERN_LENGTH) { - return { - safe: false, - reason: `Pattern exceeds maximum length of ${MAX_PATTERN_LENGTH} characters` - }; - } +export function validatePattern(pattern: string): PatternValidationResult { + const { enableProtection, maxPatternLength, maxQuantifier } = globalConfig; + + // If protection is disabled, only check syntax and skip heuristic checks. + if (!enableProtection) { + try { + new RegExp(pattern); + return { safe: true, pattern }; + } catch (error) { + return { + safe: false, + reason: `Invalid regex syntax: ${ + error instanceof Error ? error.message : 'Unknown error' + }`, + }; + } + } + + // Check pattern length first + if (pattern.length > maxPatternLength) { + return { + safe: false, + reason: `Pattern exceeds maximum length of ${maxPatternLength} characters`, + }; + } @@ - const quantifierMatch = pattern.match(/\{(\d+),?(\d+)?\}/g); + const quantifierMatch = pattern.match(/\{(\d+),?(\d+)?\}/g); if (quantifierMatch) { for (const q of quantifierMatch) { const nums = q.match(/\d+/g); - if (nums && nums.some(n => parseInt(n) > MAX_QUANTIFIER)) { + if (nums && nums.some((n) => parseInt(n, 10) > maxQuantifier)) { return { safe: false, - reason: `Quantifier exceeds maximum value of ${MAX_QUANTIFIER}` + reason: `Quantifier exceeds maximum value of ${maxQuantifier}`, }; } } }This makes
maxPatternLength,maxQuantifier, andenableProtectionactually affect validation, aligning the implementation with the public configuration type and docs. As per coding guidelines.
133-169: createSafeRegExp ignores PatternSecurityConfig (enableProtection/warnOnUnsafe/throwOnUnsafe/timeoutMs) and has a lint error on timeoutMs.Right now:
- Validation is always enforced (no respect for
enableProtection).- Unsafe patterns always log via
console.warn, never throw, sowarnOnUnsafe/throwOnUnsafedo nothing.- The
timeoutMsparameter default is hard-coded to100, ignoringglobalConfig.timeoutMs.timeoutMs: number = 100triggers@typescript-eslint/no-inferrable-types.To both fix the lint error and honor the documented config, you can restructure
createSafeRegExplike:-export function createSafeRegExp( - pattern: string, - flags?: string, - timeoutMs: number = 100 -): RegExp | null { - // First validate the pattern - const validation = validatePattern(pattern); - if (!validation.safe) { - console.warn(`[ReDoS Protection] Rejected unsafe pattern: ${validation.reason}`); - return null; - } +export function createSafeRegExp( + pattern: string, + flags?: string, + timeoutMs?: number, +): RegExp | null { + const { + enableProtection, + warnOnUnsafe, + throwOnUnsafe, + timeoutMs: configTimeoutMs, + } = globalConfig; + + const effectiveTimeout = timeoutMs ?? configTimeoutMs; + + const handleUnsafe = (message: string, error?: unknown): null => { + if (throwOnUnsafe) { + throw new Error(message); + } + if (warnOnUnsafe) { + if (error) { + console.warn(message, error); + } else { + console.warn(message); + } + } + return null; + }; + + if (enableProtection) { + const validation = validatePattern(pattern); + if (!validation.safe) { + return handleUnsafe( + `[ReDoS Protection] Rejected unsafe pattern: ${validation.reason ?? 'unknown reason'}`, + ); + } + } @@ - // Test the regex with a simple string to catch runtime issues - const testStart = Date.now(); - try { - regex.test('test'); - const elapsed = Date.now() - testStart; - - if (elapsed > timeoutMs) { - console.warn(`[ReDoS Protection] Regex took ${elapsed}ms to test, rejecting pattern`); - return null; - } - } catch (error) { - console.warn(`[ReDoS Protection] Regex test failed:`, error); - return null; - } + if (enableProtection) { + // Test the regex with a simple string to catch runtime issues + const testStart = Date.now(); + try { + regex.test('test'); + const elapsed = Date.now() - testStart; + + if (elapsed > effectiveTimeout) { + return handleUnsafe( + `[ReDoS Protection] Regex took ${elapsed}ms to test, rejecting pattern`, + ); + } + } catch (error) { + return handleUnsafe('[ReDoS Protection] Regex test failed', error); + } + } @@ - } catch (error) { - console.warn(`[ReDoS Protection] Failed to create regex:`, error); - return null; - } + } catch (error) { + return handleUnsafe('[ReDoS Protection] Failed to create regex', error); + }This:
- Respects
enableProtection(can skip heuristics if disabled).- Respects
warnOnUnsafeandthrowOnUnsafe.- Uses
timeoutMs ?? globalConfig.timeoutMsinstead of a hard-coded default.- Removes the no-inferrable-types lint issue by replacing
timeoutMs: number = 100with an optional parameter and internal defaulting.Given this is exported from
libs/json-schema-to-zod-v3/src/index.ts, wiring the config is important for SDK consumers relying on the documented knobs. As per coding guidelines.libs/sdk/src/common/entries/tool.entry.ts (2)
27-50: Properties and accessor are well-structured.The new properties (
name,fullName,inputSchema,rawInputSchema,outputSchema) provide clear access to tool metadata, and thegetOutputSchema()accessor appropriately exposes the raw metadata schema for MCP conversion.Note: Past review comments already flagged the missing documentation for
getOutputSchema()as a public API change inlibs/**. That concern remains valid.
52-71: Abstract method signatures are correctly designed.The parsing methods appropriately use
Out | Partial<Out> | anyto accommodate runtime flexibility, andsafeParseOutputcorrectly usesSafeTransformResult(not Zod's parse types) since these methods transform rather than validate.Note: Past review comments already flagged missing documentation for
parseOutput()andsafeParseOutput()under thelibs/**guidelines. That concern remains valid.libs/sdk/src/tool/tool.instance.ts (3)
51-55: Type assertions indicate incomplete metadata typing.The
(record.metadata as any)casts suggestToolRecord's metadata type doesn't includerawInputSchemaandoutputSchemaproperties. This weakens type safety in a publishable SDK library.Note: A past review comment already suggested updating the
ToolRecordmetadata type definition to include these properties. That concern remains unaddressed.
60-76: Initialization and accessor methods are correctly implemented.The
initialize()method appropriately filters and registers tool-related hooks, andgetOutputSchema()provides the expected accessor functionality.Note: Past review comments already flagged missing documentation for
getOutputSchema(). That concern remains valid.
104-119: Output parsing correctly delegates to utility function.The method appropriately delegates to
buildParsedToolResult, which handles various descriptor types (primitives, arrays, structured schemas) and transforms them into MCP-compliantCallToolResult. The inline comment clearly documents the transformation rules.Note: Past review comments already flagged missing documentation for
parseOutput(). That concern remains valid.
🧹 Nitpick comments (6)
libs/json-schema-to-zod-v3/src/security.ts (1)
244-269: createSafePatternValidator relies on createSafeRegExp; consider reusing config semantics for logging/throwing.
createSafePatternValidatoralready respectsglobalConfig.timeoutMsfor per-value checks, which is good. OncecreateSafeRegExpis updated to honorenableProtection/warnOnUnsafe/throwOnUnsafe, this function will automatically benefit when creating the regex.If you want full consistency, you could also consider gating the
console.warncalls inside the validator onwarnOnUnsafeand/orenableProtection, but that’s optional and can be deferred.libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (4)
16-141: Core validator/regex tests are solid but don’t exercise config-driven behavior.The
validatePattern,createSafeRegExp, andcreateSafePatternValidatortests cover a good spread of safe/unsafe patterns and basic edge cases, which is great.Once
PatternSecurityConfigis wired intovalidatePattern/createSafeRegExp, consider adding a few focused tests here to assert that config changes actually affect behavior, for example:
- Lower
maxPatternLengthand confirm a previously safe pattern is now rejected.- Lower
maxQuantifierand confirm a previously safe quantifier is now rejected.- Set
enableProtection: falseand confirm that a pattern that would normally be blocked is allowed throughcreateSafeRegExp.- Set
throwOnUnsafe: trueand assert thatcreateSafeRegExpthrows for an unsafe pattern.That will guard against regressions where config fields become no-ops again.
143-176: Security Configuration tests only cover shape, not behavior; consider adding behavioral assertions.The “Security Configuration” block currently verifies:
- Defaults from
getSecurityConfig.- That
setSecurityConfigmutatesglobalConfig.- That
getSecurityConfigreturns an immutable copy.What’s missing are tests tying these settings to runtime behavior. After wiring
globalConfigintovalidatePattern/createSafeRegExp, I’d suggest extending this block (or adding a new one) with tests such as:
setSecurityConfig({ maxPatternLength: 10 })→validatePattern('a'.repeat(11))becomes unsafe.setSecurityConfig({ enableProtection: false })→createSafeRegExp('(a+)+')returns a non-null regex (and does not warn/throw, depending on config).setSecurityConfig({ throwOnUnsafe: true })→createSafeRegExp('(a+)+')throws.That will ensure API-level config contract remains enforced as the implementation evolves.
108-115: “slow patterns” test does not actually assert behavior and currently never reaches the timeout path.In
it('should timeout on slow patterns', ...):
- You call
createSafeRegExp('^(a|a)*$', undefined, 1)but then only assertexpect(true).toBe(true).- Given the current heuristics,
'^(a|a)*$'is rejected up front byvalidatePatternas “dangerous constructs”, so the internal “slow test” path increateSafeRegExpis never exercised.As written, this test only verifies that the call doesn’t hang, which is effectively a no-op in CI. Consider either:
- Changing the pattern or config so that the timeout path is actually hit and then asserting on the return value (e.g.,
expect(regex).toBeNull()or a warning/throw), or- Removing/relaxing this test if you don’t intend to guarantee timeout semantics at this level.
233-259: Performance tests with strict time thresholds may be flaky on slower CI.The “Performance” tests assert:
- Validating 1000 safe patterns completes in
< 100ms.- Rejecting 100 dangerous patterns completes in
< 50ms.These hard time bounds using
Date.now()can easily become flaky on slower or loaded CI machines, even if the implementation is fine.Consider making these tests more robust by, for example:
- Increasing the thresholds significantly (e.g., 500–1000ms), or
- Converting them into sanity checks (e.g., just running the loops to ensure they don’t hang or throw), or
- Marking them as optional/slow if your test runner supports it.
That keeps the intent (no pathological performance) without turning them into non-deterministic CI failures.
libs/plugins/src/cache/cache.plugin.ts (1)
13-13: Align memory provider TTL with plugin default TTL to avoid inconsistent expiry behaviorRight now the defaults are a bit inconsistent:
- The static provider uses a hard‑coded TTL:
new CacheMemoryProvider(60 * 60 * 24)(24h).dynamicProvidersfor'memory'usesnew CacheMemoryProvider(options.defaultTTL), butdefaultOptionsonly sets{ type: 'memory' }, sooptions.defaultTTLcan beundefined.- The instance field
this.optionsalways injectsdefaultTTL: 60 * 60 * 24when missing in the constructor.This can lead to a situation where the plugin assumes a 24h default (
this.options.defaultTTL) while the dynamically-registered memory provider sees anundefinedTTL, potentially changing how expiry is applied.Consider normalizing this so that memory always gets a concrete TTL, for example by either:
- Including
defaultTTLindefaultOptions, or- Falling back to the same constant in
dynamicProviders:static defaultOptions: CachePluginOptions = { - type: 'memory', + type: 'memory', + defaultTTL: 60 * 60 * 24, }; ... case 'memory': providers.push({ name: 'cache:memory', provide: CacheStoreToken, - useValue: new CacheMemoryProvider(options.defaultTTL), + useValue: new CacheMemoryProvider(options.defaultTTL ?? 60 * 60 * 24), });This keeps the memory provider’s behavior consistent regardless of whether the caller supplies
defaultTTL.Also applies to: 36-37, 43-54
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/push.yml(1 hunks)libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts(1 hunks)libs/json-schema-to-zod-v3/src/index.ts(1 hunks)libs/json-schema-to-zod-v3/src/security.ts(1 hunks)libs/plugins/src/cache/cache.plugin.ts(2 hunks)libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/tool/tool.instance.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/json-schema-to-zod-v3/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/json-schema-to-zod-v3/src/__tests__/security.spec.tslibs/plugins/src/cache/cache.plugin.tslibs/sdk/src/tool/tool.instance.tslibs/json-schema-to-zod-v3/src/security.tslibs/sdk/src/common/entries/tool.entry.ts
🧬 Code graph analysis (5)
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (2)
libs/json-schema-to-zod-v3/src/index.ts (7)
setSecurityConfig(58-58)DEFAULT_SECURITY_CONFIG(60-60)validatePattern(55-55)createSafeRegExp(56-56)createSafePatternValidator(57-57)getSecurityConfig(59-59)convertJsonSchemaToZod(79-79)libs/json-schema-to-zod-v3/src/security.ts (6)
setSecurityConfig(221-223)DEFAULT_SECURITY_CONFIG(192-199)validatePattern(64-111)createSafeRegExp(133-169)createSafePatternValidator(244-270)getSecurityConfig(228-230)
libs/plugins/src/cache/cache.plugin.ts (1)
libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)
libs/sdk/src/tool/tool.instance.ts (5)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/scope/scope.instance.ts (1)
hooks(124-126)libs/sdk/src/common/entries/tool.entry.ts (4)
ToolCallArgs(13-13)ToolCallExtra(14-16)ParsedToolResult(18-18)SafeTransformResult(19-19)libs/sdk/src/tool/tool.utils.ts (1)
buildParsedToolResult(185-267)
libs/json-schema-to-zod-v3/src/security.ts (1)
libs/json-schema-to-zod-v3/src/index.ts (8)
PatternValidationResult(64-64)validatePattern(55-55)createSafeRegExp(56-56)PatternSecurityConfig(65-65)DEFAULT_SECURITY_CONFIG(60-60)setSecurityConfig(58-58)getSecurityConfig(59-59)createSafePatternValidator(57-57)
libs/sdk/src/common/entries/tool.entry.ts (2)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)
🪛 ast-grep (0.40.0)
libs/json-schema-to-zod-v3/src/security.ts
[warning] 74-74: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 146-146: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 ESLint
libs/json-schema-to-zod-v3/src/security.ts
[error] 136-136: Type number trivially inferred from a number literal, remove type annotation.
(@typescript-eslint/no-inferrable-types)
⏰ 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 (12)
libs/json-schema-to-zod-v3/src/security.ts (2)
171-199: Config types/defaults look good, but behavior must be aligned with them.
PatternSecurityConfig,DEFAULT_SECURITY_CONFIG, andglobalConfigare well-structured, and usingMAX_PATTERN_LENGTH/MAX_QUANTIFIERas defaults is sensible. However, untilvalidatePatternandcreateSafeRegExpconsultglobalConfig, these config fields are effectively documentation-only.Once you wire
globalConfiginto the core functions (as suggested above), the rest of this section should be sufficient; no changes needed here beyond keeping docs in sync with actual behavior.
221-230: getSecurityConfig immutability is implemented correctly.
setSecurityConfigmerges intoglobalConfig, andgetSecurityConfigreturns a shallow copy ({ ...globalConfig }) typed asReadonly<PatternSecurityConfig>. That matches the tests’ intent to prevent external mutation of the global config.No changes needed here.
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (1)
203-231: JSON Schema integration tests look appropriate.The integration tests that use
convertJsonSchemaToZodwith unsafe vs safepatternvalues nicely cover end-to-end behavior, ensuring that the security layer is actually applied in the schema conversion path.No issues here from the provided context.
libs/plugins/src/cache/cache.plugin.ts (1)
111-124:hashObjectimplementation looks fine for current usageGiven that you always call
hashObjectwith a shallow wrapper object ({ tool: tool.fullName, input: toolContext.input }), this implementation is deterministic (sorted keys) and sufficient for generating stable cache keys. No functional issues stand out here.libs/sdk/src/common/entries/tool.entry.ts (2)
1-19: LGTM: Clean type definitions supporting the expanded tool API.The new type aliases (
ParsedToolResult,SafeTransformResult) provide clear, MCP-aligned types for tool output handling.SafeTransformResultappropriately uses a genericErrortype rather than coupling to Zod's parse types, accurately reflecting that these methods transform rather than validate.
21-26: Generic signature correctly wired.The default
In/Outgenerics now properly wrap schemas in objects ({ inputSchema: InSchema },{ outputSchema: OutSchema }), ensuringToolInputOfandToolOutputOfresolve correctly. This addresses the previous typing issue.libs/sdk/src/tool/tool.instance.ts (6)
1-33: LGTM: Imports and generic signature are correct.The class properly extends
ToolEntrywith matching generic parameters, and the default types are correctly set (OutSchema = ToolOutputType). All necessary imports are present.
38-58: Constructor correctly normalizes input schema.Lines 47-49 properly handle both
z.ZodObject(extracting.shape) and rawZodRawShape, ensuringparseInputalways receives a raw shape. This addresses the previous concern about mixed Zod object handling.
78-98: LGTM: Context creation correctly handles different tool kinds.The method appropriately branches on
record.kindand creates the corresponding context type (CLASS_TOKEN→ direct instantiation,FUNCTION→FunctionToolContextwrapper). Generic types are properly threaded through.
100-103: Input parsing logic is correct.The method creates a Zod object from the normalized
inputSchema(guaranteed to be a raw shape) and parses the input arguments. This will throwZodErroron validation failure, which is the expected behavior.
121-128: LGTM: Safe parsing uses appropriate return type.The method correctly uses
SafeTransformResult<ParsedToolResult>(not Zod'sSafeParseReturnType), accurately reflecting that this is a transformation operation rather than schema validation. Error handling is appropriate.
131-144: FunctionToolContext correctly implements tool wrapper.The generic signature properly uses
ToolOutputOf<{ outputSchema: OutSchema }>for theOuttype (addressing previous incorrect usage ofToolInputOf). The wrapper appropriately delegates execution to the function-based tool implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
libs/sdk/src/common/entries/tool.entry.ts (1)
42-49: Verify documentation for thegetOutputSchema()public API.Per coding guidelines for
libs/**, public API changes require matching documentation indocs/draft/docs/**. A past review comment flagged this method as missing documentation, but the issue was not marked as resolved. Please verify that documentation has been added explaining this method's purpose, return type, and usage.If documentation is still missing, generate it under
docs/draft/docs/**covering:
- Method purpose (exposes the raw
outputSchemafrom metadata)- Return type and possible values (string literal, Zod schema, raw shape, array, or undefined)
- Usage example showing how consumers retrieve and use the schema
libs/sdk/src/tool/tool.instance.ts (2)
74-76: Verify documentation for thegetOutputSchema()override.Per coding guidelines for
libs/**, this public API override requires matching documentation indocs/draft/docs/**. A past review flagged this as missing, but the issue was not marked as resolved. Please confirm documentation has been added.
104-119: Verify documentation forparseOutput()andsafeParseOutput()public APIs.Per coding guidelines for
libs/**, these new public methods require matching documentation indocs/draft/docs/**. Past reviews flagged missing documentation covering transformation rules, output schema handling, and usage examples, but these issues were not marked as resolved. Please verify documentation has been added.
🧹 Nitpick comments (3)
libs/sdk/src/common/entries/tool.entry.ts (1)
36-40: Consider tighteningrawInputSchematype for better safety.Line 38 types
rawInputSchemaasany, which is very permissive. While the comment indicates it's for JSON-schema-ish representations, consider using a more constrained type such asJSONSchema7 | unknownor a union of expected schema formats to improve type safety and catch potential misuse at compile time.- rawInputSchema: any; + rawInputSchema: JSONSchema7 | unknown;(You may need to import
JSONSchema7fromjson-schemaor define a custom union type representing your supported schema formats.)libs/sdk/src/tool/tool.instance.ts (2)
47-55: Type assertion at line 55 weakens type safety.Line 55 uses
as OutSchemato castrecord.metadata.outputSchemawithout runtime validation. If the metadata doesn't match the expectedOutSchematype (due to misconfiguration or version mismatch), this could lead to runtime type errors that TypeScript won't catch.Consider adding runtime validation or using a type guard to verify the schema structure before assignment:
const outSchema = record.metadata.outputSchema; // Optional: add runtime check if (outSchema !== undefined && !isValidOutputSchema(outSchema)) { throw new Error(`Invalid outputSchema in metadata for tool ${this.name}`); } this.outputSchema = outSchema as OutSchema;Alternatively, if runtime validation is too expensive, document the assumption that metadata is pre-validated before ToolInstance construction.
115-118: Type assertion toanyat line 116 bypasses type checking.Line 116 casts
this.outputSchematoanybefore passing it tobuildParsedToolResult, which completely bypasses TypeScript's type checking. This removes compile-time guarantees about the descriptor's structure.If
buildParsedToolResultis designed to handle the flexibleOutSchematype (which can be a string literal, Zod schema, raw shape, or array), consider updating its signature to acceptOutSchema | unknowninstead of requiring ananycast:// In tool.utils.ts export function buildParsedToolResult( descriptor: ToolOutputType | undefined, raw: unknown ): ParsedToolResult { // ... implementation }Then remove the cast:
override parseOutput(raw: Out | Partial<Out> | any): ParsedToolResult { - const descriptor = this.outputSchema as any; + const descriptor = this.outputSchema; return buildParsedToolResult(descriptor, raw); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/sdk/src/common/entries/tool.entry.ts(1 hunks)libs/sdk/src/tool/tool.instance.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/tool/tool.instance.tslibs/sdk/src/common/entries/tool.entry.ts
🧬 Code graph analysis (2)
libs/sdk/src/tool/tool.instance.ts (6)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(133-133)ToolOutputType(132-132)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/scope/scope.instance.ts (1)
hooks(124-126)libs/sdk/src/common/interfaces/tool.interface.ts (3)
input(86-88)input(90-97)ToolCtorArgs(20-26)libs/sdk/src/common/entries/tool.entry.ts (4)
ToolCallArgs(12-12)ToolCallExtra(13-15)ParsedToolResult(17-17)SafeTransformResult(18-18)libs/sdk/src/tool/tool.utils.ts (1)
buildParsedToolResult(185-267)
libs/sdk/src/common/entries/tool.entry.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (3)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)libs/sdk/src/common/decorators/tool.decorator.ts (2)
ToolInputOf(77-77)ToolOutputOf(126-130)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/plugin/plugin.registry.ts (1)
61-73: Dependency validation logic is good, but error message and presence check can be improvedThe new
buildGraphimplementation properly surfaces missing dependencies early, which is helpful. Two minor points:
- The error message says “Adapter … depends on …” even though this is in
PluginRegistry, which is likely confusing for users:
- Suggested tweak:
throw new Error(`Adapter ${tokenName(token)} depends on ${tokenName(d)}, which is not registered.`);
throw new Error(`Plugin ${tokenName(token)} depends on ${tokenName(d)}, which is not registered.`);
- Using
this.providers.get(d)purely as a presence check may:
- Instantiate providers earlier than before (if
getis eager), and- Not run the custom error branch at all if
getthrows on missing providers.If the
ProviderRegistryoffers a non-instantiating existence check (e.g.,has/hasProvider), that would be preferable here to avoid side effects and to guarantee your explicit error message is used.
🧹 Nitpick comments (4)
libs/json-schema-to-zod-v3/src/__tests__/security.config.spec.ts (1)
179-179: Useconstinstead ofletforconsoleWarnSpy.The variable is never reassigned; only methods are called on it.
Apply this diff:
- let consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();libs/sdk/src/plugin/plugin.registry.ts (1)
34-39: PluginRegistry now assumes ProviderRegistry has an active Scope
this.scope = providers.getActiveScope();makesPluginRegistrydependent on an activeScopein theProviderRegistry. This is reasonable for integrating plugin hooks with scope-level hooks, but it can break any callers that construct aPluginRegistrywith a provider tree that does not have an active scope configured.Consider either:
- Ensuring (and documenting) that
PluginRegistryis only ever constructed from scoped providers, or- Providing a clearer error path if no active scope is present (e.g., a tailored error message).
Please double-check all construction sites of
PluginRegistryto ensure an active scope is always available.libs/adapters/src/openapi/openapi.tool.ts (1)
80-80: Consider improving type safety.The
as anytype assertion bypasses type safety. While this may be necessary for dynamic schema conversion, consider narrowing the return type or adding runtime checks.Apply this diff to add more specific typing:
- return zodSchema as any; + return zodSchema as z.ZodObject<any>;This maintains flexibility while providing slightly better type information.
libs/plugins/src/cache/cache.plugin.ts (1)
111-124: Consider enhancing hashObject for edge cases.The current
hashObjectimplementation may produce inconsistent hashes or fail for:
- Arrays (order matters, but keys would be numeric indices)
- Dates (converted to strings via concatenation, may not be deterministic)
undefined/nullvalues in nested objects- Circular references (would cause infinite recursion)
If these edge cases are relevant to your use case, consider using a well-tested hashing library like
object-hashorhash-it:+import objectHash from 'object-hash'; @ToolHook.Will('execute', { priority: 1000 }) async willReadCache(flowCtx: FlowCtxOf<'tools:call-tool'>) { // ... - const hash = hashObject({ tool: tool.fullName, input: toolContext.input }); + const hash = objectHash({ tool: tool.fullName, input: toolContext.input }); // ... } @ToolHook.Did('execute', { priority: 1000 }) async willWriteCache(flowCtx: FlowCtxOf<'tools:call-tool'>) { // ... - const hash = hashObject({ tool: tool.fullName, input: toolContext.input }); + const hash = objectHash({ tool: tool.fullName, input: toolContext.input }); // ... } - -function hashObject(obj: any) { - // ... remove custom implementation -}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
eslint.config.mjs(1 hunks)libs/adapters/jest.config.ts(0 hunks)libs/adapters/package.json(1 hunks)libs/adapters/src/openapi/openapi.tool.ts(6 hunks)libs/json-schema-to-zod-v3/jest.config.ts(1 hunks)libs/json-schema-to-zod-v3/src/__tests__/security.config.spec.ts(1 hunks)libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts(1 hunks)libs/json-schema-to-zod-v3/src/security.ts(1 hunks)libs/plugins/package.json(1 hunks)libs/plugins/src/cache/cache.plugin.ts(2 hunks)libs/sdk/src/common/decorators/tool.decorator.ts(3 hunks)libs/sdk/src/plugin/plugin.registry.ts(6 hunks)libs/sdk/src/prompt/prompt.registry.ts(1 hunks)libs/sdk/src/resource/resource.registry.ts(1 hunks)libs/sdk/src/store/adapters/store.memory.adapter.ts(1 hunks)libs/sdk/src/tool/tool.registry.ts(15 hunks)libs/sdk/src/transport/flows/handle.sse.flow.ts(4 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(1 hunks)libs/sdk/src/transport/transport.local.ts(1 hunks)libs/sdk/src/utils/string.utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/adapters/jest.config.ts
✅ Files skipped from review due to trivial changes (3)
- libs/sdk/src/prompt/prompt.registry.ts
- libs/sdk/src/transport/transport.local.ts
- libs/sdk/src/resource/resource.registry.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sdk/src/tool/tool.registry.ts
- libs/json-schema-to-zod-v3/jest.config.ts
- libs/adapters/package.json
🧰 Additional context used
📓 Path-based instructions (1)
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/package.jsonlibs/sdk/src/store/adapters/store.memory.adapter.tslibs/plugins/src/cache/cache.plugin.tslibs/json-schema-to-zod-v3/src/__tests__/security.config.spec.tslibs/adapters/src/openapi/openapi.tool.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/json-schema-to-zod-v3/src/__tests__/security.spec.tslibs/sdk/src/utils/string.utils.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/json-schema-to-zod-v3/src/security.ts
🧬 Code graph analysis (8)
libs/plugins/src/cache/cache.plugin.ts (2)
libs/plugins/src/cache/cache.symbol.ts (1)
CacheStoreToken(4-4)libs/sdk/src/store/adapters/store.memory.adapter.ts (1)
ttl(73-78)
libs/json-schema-to-zod-v3/src/__tests__/security.config.spec.ts (1)
libs/json-schema-to-zod-v3/src/security.ts (6)
setSecurityConfig(257-259)DEFAULT_SECURITY_CONFIG(228-235)validatePattern(64-113)createSafeRegExp(135-205)createSafePatternValidator(280-312)getSecurityConfig(264-266)
libs/adapters/src/openapi/openapi.tool.ts (2)
libs/adapters/src/openapi/openapi.types.ts (1)
OpenApiAdapterOptions(65-65)libs/adapters/src/openapi/openapi.adapter.ts (1)
fetch(24-45)
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (2)
libs/json-schema-to-zod-v3/src/security.ts (6)
setSecurityConfig(257-259)DEFAULT_SECURITY_CONFIG(228-235)validatePattern(64-113)createSafeRegExp(135-205)createSafePatternValidator(280-312)getSecurityConfig(264-266)libs/json-schema-to-zod-v3/src/index.ts (7)
setSecurityConfig(58-58)DEFAULT_SECURITY_CONFIG(60-60)validatePattern(55-55)createSafeRegExp(56-56)createSafePatternValidator(57-57)getSecurityConfig(59-59)convertJsonSchemaToZod(79-79)
libs/sdk/src/plugin/plugin.registry.ts (1)
libs/sdk/src/scope/scope.instance.ts (3)
Scope(26-160)tools(140-142)providers(132-134)
libs/sdk/src/common/decorators/tool.decorator.ts (6)
libs/sdk/src/common/metadata/tool.metadata.ts (7)
ToolInputType(133-133)ToolOutputType(132-132)ToolMetadata(138-186)ImageOutputSchema(87-87)AudioOutputSchema(94-94)ResourceOutputSchema(101-101)ResourceLinkOutputSchema(108-108)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(86-88)input(90-97)apps/demo/src/apps/expenses/tools/create-expense.tool.ts (1)
Tool(4-27)apps/demo/src/apps/expenses/tools/add.tool.ts (1)
Tool(4-15)apps/demo/src/apps/calculator/tools/abs.tool.ts (1)
Tool(4-14)apps/demo/src/apps/employee-time/tools/admin-add-entry.tool.ts (1)
Tool(8-40)
libs/sdk/src/transport/flows/handle.sse.flow.ts (2)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (2)
plan(10-24)stateSchema(27-31)libs/sdk/src/scope/scope.instance.ts (1)
Scope(26-160)
libs/json-schema-to-zod-v3/src/security.ts (1)
libs/json-schema-to-zod-v3/src/index.ts (8)
PatternValidationResult(64-64)validatePattern(55-55)createSafeRegExp(56-56)PatternSecurityConfig(65-65)DEFAULT_SECURITY_CONFIG(60-60)setSecurityConfig(58-58)getSecurityConfig(59-59)createSafePatternValidator(57-57)
🪛 ast-grep (0.40.0)
libs/json-schema-to-zod-v3/src/security.ts
[warning] 75-75: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 143-143: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 170-170: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 ESLint
libs/json-schema-to-zod-v3/src/__tests__/security.config.spec.ts
[error] 179-179: 'consoleWarnSpy' is never reassigned. Use 'const' instead.
(prefer-const)
🔇 Additional comments (21)
libs/sdk/src/utils/string.utils.ts (1)
3-3: LGTM! Good refactor toconst.Changing
lettoconstis appropriate sincecleanedis never reassigned. This improves immutability and aligns with best practices.libs/sdk/src/store/adapters/store.memory.adapter.ts (1)
14-16: LGTM! Intent is now explicit.The explicit comment clarifies that the no-op is intentional, which is appropriate for an in-memory store adapter that requires no connection setup.
libs/json-schema-to-zod-v3/src/__tests__/security.spec.ts (1)
1-261: LGTM! Comprehensive ReDoS protection test suite.The test coverage is thorough and well-structured, validating all aspects of the security module:
- Pattern validation (safe, dangerous, length/quantifier limits, syntax errors)
- Safe RegExp creation with flags and timeout handling
- Pattern validators for both safe and unsafe patterns
- Security configuration (defaults, updates, immutability)
- Real-world attack patterns
- Integration with JSON Schema conversion
- Performance characteristics
The previously flagged
@ts-ignoreissue has been properly addressed with@ts-expect-errorand a descriptive comment.libs/json-schema-to-zod-v3/src/__tests__/security.config.spec.ts (1)
1-278: Excellent configuration wiring test coverage.The test suite thoroughly validates that
PatternSecurityConfigchanges propagate correctly through all security utilities and integration points, ensuring the configuration contract is honored at runtime.libs/json-schema-to-zod-v3/src/security.ts (4)
64-113: Config wiring has been properly implemented.The previously flagged issue regarding
PatternSecurityConfigbeing ignored has been correctly addressed. The implementation now:
- Reads
maxPatternLengthandmaxQuantifierfromglobalConfig(lines 66, 85)- Checks dangerous patterns after applying config constraints
- Properly validates syntax while respecting config limits
The static analysis warning about
new RegExp(pattern)at line 75 is a false positive—this try-catch block validates regex syntax before applying the security heuristics, which is the correct approach for a ReDoS protection module.
135-205: Config integration increateSafeRegExpis correct.The function now properly honors all
PatternSecurityConfigfields:
enableProtectioncontrols validation bypass (lines 141-151)throwOnUnsafeandwarnOnUnsafecontrol error/warning behavior (lines 158-164)timeoutMsfrom config is used as fallback (line 174)The static analysis warnings at lines 143 and 170 about constructing RegExp from variable input are false positives in this security context:
- Line 143 is in the intentional bypass path when protection is disabled
- Line 170 is only reached after
validatePatternhas approved the pattern
228-266: LGTM! Config management API is well-designed.The configuration surface provides:
- Immutable default config export
- Mutable global config with getter/setter
- Proper defensive copying in
getSecurityConfigto prevent external mutation- Clear JSDoc explaining usage patterns
280-312: Pattern validator correctly integrates with config.
createSafePatternValidatorproperly:
- Uses
createSafeRegExpwhich respects full config- Checks
globalConfig.warnOnUnsafebefore logging- Enforces timeout using
globalConfig.timeoutMsfor each validation- Returns a safe fallback (always false) for unsafe patterns
libs/sdk/src/plugin/plugin.registry.ts (3)
3-16: Imports and new dependencies align with usageAll newly added imports (
pluginDiscoveryDeps,tokenName,Ctor,normalizeProvider,Registry*,Scope,normalizeHooksFromCls) are used and consistent with the responsibilities ofPluginRegistry; no API surface change here.
75-97: Parallelizing tools/resources/prompts readiness is a sensible optimizationSwitching from sequential awaits to
await Promise.all([tools.ready, resources.ready, prompts.ready]);should reduce initialization time without changing semantics, assuming there are no ordering dependencies between these registries.Please confirm that none of
ToolRegistry,ResourceRegistry, orPromptRegistryrelies on another having finished initialization before its ownreadyresolves.
109-144: Exported providers, hook registration, and dynamic providers wiring look correct
Normalizing
rec.metadata.exportsvianormalizeProviderbefore callingproviders.getProviderInfomakes export resolution clearer and more robust.The new hook wiring:
const hooks = normalizeHooksFromCls(pluginInstance); if (hooks.length > 0) { await this.scope.hooks.registerHooks(false, ...hooks); }cleanly connects plugin instances into the scope-level hook system and uses the existing
Scope.hooksaccessor. This is a solid integration point.Using a
constfordynamicProvidersand guarding it before callingaddDynamicProvidersmaintains previous behavior with clearer intent.One point to verify: passing
falseas the first argument toregisterHooks—ensure that this “non-sticky” behavior (or whatever the flag represents) matches how you expect plugin-defined hooks to behave across the scope lifetime.libs/adapters/src/openapi/openapi.tool.ts (2)
4-4: The review comment is incorrect.
json-schema-to-zod-v3is an internal workspace package located atlibs/json-schema-to-zod-v3/, not an external npm dependency. It is already properly configured inpackage.jsonfiles and correctly exported fromlibs/json-schema-to-zod-v3/src/index.ts(line 79), with the import inopenapi.tool.tsresolving correctly via the tsconfig path alias. No issues exist with this dependency.Likely an incorrect or invalid review comment.
15-15: No issues found — this change is supported by design and not a breaking change.The tool decorator explicitly supports both
ZodRawShape(raw shape objects) andZodObjectasinputSchema. The type system (line 168 in tool.decorator.ts) is annotated asI | z.ZodObject<I>, and the helper function__AsZodObj(line 75) normalizes both forms for type inference. The runtime code (tool.instance.ts line 101) wrapsinputSchemainz.object()before calling.parse(), so raw shapes are properly handled. The change frominputSchematoinputSchema.shapealigns with the decorator's documented support forZodRawShape(see line 67–68 comment).libs/sdk/src/transport/flows/handle.sse.flow.ts (2)
56-83: LGTM! Router implementation and auth handling improvements.The functional changes look good:
- The
parseInputstage now properly reads authorization fromrequest[ServerRequestTokens.auth]before destructuring- The
routerstage now implements path-based routing logic to determinerequestType(previously incomplete)- These changes align with the similar implementation in
handle.streamable-http.flow.ts
101-109: Explicit failure for unimplemented elicitResult is correct.The
onElicitResultstage now explicitly fails with "Not implemented", which is the appropriate behavior for an unimplemented code path. This prevents requests from hanging and provides clear feedback.libs/plugins/src/cache/cache.plugin.ts (1)
56-108: LGTM! Previous critical issues properly addressed.The cache plugin refactoring correctly addresses the issues raised in previous reviews:
- ✅ Cache keys now include
tool.fullNameto prevent cross-tool collisions- ✅ Explicit
undefined/nullchecks allow falsy primitive values (0, false, '') to be cached correctly- ✅ Cache hit validation against tool output schema provides defensive integrity checking
The token-based storage pattern via
CacheStoreTokenprovides good abstraction over the backing store.libs/sdk/src/common/decorators/tool.decorator.ts (4)
35-61: LGTM! Handler and decorator types properly support primitive outputs.The updated type signatures correctly address previous limitations:
- ✅
FrontMcpToolExecuteHandlernow usesInSchema extends ToolInputTypeandOutSchema extends ToolOutputTypeinstead of constraining toobject- ✅
frontMcpToolgenerics properly leverageToolInputOf<T>andToolOutputOf<T>for inference- ✅ Tools can now return primitives, arrays, and special types (image, audio, resource, etc.)
73-170: Comprehensive output type inference system.The new inference helpers provide excellent coverage for the expanded output schema surface:
ToolInputOf<Opt>andToolOutputOf<Opt>provide clean public APIs for extracting types__InferFromSingleSchemahandles literals ('string', 'number', 'image', 'audio', etc.) and Zod schemas__InferFromArraySchemaenables tuple return types- The type system correctly handles ZodRawShape inputs
This is well-structured and maintainable.
189-232: Error helper types properly resolved.The branded error types correctly use
unknowninstead of{}for "OK" branches, resolving the@typescript-eslint/no-empty-object-typelint issues from previous reviews:
- ✅
__MustExtendCtxreturnsunknownwhen class extendsToolContext- ✅
__MustParamreturnsunknownfor exact matches- ✅
__MustReturnreturnsunknownwhen return type matchesThe error branches provide detailed, actionable messages for type mismatches.
243-275: Module augmentation overloads correctly handle optional outputSchema.The two decorator overloads provide appropriate type safety:
- Overload 1 (lines 248-259): When
outputSchemais provided, enforces strict return type checking via__MustReturn<C, ToolOutputOf<T>>- Overload 2 (lines 263-274): When
outputSchemais absent (outputSchema?: never), allows any return type while still checking input typesThis design gives developers flexibility while maintaining type safety where schemas are defined.
libs/plugins/package.json (1)
35-35: Documentation update verification needed.Version and security checks pass:
- Latest ioredis version is 5.8.2; your specified range
^5.8.0is safe and compatible- No security vulnerabilities found for ioredis
However, documentation updates require verification:
- The cache plugin documentation in
docs/draft/docs/guides/caching-and-cache-miss.mdxmentions Redis as a store option ("prefer Redis in production") but does not specifically mention ioredis or how to configure it as the Redis client- Per coding guidelines for
libs/**, public API dependencies should have matching documentation updates indocs/draft/docs/**Please confirm that documentation updates specifying ioredis configuration (e.g., how to pass a Redis client to the cache plugin) were included in this PR or are being addressed separately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/adapters/src/openapi/openapi.tool.ts (1)
56-56: Consider migrating to structured logging.The TODO comment indicates a need to migrate from
console.errorto a proper logger. Structured logging would improve observability and error tracking in production.Do you want me to open an issue to track the migration to a structured logging framework?
libs/sdk/src/plugin/plugin.registry.ts (1)
75-147: Ordering of dynamic provider registration relative to child registries and exports
initialize()now parallelizestools/resources/promptsreadiness viaPromise.all, which is fine since registration into the internal maps still happens after readiness resolves. The newrec.providershandling adds dynamic providers after:
plugins,adapters,tools,resources, andpromptshave been constructed and awaited ready, and- exported providers have already been merged back into the parent registry.
This ordering assumes dynamic providers are not required during child registry initialization or for any of the exported providers. If that assumption doesn’t hold (e.g., tools/adapters need dynamic providers during their own
readyphase, or dynamic providers should be exportable), you may want to moveaddDynamicProvidersto immediately afterproviders.readyso everything downstream sees them.For example:
const providers = new ProviderRegistry(rec.metadata.providers ?? [], this.providers); await providers.ready; + + const dynamicProviders = rec.providers; + if (dynamicProviders) { + await providers.addDynamicProviders(dynamicProviders); + } - const plugins = new PluginRegistry(providers, rec.metadata.plugins ?? []); + const plugins = new PluginRegistry(providers, rec.metadata.plugins ?? []); await plugins.ready; @@ - const hooks = normalizeHooksFromCls(pluginInstance); + const hooks = normalizeHooksFromCls(pluginInstance); if (hooks.length > 0) { await this.scope.hooks.registerHooks(false, ...hooks); } - pluginInstance.get = providers.get.bind(providers) as any; - const dynamicProviders = rec.providers; - if (dynamicProviders) { - await providers.addDynamicProviders(dynamicProviders); - } + pluginInstance.get = providers.get.bind(providers) as any; this.instances.set(token, pluginInstance);Please confirm the intended lifecycle of
rec.providersand whether this reordering would better match expectations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
eslint.config.mjs(2 hunks)libs/adapters/src/openapi/openapi.tool.ts(6 hunks)libs/sdk/src/flows/flow.registry.ts(1 hunks)libs/sdk/src/plugin/plugin.registry.ts(7 hunks)libs/sdk/src/scope/scope.registry.ts(1 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/sdk/src/scope/scope.registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/transport/flows/handle.streamable-http.flow.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/adapters/src/openapi/openapi.tool.tslibs/sdk/src/flows/flow.registry.tslibs/sdk/src/plugin/plugin.registry.ts
🧬 Code graph analysis (3)
libs/adapters/src/openapi/openapi.tool.ts (2)
libs/adapters/src/openapi/openapi.types.ts (2)
OpenApiAdapterOptions(65-65)BaseOptions(5-48)libs/adapters/src/openapi/openapi.adapter.ts (3)
fetch(24-45)parseTools(47-51)OpenapiAdapter(15-52)
libs/sdk/src/flows/flow.registry.ts (1)
libs/sdk/src/auth/auth.registry.ts (1)
buildGraph(66-78)
libs/sdk/src/plugin/plugin.registry.ts (2)
libs/sdk/src/common/interfaces/internal/registry.interface.ts (1)
PluginRegistryInterface(92-94)libs/sdk/src/scope/scope.instance.ts (3)
Scope(26-160)tools(140-142)providers(132-134)
⏰ 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/flows/flow.registry.ts (1)
39-41: Clarified dependency error message for flows looks goodThe updated error text correctly refers to a missing Flow dependency and matches the style used in other registries (e.g.,
AuthProvider ... depends on ... which is not registered.). There’s no behavioral or API impact here, only clearer diagnostics, which is appropriate for the SDK surface.eslint.config.mjs (2)
11-27: Good compromise by makingno-explicit-anya global warning instead of disabling it.Changing
@typescript-eslint/no-explicit-anyto'warn'here restores type-safety visibility without breaking builds, which is a solid middle ground compared to a globaloff.
30-42: Scoped override for json-schema-to-zod and rule precedence look correct.Limiting
no-explicit-any: 'off'tolibs/json-schema-to-zod-v3/**/*.tswhile keeping it at'warn'elsewhere addresses the earlier global-suppression concern and leverages flat-config ordering correctly: files in that library will see the scopedoff, and all other files retain the warning, with the final block only affectingno-unsafe-function-type.libs/adapters/src/openapi/openapi.tool.ts (2)
90-90: Previous concern resolved: hardcoded query parameter removed.The hardcoded
v: '1'query parameter flagged in the previous review has been successfully removed. TheURLSearchParamsis now initialized cleanly without any default parameters.
15-15: The review comment is incorrect. The change toinputSchema.shapeis not breaking.The codebase already uses this pattern. In
tool.instance.ts:49,this.inputSchemais explicitly stored as a shape object:this.inputSchema = schema instanceof z.ZodObject ? schema.shape : schema ?? {};. Consumers then reconstruct the ZodObject when needed:z.object(this.inputSchema)(line 101).The change to
openapi.tool.ts:15aligns with this existing pattern—extracting.shapeensures consistency across tool implementations. The downstream code that usestool.inputSchemaexpects a shape object, not a ZodObject instance.Likely an incorrect or invalid review comment.
libs/sdk/src/plugin/plugin.registry.ts (3)
34-39: Scope dependency and hook registration assumptions
PluginRegistrynow capturesscopeviaproviders.getActiveScope()and unconditionally callsthis.scope.hooks.registerHooks(...). If aProviderRegistrycan exist without an activeScope(e.g., in tests, CLI/global contexts, or before scope initialization), this will throw at construction or duringinitialize(). Please confirm all call sites guarantee an activeScope, or consider guarding this path / makingScopean explicit dependency where needed.Also applies to: 136-139
41-43: getPlugins() API looks correct; ensure docs reflect the new public surface
getPlugins()cleanly exposesPluginEntry[]from the internalinstancesmap and matchesPluginRegistryInterface; no functional concerns. Since this sits underlibs/sdk, please also ensure this new public method is documented underdocs/draft/docs/**so SDK consumers know it’s available. As per coding guidelines
58-69: buildMap/buildGraph changes preserve behavior with clearer error messagingReturning
{ tokens, defs, graph }frombuildMapmatches the expectedRegistryBuildMapResultshape, and the updated error now clearly reportsPlugin ${tokenName(token)} depends on ${tokenName(d)}..., tying failures directly to plugin tokens. This improves debuggability without changing semantics.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
Chores