feat: Add support for multiple openapi adapters and with tool list pagination #208
Conversation
… update logic for existing comments
📝 WalkthroughWalkthroughAdds cursor-based, configurable pagination for tool lists and introduces adapter name deduplication with per-instance Symbol tokens to allow multiple adapters of the same class while preventing duplicate registrations. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Scope
participant ToolsListFlow
participant Metadata as Metadata/Config
participant Tools as Tool Repository
Client->>Scope: listTools(cursor?)
Scope->>Metadata: read pagination config
Metadata-->>Scope: pagination options
Scope->>ToolsListFlow: execute list flow with config
ToolsListFlow->>Tools: fetch all tools
Tools-->>ToolsListFlow: all resolved tools
ToolsListFlow->>ToolsListFlow: shouldPaginate(count, config)
alt Pagination Enabled
ToolsListFlow->>ToolsListFlow: parseCursor(cursor) → offset
ToolsListFlow->>ToolsListFlow: sort tools by finalName
ToolsListFlow->>ToolsListFlow: slice [offset:offset+pageSize]
ToolsListFlow->>ToolsListFlow: encodeCursor(nextOffset) if more exist
else Pagination Disabled
ToolsListFlow->>ToolsListFlow: return all tools
end
ToolsListFlow-->>Scope: tools + nextCursor?
Scope-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
🧹 Nitpick comments (3)
libs/sdk/src/common/dynamic/dynamic.adapter.ts (1)
12-13: Consider reducinganyusage per coding guidelines.Per coding guidelines,
anytypes should be avoided without strong justification. While the use here is in internal type definitions that bridge TypeScript's type inference limitations for factory patterns, consider adding inline comments explaining whyanyis necessary for these specific cases.Also applies to: 18-18
libs/sdk/src/scope/scope.instance.ts (1)
361-367: LGTM! Clean accessor for pagination configuration.The getter appropriately exposes the pagination configuration from metadata. The JSDoc clearly documents its purpose.
Consider adding an explicit return type annotation for API clarity:
♻️ Optional: Add explicit return type
/** * Get pagination configuration for list operations. * Returns the parsed pagination config from @FrontMcp metadata. */ - get pagination() { + get pagination(): PaginationOptions | undefined { return this.metadata.pagination; }This would require importing
PaginationOptionsfrom'../common'if not already available.libs/sdk/src/common/types/options/pagination.options.ts (1)
68-72: Consider making DEFAULT_TOOL_PAGINATION readonly.To prevent accidental mutation of the default values:
♻️ Optional: Use const assertion or Object.freeze
-export const DEFAULT_TOOL_PAGINATION: Required<ToolPaginationOptions> = { +export const DEFAULT_TOOL_PAGINATION: Readonly<Required<ToolPaginationOptions>> = Object.freeze({ mode: 'auto', pageSize: 40, autoThreshold: 40, -}; +});Or use
as constfor a simpler approach:-export const DEFAULT_TOOL_PAGINATION: Required<ToolPaginationOptions> = { +export const DEFAULT_TOOL_PAGINATION = { mode: 'auto', pageSize: 40, autoThreshold: 40, -}; +} as const satisfies Required<ToolPaginationOptions>;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/sdk/src/common/dynamic/dynamic.adapter.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/types/options/index.tslibs/sdk/src/common/types/options/pagination.options.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/tool/flows/tools-list.flow.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript mode with noanytypes without strong justification
Avoid non-null assertions (!) - use proper error handling and type guards instead
Use@frontmcp/utilsfor cryptographic operations instead ofnode:cryptodirectly
Use@frontmcp/utilsfor file system operations instead offs/promisesornode:fsdirectly
Files:
libs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/types/options/index.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/types/options/pagination.options.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/common/dynamic/dynamic.adapter.ts
libs/{sdk,adapters}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters}/**/*.{ts,tsx}: Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities
Files:
libs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/types/options/index.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/types/options/pagination.options.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/common/dynamic/dynamic.adapter.ts
libs/sdk/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript, avoidanytypes
Use type parameters with constraints instead of unconstrained generics withanydefaults
Validate URIs per RFC 3986 at metadata level usingisValidMcpUrirefinement
UsechangeScopeinstead ofscopein change event properties to avoid confusion with Scope class
Fail fast on invalid hook flows by validating hooks match their entry type
Centralize record types in common/records and import from there, not from module-specific files
Return strictly typed MCP protocol responses (e.g.,Promise<GetPromptResult>) instead ofPromise<unknown>for MCP entry methods
Useunknowninstead ofanyfor generic type parameter defaults (not for MCP protocol types)
Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Validate inputs and outputs through parseOutput/safeParseOutput methods in validation flows
Files:
libs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/types/options/index.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/types/options/pagination.options.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/common/dynamic/dynamic.adapter.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/types/options/index.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/types/options/pagination.options.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/common/dynamic/dynamic.adapter.ts
🧠 Learnings (8)
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to libs/*/src/index.ts : Export everything users need through barrel exports (index.ts), do not add backwards compatibility aliases
Applied to files:
libs/sdk/src/common/types/options/index.ts
📚 Learning: 2026-01-04T14:35:18.366Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.366Z
Learning: Applies to libs/uipack/**/index.{ts,js} : Export all public APIs through appropriate entry points (frontmcp/uipack, frontmcp/uipack/adapters, frontmcp/uipack/theme, etc.)
Applied to files:
libs/sdk/src/common/types/options/index.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to libs/{sdk,adapters}/**/*.{ts,tsx} : Use `getCapabilities()` for dynamic capability exposure instead of hardcoding capabilities
Applied to files:
libs/sdk/src/common/types/options/index.tslibs/sdk/src/common/dynamic/dynamic.adapter.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Extend tool metadata using `declare global` pattern to allow tools to specify plugin-specific options in their decorators
Applied to files:
libs/sdk/src/common/types/options/index.tslibs/sdk/src/common/types/options/pagination.options.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*plugin.ts : Extend ExecutionContextBase with plugin-specific properties using module declaration (`declare module 'frontmcp/sdk'`) combined with `contextExtensions` in plugin metadata
Applied to files:
libs/sdk/src/common/metadata/front-mcp.metadata.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to libs/sdk/**/*.{ts,tsx} : Use `changeScope` instead of `scope` in change event properties to avoid confusion with Scope class
Applied to files:
libs/sdk/src/scope/scope.instance.ts
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*plugin.ts : Plugins should extend `DynamicPlugin<Options, OptionsInput>` for configurable behavior, with `Plugin` decorator specifying name, description, and static providers
Applied to files:
libs/sdk/src/common/dynamic/dynamic.adapter.ts
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Applies to libs/sdk/**/*.{ts,tsx} : Return strictly typed MCP protocol responses (e.g., `Promise<GetPromptResult>`) instead of `Promise<unknown>` for MCP entry methods
Applied to files:
libs/sdk/src/common/dynamic/dynamic.adapter.ts
🧬 Code graph analysis (4)
libs/sdk/src/common/tokens/front-mcp.tokens.ts (1)
libs/sdk/src/common/tokens/base.tokens.ts (1)
tokenFactory(9-9)
libs/sdk/src/common/metadata/front-mcp.metadata.ts (2)
libs/sdk/src/common/types/options/pagination.options.ts (2)
PaginationOptions(42-48)paginationOptionsSchema(61-63)libs/ui/src/components/table.schema.ts (1)
PaginationOptions(152-152)
libs/sdk/src/tool/flows/tools-list.flow.ts (3)
libs/sdk/src/common/types/options/pagination.options.ts (2)
ToolPaginationOptions(14-36)DEFAULT_TOOL_PAGINATION(68-72)libs/sdk/src/errors/mcp.error.ts (1)
InvalidInputError(227-244)libs/sdk/src/scope/scope.instance.ts (1)
Scope(37-392)
libs/sdk/src/common/dynamic/dynamic.adapter.ts (1)
libs/sdk/src/common/interfaces/adapter.interface.ts (2)
AdapterType(29-33)AdapterInterface(8-16)
⏰ 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). (4)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
- GitHub Check: Package E2E (Verdaccio)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
libs/sdk/src/common/dynamic/dynamic.adapter.ts (3)
24-25: LGTM! Good approach for per-class adapter name tracking.Using a
WeakMapkeyed by the adapter class is appropriate here - it allows garbage collection when the class is no longer referenced while still providing the per-class isolation needed for duplicate detection.
52-61: Good validation for requirednameoption.The validation correctly checks for empty strings (including whitespace-only), and the error message clearly explains the requirement.
79-82: Symbol.for() usage is appropriate and collision risk is mitigated.The code already protects against name collisions through:
- Per-class validation (lines 64-77):
usedAdapterNamesWeakMap ensures duplicate names within the same adapter class are rejected at registration time- Composite key design (line 82): The symbol key includes both the class name (
this.name) and the validated unique instance name (adapterName), making cross-class collisions impossibleSymbol.for() is the correct choice here because it ensures stable identity across module boundaries—the documented intent. Using a local Symbol() instead would break cross-module stability and defeat the purpose. Process-lifetime persistence of the global registry entry is expected and appropriate for this use case.
libs/sdk/src/common/tokens/front-mcp.tokens.ts (1)
27-28: LGTM! Consistent token addition.The pagination token follows the established pattern for other metadata tokens in this file.
libs/sdk/src/common/metadata/front-mcp.metadata.ts (2)
90-95: LGTM! Well-documented optional pagination configuration.The JSDoc clearly explains that currently only tool list pagination is supported, and the optional typing maintains backward compatibility.
112-112: Schema addition is consistent with interface.The schema correctly mirrors the interface's optional
paginationfield.libs/sdk/src/common/types/options/index.ts (1)
8-8: LGTM! Consistent barrel export.The export follows the established pattern for other options modules, correctly exposing the new pagination options through the public API.
libs/sdk/src/tool/flows/tools-list.flow.ts (5)
88-109: LGTM! Clean pagination mode logic.The
shouldPaginatemethod correctly handles all three modes ('auto',true,false) and falls back to defaults appropriately. The logic is easy to follow and well-commented.
117-138: Good cursor parsing with proper error handling.The
parseCursormethod correctly:
- Uses
InvalidInputError(per coding guidelines for specific error classes)- Validates offset is a non-negative integer
- Provides clear error messages
- Re-throws
InvalidInputErrorwithout wrappingOne edge case to consider:
Number.isInteger(offset)will returnfalseforNaN, but thetypeof offset !== 'number'check handles this first.
329-331: Good choice to sort for deterministic pagination.Sorting by
finalNameensures consistent ordering across paginated requests, which is essential for cursor-based pagination. ThelocaleCompareis appropriate for string sorting.Note: If tools are added/removed between paginated requests, clients may see duplicates or miss items. This is inherent to offset-based pagination. Consider documenting this behavior.
520-525: LGTM! Conditional nextCursor in response.The response correctly includes
nextCursoronly when more results are available, avoiding unnecessary empty fields.
342-347: Current offset validation is acceptable for MCP pagination.The check
offset > sortedResolved.lengthcorrectly allowsoffset === sortedResolved.length, which returns an empty page with nonextCursor. This matches MCP cursor pagination conventions—the absence ofnextCursorsignals clients that no more results exist. This is standard pagination behavior and not confusing; clients already expect empty results when requesting beyond the dataset bounds.No change needed.
libs/sdk/src/common/types/options/pagination.options.ts (2)
1-72: LGTM! Well-structured pagination options module.The module is well-designed with:
- Clear TypeScript interfaces following the coding guideline to prefer
interfacefor object shapes- Consistent documentation across interfaces and the constant
- Appropriate Zod validation (positive integers for numeric fields)
- Default values that align between the schema and the
DEFAULT_TOOL_PAGINATIONconstant
51-58: Verify Zod 4 default behavior with optional fields.In Zod 4, the combination of
.optional().default()creates a schema where:
- Input can be
undefined(due to.optional())- Output will have the default applied when
undefinedThis means
toolPaginationOptionsSchema.parse({})will return{ mode: 'auto', pageSize: 40, autoThreshold: 40 }.However, in
paginationOptionsSchema(line 62),toolsuses.optional()without.default(), so iftoolsis omitted, it staysundefinedrather than getting default values. This is likely intentional (tools pagination is opt-in), but verify this matches the expected behavior intools-list.flow.tswherepaginationConfigmay beundefined.
⏹️ PR Testing Registry StoppedThe temporary npm registry for this PR has been shut down.
To start a new registry, trigger the workflow again. |
…ublishable projects and updating output messages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pr-testing-registry.yml (1)
251-265: Consider handlingnx show projectsfailures more explicitly.Redirecting stderr to
/dev/nullon line 253 will silently swallow any Nx errors (e.g., misconfigurednx.json, missing targets). If the command fails,PUBLISHABLE_PROJECTSbecomes empty and the loop is skipped without warning, potentially causing confusion when no packages are published.🔧 Suggested improvement to surface errors
# Dynamically discover all publishable packages using Nx echo "Discovering publishable packages..." - PUBLISHABLE_PROJECTS=($(npx nx show projects --with-target publish 2>/dev/null)) + if ! PUBLISHABLE_OUTPUT=$(npx nx show projects --with-target publish 2>&1); then + echo "::warning::Failed to discover publishable projects: $PUBLISHABLE_OUTPUT" + fi + read -ra PUBLISHABLE_PROJECTS <<< "$PUBLISHABLE_OUTPUT" echo "Found ${#PUBLISHABLE_PROJECTS[@]} publishable packages: ${PUBLISHABLE_PROJECTS[*]}" + + if [ ${#PUBLISHABLE_PROJECTS[@]} -eq 0 ]; then + echo "::warning::No publishable projects found. Verify 'publish' target exists in project configurations." + fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-testing-registry.yml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Nx-based monorepo build system - use commands like `nx build sdk`, `nx test ast-guard`, `nx run-many -t test`
Applied to files:
.github/workflows/pr-testing-registry.yml
📚 Learning: 2026-01-06T02:34:55.689Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/plugins/CLAUDE.md:0-0
Timestamp: 2026-01-06T02:34:55.689Z
Learning: Applies to libs/plugins/**/*.ts : Use proper ES module imports instead of `require()` for SDK imports; avoid dynamic require of `frontmcp/sdk` modules
Applied to files:
.github/workflows/pr-testing-registry.yml
📚 Learning: 2026-01-06T17:16:04.304Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:16:04.304Z
Learning: Use yarn as the package manager for the monorepo
Applied to files:
.github/workflows/pr-testing-registry.yml
⏰ 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). (15)
- GitHub Check: E2E Tests (demo-e2e-multiapp)
- GitHub Check: E2E Tests (demo-e2e-serverless)
- GitHub Check: E2E Tests (demo-e2e-hooks)
- GitHub Check: E2E Tests (demo-e2e-config)
- GitHub Check: E2E Tests (demo-e2e-errors)
- GitHub Check: E2E Tests (demo-e2e-codecall)
- GitHub Check: E2E Tests (demo-e2e-redis)
- GitHub Check: E2E Tests (demo-e2e-openapi)
- GitHub Check: E2E Tests (demo-e2e-public)
- GitHub Check: E2E Tests (demo-e2e-ui)
- GitHub Check: E2E Tests (demo-e2e-cache)
- GitHub Check: Unit Tests
- GitHub Check: Lint & Format Checks
- GitHub Check: Package E2E (Verdaccio)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
.github/workflows/pr-testing-registry.yml (2)
267-282: LGTM!The package name extraction with jq's
// emptyoperator and fallback logic is well-implemented. The publishing command uses appropriate flags for the local Verdaccio registry.
347-355: LGTM!The documentation updates accurately reflect the shift to dynamic package discovery while maintaining helpful concrete examples for users.
Also applies to: 474-483
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
CI
✏️ Tip: You can customize this high-level summary in your review settings.