feat: Add support for skills #221
Conversation
📝 WalkthroughWalkthroughAdds a Skills subsystem (metadata, decorators, records, registry, providers, flows, sessions, guards), an e2e-skills app with many tests, CI workflow changes for dynamic E2E discovery and per-project coverage, multiple demo app logging/test tweaks, and various docs and helper additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/LLM
participant Scope as Scope/Server
participant SearchFlow as SearchSkillsFlow
participant Registry as SkillRegistry
participant Provider as SkillProvider
participant Validator as SkillToolValidator
Client->>Scope: skills/search(query)
Scope->>SearchFlow: execute(input)
SearchFlow->>Registry: search(options)
Registry->>Provider: fetch/index query
Provider-->>Registry: results (skills + scores)
Registry-->>SearchFlow: mapped results
SearchFlow->>Validator: validateToolAvailability(results)
Validator-->>SearchFlow: available/missing tools
SearchFlow-->>Scope: SearchSkillsResult
Scope-->>Client: response
sequenceDiagram
participant Client as Client/LLM
participant Scope as Scope/Server
participant LoadFlow as LoadSkillFlow
participant Registry as SkillRegistry
participant Provider as SkillProvider
participant Session as SkillSessionManager
participant Guard as ToolAuthorizationGuard
Client->>Scope: skills/load(skillId, activate?)
Scope->>LoadFlow: execute(input)
LoadFlow->>Registry: load(skillId)
Registry->>Provider: load content
Provider-->>Registry: SkillContent
alt activateSession
LoadFlow->>Session: activateSkill(skillId, content, policy?)
Session-->>LoadFlow: session metadata
end
LoadFlow-->>Scope: LoadSkillResult (skill + tools + session?)
Scope-->>Client: response
Client->>Scope: tools:call(toolName)
Scope->>Guard: check(toolName)
Guard->>Session: getActiveSession()
Session-->>Guard: SkillSessionState
Guard-->>Scope: allowed? / error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/live/docs/extensibility/plugins.mdx (1)
17-24: Revert edits in docs/live/.
Per repo rules, live docs are generated and should not be edited directly. Please revert these changes and keep the update in docs/draft so automation can regenerate live docs.As per coding guidelines, ...
Also applies to: 242-296
docs/live/docs/servers/apps.mdx (1)
52-85: Update the draft app docs instead of live output.
Changes underdocs/live/docs/**should be made indocs/draft/docs/**and regenerated. Please move this update to the draft app docs and revert the live edit. As per coding guidelines, avoid manual edits to live docs.
🤖 Fix all issues with AI agents
In @.github/workflows/push.yml:
- Around line 123-152: The discover-e2e job lacks explicit permissions; add a
minimal permissions block under the discover-e2e job (job name "discover-e2e")
such as permissions: contents: read to follow least-privilege principles —
insert this directly beneath the job declaration so the job only has repository
read access it needs; optionally mirror or add equivalent permissions at
workflow level or to other jobs for consistency.
In `@docs/live/docs/servers/skills.mdx`:
- Around line 1-5: Revert the live MDX change for the document with title
"Skills" and slug "servers/skills" in the generated live docs, and instead apply
your edits to the draft source (the draft copy of the Skills doc); after
updating the draft, run the documentation regeneration pipeline so the live docs
are rebuilt from draft sources and the live file is no longer modified directly.
In `@libs/auth/jest.config.cts`:
- Around line 12-19: Update the global coverageThreshold values in
jest.config.cts (the coverageThreshold -> global block: statements, branches,
functions, lines) to 95 instead of lowering them; if there are known gaps that
must be excluded, add explicit, justified exclusions via
coveragePathIgnorePatterns or per-file/per-directory coverageThreshold overrides
and create a follow-up ticket documenting those exclusions and remediation
steps; also add a TODO comment referencing the ticket ID so reviewers can trace
the justification.
In `@libs/sdk/src/common/metadata/skill.metadata.ts`:
- Around line 305-333: The instruction URL branch in
skillInstructionSourceSchema currently uses z.string().url(), which doesn't
enforce RFC 3986 MCP URI rules; import isValidMcpUri from '@frontmcp/utils' and
replace the z.object({ url: z.string().url() }).strict() entry with a refinement
using z.string().refine(isValidMcpUri, { message: 'Invalid MCP URI' }) (keeping
.strict()), so skillInstructionSourceSchema (and thus skillMetadataSchema)
validates MCP URIs the same way as resource.metadata.ts and app.metadata.ts.
- Around line 385-424: In normalizeToolRef replace the three plain Error throws
with new PublicMcpError(MCP_ERROR_CODES.INVALID_PARAMS, ...) so invalid tool
class and invalid reference format are surfaced as parameter validation errors;
import { PublicMcpError, MCP_ERROR_CODES } from 'libs/sdk/src/errors', and use
descriptive messages in each throw (where getToolNameFromClass returns falsy and
in the final invalid reference fallback) — update the throws inside the function
(the branches using getToolNameFromClass and the final JSON.stringify(ref)
throw) to use PublicMcpError instead of Error.
In `@libs/sdk/src/common/tokens/skill.tokens.ts`:
- Around line 1-4: The TypeScript error is caused by a missing import for
ExtendFrontMcpSkillMetadata referenced in the satisfies clause; update the top
imports in skill.tokens.ts to include ExtendFrontMcpSkillMetadata (alongside
tokenFactory, RawMetadataShape, and SkillMetadata), importing it from the module
that exports it (e.g., ../metadata), so the satisfies check compiles correctly
when using tokenFactory and the SkillMetadata/RawMetadataShape types.
In `@libs/sdk/src/direct/__tests__/direct-server.spec.ts`:
- Around line 18-33: The tests currently cast the mock scope to any when
constructing DirectMcpServerImpl; instead define createMockScope to return a
properly typed partial scope (e.g., const createMockScope = (): Pick<Scope,
'runFlowForOutput' | 'transportService'> => ({ runFlowForOutput: jest.fn(),
transportService: { destroy: jest.fn().mockResolvedValue(undefined) } });) and
remove all as any casts, then pass the result directly into new
DirectMcpServerImpl(mockScope) so runFlowForOutput and transportService are
correctly typed for the tests.
In `@libs/sdk/src/elicitation/helpers/fallback.helper.ts`:
- Around line 103-177: The subscribeFallbackResult promise can resolve before
the timeout/callback, leaving no unsubscribe reference and causing a
subscription leak; change handleWaitingFallback to capture the unsubscribe
function into a local variable (e.g., let unsubscribeFn: (() => Promise<void>) |
null = null) when the .then callback runs, and call unsubscribeFn() from every
resolution path (timeout handler, fallback callback branch where resolved is
set, and the subscription .catch branch) after clearing timeoutHandle; also
ensure unsubscribe errors are caught/ignored and that unsubscribe is awaited or
its promise is handled to guarantee cleanup.
In `@libs/sdk/src/front-mcp/front-mcp.ts`:
- Around line 251-271: The wrapper for MCP handlers currently clobbers any
existing authInfo; update the wrappedHandler created around handler.handler
(from createMcpHandlers/handlers) to merge existing ctx.authInfo with the new
sessionId instead of overwriting it (e.g., const enrichedCtx = { ...ctx,
authInfo: { ...ctx.authInfo, sessionId } }); also remove the gratuitous "any"
annotations on the wrappedHandler parameters and use the proper handler/request
context types expected by mcpServer.setRequestHandler so TypeScript
type-checking is preserved.
In `@libs/sdk/src/index.ts`:
- Around line 61-98: The SDK index now exports developer-facing Skill APIs that
lack documentation; update the docs to add an API reference section documenting
SkillRegistry, SkillInstance, createSkillInstance, SkillEmitter,
MemorySkillProvider, SkillToolValidator and the utility functions
normalizeSkill, isSkillRecord, and formatSkillForLLM, describing their purpose,
signatures, returned types (e.g., SkillRegistryInterface, IndexedSkill,
SkillLoadResult, ToolValidationResult), and example usage for
creating/instantiating skills, registering/listening with the registry, using
the memory provider, and validating tools so integrators can use the
programmatic SDK surface.
In `@libs/sdk/src/skill/__tests__/skill-validator.spec.ts`:
- Around line 26-43: The mock returned by createMockToolRegistry should include
the missing getToolsForListing method to satisfy the ToolRegistryInterface
contract; update createMockToolRegistry to add a getToolsForListing stub
(returning the same list shape as getTools or an empty array) so the object
matches ToolRegistryInterface without relying on the as unknown as cast, and
keep other jest.fn() stubs intact; reference createMockToolRegistry and
ToolRegistryInterface and ensure the added getToolsForListing signature aligns
with how SkillToolValidator would expect it.
In `@libs/sdk/src/skill/flows/__tests__/search-skills.flow.spec.ts`:
- Line 14: The test file contains an unused import of SearchSkillsFlow; remove
the unused import statement for SearchSkillsFlow (import SearchSkillsFlow from
'../search-skills.flow') to silence the lint/test warning, or alternatively add
tests that instantiate/exercise the SearchSkillsFlow class (e.g., call its
constructor or relevant methods) so the import is actually used; update the test
file accordingly to either delete the import or reference the SearchSkillsFlow
symbol.
In `@libs/sdk/src/skill/flows/load-skill.flow.ts`:
- Around line 78-86: The stateSchema currently uses z.any() for loadResult and
activationResult which violates the no-any guideline; replace those with proper
Zod schemas (e.g., define skillLoadResultSchema and skillActivationResultSchema
matching SkillLoadResult and SkillActivationResult and use them in stateSchema)
or, if you cannot fully model the types yet, change to z.unknown().optional()
for the loadResult and activationResult fields (instead of z.any()) and keep the
existing type assertions removed; update references in stateSchema and ensure
output still uses outputSchema.
In `@libs/sdk/src/skill/guards/tool-authorization.guard.ts`:
- Around line 95-119: The approval branch must not fall through to the generic
denial path: when result.requiresApproval is true and opts.onApprovalRequired is
absent, explicitly handle it by throwing ToolApprovalRequiredError(toolName,
result.skillId) if opts.throwOnDenied is true, otherwise return result as-is
(preserving requiresApproval). Also when opts.onApprovalRequired returns
approved, update the returned object to set allowed: true, reason:
'dynamically_approved' AND clear requiresApproval (set to false) before
returning, and ensure the sessionManager.approveToolForSession/tool call
remains. This avoids reaching the ToolNotAllowedError(result, allowedTools)
branch for approval-required cases.
In `@libs/sdk/src/skill/index.ts`:
- Around line 37-40: Add API-level documentation for the exported registry
surface: document SkillRegistry, SkillRegistryInterface, IndexedSkill, and
SkillRegistryOptions in the docs/draft/docs site (e.g., extend
docs/draft/docs/servers/skills.mdx or add a new docs page). Describe each
exported type/class signature at a high level, list required/optional properties
and methods on SkillRegistryInterface, show typical usage patterns (implementing
a custom registry, registering/unregistering skills, indexing behavior), and
include example snippets and use-case notes for extending SkillRegistry to
ensure SDK consumers can implement or extend registries.
In `@libs/sdk/src/skill/providers/external-skill.provider.ts`:
- Around line 336-346: In syncSkills, ensure the provider is initialized before
using persisted state and replace the generic Error with a typed SDK error: if
this.mode !== 'persistent' throw new SdkError(...) (or the project’s
SDK-specific error class) instead of new Error; then before computing prevState,
if this.syncState is undefined call await this.initialize() and then set
prevState = this.syncState ?? createEmptySyncState() so persisted sync state is
respected (keep references to syncSkills, initialize, syncState,
createEmptySyncState and mode).
In `@libs/sdk/src/skill/providers/memory-skill.provider.ts`:
- Around line 495-512: In skillToMetadata, the current tools mapping drops the
tool.required flag when t.purpose is undefined; update the mapping in the tools
property so each mapped entry always preserves t.required (e.g., include
required: t.required when present) while still using the compact { name, purpose
} shape when purpose exists — ensure the transformed items for tools in
skillToMetadata keep required semantics so downstream code relying on required
continues to work.
In `@libs/sdk/src/skill/README.md`:
- Around line 28-41: The README.md has fenced code blocks lacking language
identifiers (MD040); edit the README.md blocks that render the ASCII diagrams
(the "SkillRegistry (Main Facade)" diagram, the "Session State" block, and the
"libs/sdk/src/ skill tree" block) and add a language identifier such as "text"
(e.g., replace ``` with ```text) so each fenced block is tagged; locate these
blocks by the diagram content or headings (SkillRegistry, Session State, and the
libs/sdk/src/ tree) and update them consistently throughout the file.
In `@libs/sdk/src/skill/session/skill-session-store.interface.ts`:
- Around line 52-125: MemorySkillSessionStore is leaking mutable references
because activeSkills and the nested Sets inside each ActiveSkill are
stored/returned by reference; update save, get, update, and listActive to
deep-clone activeSkills and every nested Set. Specifically, in save and when
persisting from update ensure you copy session.activeSkills as a new array and
for each skill clone its properties and convert
allowedTools/requiredTools/approvedTools/deniedTools into new Set instances
before storing; in get and listActive return deep clones the same way (new
array, new skill objects, new Sets) so consumers cannot mutate the store; in
update also detect updates.activeSkills and replace with a deep-cloned version
before Object.assign into the stored session.
In `@libs/sdk/src/skill/skill-storage.factory.ts`:
- Around line 219-227: The switch branch handling providerType === 'external' in
createSkillStorageProvider currently throws a generic Error when
externalProvider is missing; replace that with the SDK's typed error (e.g.,
SdkError or McpError) populated with the appropriate MCP error code and a clear
message. Update the throw inside the 'external' case so it constructs and throws
the SDK-specific error class (including the same descriptive text and the MCP
code), and keep the existing logger?.error call; reference
createSkillStorageProvider, providerType 'external', externalProvider, and
ExternalSkillProviderBase when implementing the fix.
🧹 Nitpick comments (39)
libs/cli/src/__tests__/env.spec.ts (1)
292-302: Consider a more explicit singular form assertion.The assertion on line 301 works but is subtle. A clearer approach would be to match the exact expected output format or use a regex to ensure
variableappears without anssuffix.♻️ Suggested improvement
it('should use singular "variable" for count of 1', () => { (fs.existsSync as jest.Mock).mockImplementation((p: string) => { return p.endsWith('.env'); }); (fs.readFileSync as jest.Mock).mockReturnValue('SINGLE=value'); loadDevEnv('/test/cwd'); - expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('loaded 1 environment variable')); - expect(consoleSpy).not.toHaveBeenCalledWith(expect.stringContaining('variables')); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringMatching(/loaded 1 environment variable(?!s)/) + ); });libs/adapters/src/openapi/__tests__/openapi-branch-coverage.spec.ts (4)
51-61: Consider using proper type guards instead ofanywith non-null assertions.The helper functions use
anytypes and a non-null assertion on line 59. Per coding guidelines, avoidanywithout strong justification and use proper error handling instead of non-null assertions.♻️ Suggested improvement
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -function getToolMeta(tool: any): any { - return tool[FrontMcpToolTokens.metadata] || {}; -} - -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -function getFirstTool(result: { tools?: any[] }): any { - expect(result.tools).toBeDefined(); - expect(result.tools!.length).toBeGreaterThan(0); - return result.tools![0]; -} +function getToolMeta(tool: Record<string | symbol, unknown>): Record<string, unknown> { + return (tool[FrontMcpToolTokens.metadata] as Record<string, unknown>) || {}; +} + +function getFirstTool(result: { tools?: unknown[] }): unknown { + expect(result.tools).toBeDefined(); + expect(result.tools?.length).toBeGreaterThan(0); + return result.tools?.[0]; +}
108-111: Weak assertion - consider verifying actual logger calls.The assertion
expect(debugSpy).toBeDefined()is always true sincedebugSpyis defined on line 96. The comment indicates the debug method should have been called, but this isn't actually verified.♻️ Suggested fix
- // The console logger's verbose/debug methods should have been called - // Verify the spy was set up correctly - expect(debugSpy).toBeDefined(); + // Verify the console logger's debug method was called during fetch + expect(debugSpy).toHaveBeenCalled();
440-445: Missing assertion for documented expected behavior.The comment states "Debug log should NOT be called when schemas are unchanged" but there's no assertion to verify this. This reduces the value of the test for detecting regressions.
♻️ Suggested fix
const result = await adapter.fetch(); expect(result.tools).toHaveLength(1); - // Debug log should NOT be called when schemas are unchanged + // Debug log should NOT be called when schemas are unchanged + expect(mockLogger.debug).not.toHaveBeenCalledWith(expect.stringContaining('Applied schema transforms')); });
608-654: Consider asserting that transform functions are actually invoked.The test sets up mock functions (
globalTransform,perToolTransform,filterFn) but only assertsresult.tools.toHaveLength(1). Verifying the mocks were called would provide stronger coverage guarantees.♻️ Suggested enhancement
const result = await adapter.fetch(); expect(result.tools).toHaveLength(1); + // Verify transforms were invoked + expect(filterFn).toHaveBeenCalled(); + expect(globalTransform).toHaveBeenCalled(); + expect(perToolTransform).toHaveBeenCalled(); });libs/sdk/src/skill/__tests__/skill-validator.spec.ts (1)
11-43: Consider reusing the shared registry mock to avoid drift.There’s already a mock helper at
libs/sdk/src/__test-utils__/mocks/tool-registry.mock.ts. Reusing it (and extending it for hidden tools) will keep tests aligned with canonical mock behavior and reduce duplication.libs/sdk/jest.config.ts (1)
19-28: Coverage thresholds are well below the 95% guideline - consider tracking this debt.The pragmatic approach of setting low thresholds for incremental improvement is reasonable given existing coverage gaps. The TODO comment is helpful for documenting intent.
Consider creating a tracking issue to monitor progress toward the 95% target. This would help ensure the thresholds are incrementally raised as tests are added.
Would you like me to help draft an issue to track the SDK coverage improvement effort?
apps/e2e/demo-e2e-skills/src/apps/skills/tools/github-add-comment.tool.ts (2)
4-15: Harden tool schemas withz.object().strict()and minimal constraints.
This aligns with SDK tool patterns and prevents unknown fields from slipping into E2E flows.♻️ Proposed refactor
-const inputSchema = { - prNumber: z.number().describe('The pull request number'), - comment: z.string().describe('The comment text to add'), -}; +const inputSchema = z + .object({ + prNumber: z.number().int().positive().describe('The pull request number'), + comment: z.string().min(1).describe('The comment text to add'), + }) + .strict(); -const outputSchema = { - commentId: z.number(), - success: z.boolean(), -}; +const outputSchema = z + .object({ + commentId: z.number().int().nonnegative(), + success: z.boolean(), + }) + .strict(); -type Input = z.infer<z.ZodObject<typeof inputSchema>>; -type Output = z.infer<z.ZodObject<typeof outputSchema>>; +type Input = z.infer<typeof inputSchema>; +type Output = z.infer<typeof outputSchema>;
25-31: Prefer deterministic IDs in E2E mocks to avoid flaky runs.
Math.random()makes outputs non-reproducible.♻️ Proposed refactor
- return { - commentId: Math.floor(Math.random() * 10000), - success: true, - }; + return { + commentId: input.prNumber * 1000 + input.comment.length, + success: true, + };apps/e2e/demo-e2e-skills/src/apps/skills/tools/github-get-pr.tool.ts (1)
4-19: Wrap schemas inz.object().strict()and tightenprNumbervalidation.
This keeps schema behavior consistent with other SDK tool patterns.♻️ Proposed refactor
-const inputSchema = { - prNumber: z.number().describe('The pull request number'), -}; +const inputSchema = z + .object({ + prNumber: z.number().int().positive().describe('The pull request number'), + }) + .strict(); -const outputSchema = { - pr: z.object({ - number: z.number(), - title: z.string(), - author: z.string(), - status: z.enum(['open', 'closed', 'merged']), - files: z.array(z.string()), - }), -}; +const outputSchema = z + .object({ + pr: z + .object({ + number: z.number(), + title: z.string(), + author: z.string(), + status: z.enum(['open', 'closed', 'merged']), + files: z.array(z.string()), + }) + .strict(), + }) + .strict(); -type Input = z.infer<z.ZodObject<typeof inputSchema>>; -type Output = z.infer<z.ZodObject<typeof outputSchema>>; +type Input = z.infer<typeof inputSchema>; +type Output = z.infer<typeof outputSchema>;apps/e2e/demo-e2e-skills/src/apps/skills/tools/slack-notify.tool.ts (2)
4-16: Wrap schemas inz.object().strict()and enforce non-empty inputs.
Optionally, consider an ISO timestamp validator (z.iso.datetime()in Zod v4) if you want strict timestamp formatting.♻️ Proposed refactor
-const inputSchema = { - channel: z.string().describe('The Slack channel to send the message to'), - message: z.string().describe('The message text to send'), -}; +const inputSchema = z + .object({ + channel: z.string().min(1).describe('The Slack channel to send the message to'), + message: z.string().min(1).describe('The message text to send'), + }) + .strict(); -const outputSchema = { - messageId: z.string(), - success: z.boolean(), - timestamp: z.string(), -}; +const outputSchema = z + .object({ + messageId: z.string(), + success: z.boolean(), + timestamp: z.string(), + }) + .strict(); -type Input = z.infer<z.ZodObject<typeof inputSchema>>; -type Output = z.infer<z.ZodObject<typeof outputSchema>>; +type Input = z.infer<typeof inputSchema>; +type Output = z.infer<typeof outputSchema>;
26-32: Optional: make mock outputs deterministic for stable E2E runs.♻️ Proposed refactor
- return { - messageId: `msg-${Math.floor(Math.random() * 10000)}`, - success: true, - timestamp: new Date().toISOString(), - }; + const stableId = `msg-${input.channel}-${input.message.length}`; + return { + messageId: stableId, + success: true, + timestamp: new Date(0).toISOString(), + };apps/e2e/demo-e2e-skills/src/apps/skills/plugins/devops-plugin.ts (1)
8-22: Preferz.object().strict()and basic validation on tool schemas.
Also consider a stricter timestamp validator (e.g.,z.iso.datetime()in Zod v4) if you want format enforcement.♻️ Proposed refactor
-const deployInputSchema = { - environment: z.enum(['staging', 'production']).describe('Deployment environment'), - version: z.string().describe('Version to deploy'), -}; +const deployInputSchema = z + .object({ + environment: z.enum(['staging', 'production']).describe('Deployment environment'), + version: z.string().min(1).describe('Version to deploy'), + }) + .strict(); -const deployOutputSchema = { - success: z.boolean(), - environment: z.enum(['staging', 'production']), - version: z.string(), - timestamp: z.string(), -}; +const deployOutputSchema = z + .object({ + success: z.boolean(), + environment: z.enum(['staging', 'production']), + version: z.string(), + timestamp: z.string(), + }) + .strict(); -type DeployInput = z.infer<z.ZodObject<typeof deployInputSchema>>; -type DeployOutput = z.infer<z.ZodObject<typeof deployOutputSchema>>; +type DeployInput = z.infer<typeof deployInputSchema>; +type DeployOutput = z.infer<typeof deployOutputSchema>; -const rollbackInputSchema = { - environment: z.enum(['staging', 'production']).describe('Environment to rollback'), - targetVersion: z.string().optional().describe('Target version to rollback to'), -}; +const rollbackInputSchema = z + .object({ + environment: z.enum(['staging', 'production']).describe('Environment to rollback'), + targetVersion: z.string().min(1).optional().describe('Target version to rollback to'), + }) + .strict(); -const rollbackOutputSchema = { - success: z.boolean(), - environment: z.enum(['staging', 'production']), - rolledBackTo: z.string(), -}; +const rollbackOutputSchema = z + .object({ + success: z.boolean(), + environment: z.enum(['staging', 'production']), + rolledBackTo: z.string(), + }) + .strict(); -type RollbackInput = z.infer<z.ZodObject<typeof rollbackInputSchema>>; -type RollbackOutput = z.infer<z.ZodObject<typeof rollbackOutputSchema>>; +type RollbackInput = z.infer<typeof rollbackInputSchema>; +type RollbackOutput = z.infer<typeof rollbackOutputSchema>;Also applies to: 41-54
apps/e2e/demo-e2e-skills/webpack.config.js (1)
4-12: Makemode/devtoolenvironment‑aware.
Right nowmodeis always"development"anddevtoolalways uses eval source maps. This pattern is consistent across all demo/e2e apps (exceptdemo-e2e-standalonewhich already makesmodeenvironment-aware). If these configs are used for production builds, you'll still emit dev bundles. Consider conditioning onNODE_ENV.♻️ Suggested update
const { NxAppWebpackPlugin } = require('@nx/webpack/app-plugin'); const { join } = require('path'); +const isProd = process.env.NODE_ENV === 'production'; module.exports = { output: { path: join(__dirname, '../../../dist/apps/e2e/demo-e2e-skills'), ...(process.env.NODE_ENV !== 'production' && { devtoolModuleFilenameTemplate: '[absolute-resource-path]', }), }, - mode: 'development', - devtool: 'eval-cheap-module-source-map', + mode: isProd ? 'production' : 'development', + devtool: isProd ? 'source-map' : 'eval-cheap-module-source-map',libs/sdk/src/skill/session/skill-session.manager.ts (4)
190-190: Fire-and-forget async calls topersistSessionmay silently lose data.
persistSessionis an async method, but it's called withoutawaitin multiple locations (lines 190, 331, 528, 556, 580, 602). While errors are logged internally, callers have no way to know if persistence failed, which could lead to session state loss in distributed scenarios.If this is intentional for performance (non-blocking persistence), consider documenting this behavior. Otherwise, consider awaiting these calls or using a background queue with retry logic.
Example: Await persistence in critical paths
// Persist to store if available - this.persistSession(session); + await this.persistSession(session);Also applies to: 331-331, 528-528, 556-556, 580-580, 602-602
411-423: Side effect during authorization check: session deactivation on expiry.The
checkToolAuthorizationmethod deactivates the skill session when duration exceeds the limit (line 415). This mutation during what appears to be a "check" operation could be unexpected. Consider either:
- Renaming to indicate mutation (e.g.,
authorizeToolCall)- Moving expiry handling to a separate method called explicitly
- Documenting this behavior clearly
145-148: Consider using a specific error class instead of genericError.The coding guidelines for
libs/sdk/**recommend using specific error classes with MCP error codes instead of generic errors. Consider creating aSkillSessionErroror similar for session-related errors.Example with specific error class
+import { SkillSessionError } from '../errors/skill-session.error'; + // In activateSkill: if (!session) { - throw new Error('Cannot activate skill: not running within a session context'); + throw new SkillSessionError('Cannot activate skill: not running within a session context', 'NO_SESSION_CONTEXT'); }
506-510: Consistent use of genericErroracross session methods.Similar to
activateSkill, methodsapproveToolForSession,denyToolForSession, andsetPolicyModethrow genericErrorinstances. Consider using a specific error class for consistency with SDK guidelines.Also applies to: 534-538, 595-599
libs/sdk/src/skill/__tests__/memory-skill.provider.spec.ts (1)
126-151: Type assertion onaddManycall for skills with extended properties.The
addManycall usesas SkillContent[]to cast skills that include extra properties (tags,priority,hideFromDiscovery). This is acceptable for testing internal behavior, but consider whether these extended properties should be formally part of theSkillContentinterface or a separateSkillMetadatatype.libs/sdk/src/skill/flows/index.ts (1)
1-1: Consider removing the redundant file path comment.The
// file: ...comment on line 1 is redundant since modern IDEs and tooling display file paths. This pattern isn't used elsewhere in the codebase.Suggested change
-// file: libs/sdk/src/skill/flows/index.ts - /** * Skill Flowslibs/sdk/src/skill/hooks/index.ts (1)
1-1: Consider removing the redundant file path comment.Same as the flows barrel - this comment is redundant.
apps/e2e/demo-e2e-skills/e2e/search-skills.e2e.test.ts (1)
174-184: Non-null assertions used aftertoBeDefined()checks.Per coding guidelines, avoid non-null assertions (
!). While theexpect(...).toBeDefined()provides a runtime check, you could refactor to avoid the!operator:Suggested approach
expect(reviewPrSkill).toBeDefined(); - expect(reviewPrSkill!.tools).toBeDefined(); - expect(reviewPrSkill!.tools.length).toBeGreaterThan(0); + if (!reviewPrSkill) throw new Error('reviewPrSkill not found'); + expect(reviewPrSkill.tools).toBeDefined(); + expect(reviewPrSkill.tools.length).toBeGreaterThan(0); // Check tool availability - for (const tool of reviewPrSkill!.tools) { + for (const tool of reviewPrSkill.tools) {Alternatively, since Jest's
expect().toBeDefined()throws on failure, you could also use a type guard helper or simply accept thatexpectalready guards the code path. Given this is E2E test code and the pattern is common, this is optional.libs/sdk/src/skill/__tests__/tool-authorization.spec.ts (2)
1-1: Consider removing the redundant file path comment.Consistent with other new files in this PR.
207-229: Consider adding explicitinstanceofcheck for error hierarchy.Per coding guidelines for test files: "Include
instanceofchecks in tests for error classes to verify proper error hierarchy." WhiletoThrow(ToolNotAllowedError)implicitly checks instanceof, an explicit assertion would make the error hierarchy verification clearer:Suggested addition
it('should format error message for not_in_allowlist with skill name', () => { const error = new ToolNotAllowedError( { allowed: false, reason: 'not_in_allowlist', toolName: 'unauthorized_tool', skillId: 'test-skill', skillName: 'Test Skill', }, ['tool_a', 'tool_b'], ); + expect(error).toBeInstanceOf(ToolNotAllowedError); + expect(error).toBeInstanceOf(Error); expect(error.message).toContain('unauthorized_tool');libs/sdk/src/skill/__tests__/skill.metadata.spec.ts (2)
18-143: Good coverage for schema validation, but consider adding test fortoolValidationdefault.The
skillMetadataSchematests comprehensively cover field validation and defaults, but thetoolValidationfield (which defaults to'warn'per the schema at lines 318-332 ofskill.metadata.ts) is not explicitly verified. Consider adding a check in the minimal metadata test:expect(result.data.toolValidation).toBe('warn'); // default value
145-162: Consider expandingnormalizeToolReftests to cover class-based tool references.The implementation supports four input types (string, function/tool class,
ToolRefWithClass, andSkillToolRefWithName), but tests only cover string andSkillToolRefWithNamecases. For comprehensive coverage, consider adding tests for class-based tool references if feasible within the test setup.apps/e2e/demo-e2e-skills/e2e/load-skill.e2e.test.ts (2)
185-190: Consider using type narrowing instead of non-null assertion.After
expect(githubGetPr).toBeDefined(), the non-null assertion on line 188-189 works but doesn't provide compile-time safety. A conditional check would be more robust:♻️ Suggested improvement
const githubGetPr = content.skill.tools.find((t) => t.name === 'github_get_pr'); expect(githubGetPr).toBeDefined(); - expect(githubGetPr!.purpose).toBeDefined(); - expect(githubGetPr!.purpose).toContain('Fetch PR details'); + if (githubGetPr) { + expect(githubGetPr.purpose).toBeDefined(); + expect(githubGetPr.purpose).toContain('Fetch PR details'); + }
202-210: Same pattern: prefer conditional narrowing over non-null assertions.♻️ Suggested improvement
// Should have pr_url parameter const prUrlParam = content.skill.parameters!.find((p) => p.name === 'pr_url'); - expect(prUrlParam).toBeDefined(); - expect(prUrlParam!.required).toBe(true); - expect(prUrlParam!.type).toBe('string'); + expect(prUrlParam).toBeDefined(); + if (prUrlParam) { + expect(prUrlParam.required).toBe(true); + expect(prUrlParam.type).toBe('string'); + }libs/sdk/src/skill/flows/__tests__/search-skills.flow.spec.ts (1)
48-118: Consider testing actualSearchSkillsFlowmethods rather than simulating logic.The pagination and output format tests replicate the flow's internal logic rather than exercising the actual flow class. While this validates algorithmic correctness, it may diverge from the actual implementation. For more robust coverage, consider instantiating
SearchSkillsFlowwith mocked dependencies and testing its actualexecute()or equivalent methods.libs/sdk/src/plugin/__tests__/plugin-skills.spec.ts (1)
64-71: Avoid non-null assertions in tests.Use an explicit guard after
expect(...).toBeDefined()so strict typing doesn't rely on!; apply the same pattern to the other!usages here. As per coding guidelines, please avoid non-null assertions in TS.♻️ Example adjustment
- const pluginSkillRegistry = skillRegistries.find( - (r) => (r as SkillRegistry).owner.kind === 'plugin' && (r as SkillRegistry).owner.id === 'test-plugin', - ) as SkillRegistry | undefined; - expect(pluginSkillRegistry).toBeDefined(); - const skills = pluginSkillRegistry!.getSkills(); + const pluginSkillRegistry = skillRegistries.find( + (r) => (r as SkillRegistry).owner.kind === 'plugin' && (r as SkillRegistry).owner.id === 'test-plugin', + ) as SkillRegistry | undefined; + expect(pluginSkillRegistry).toBeDefined(); + if (!pluginSkillRegistry) { + throw new Error('Expected plugin skill registry to be defined'); + } + const skills = pluginSkillRegistry.getSkills();Also applies to: 94-98, 124-129
libs/sdk/src/skill/flows/search-skills.flow.ts (1)
48-57: Preferz.unknown()overz.any()in state schema.
unknownavoids propagatinganywhile keeping flexibility. As per coding guidelines, preferunknowntoany.♻️ Suggested tweak
- results: z.array(z.any()), + results: z.array(z.unknown()),libs/sdk/src/app/instances/app.remote.instance.ts (1)
72-123: Align EmptySkillRegistry method signatures with SkillRegistryInterface.Several methods omit parameters or narrow return types (e.g.,
Promise<[]>), which can drift from the interface and weaken typing. Consider usingParameters<>/ReturnType<>to keep signatures aligned (apply to the other methods too).♻️ Example alignment
- findByName(): SkillEntry | undefined { + findByName( + ..._args: Parameters<SkillRegistryInterface['findByName']> + ): ReturnType<SkillRegistryInterface['findByName']> { return undefined; } - async search(): Promise<[]> { + async search( + ..._args: Parameters<SkillRegistryInterface['search']> + ): ReturnType<SkillRegistryInterface['search']> { return []; }libs/sdk/src/common/records/skill.record.ts (1)
28-56: Prefer interfaces for record shapesThese record definitions are object shapes; consider using
interfacefor consistency with the SDK style guide, keeping the union type as-is. As per coding guidelines, please preferinterfacefor object shapes.♻️ Suggested refactor
-export type SkillClassTokenRecord = { +export interface SkillClassTokenRecord { kind: SkillKind.CLASS_TOKEN; provide: Type<SkillContext>; metadata: SkillMetadata; -}; +} -export type SkillValueRecord = { +export interface SkillValueRecord { kind: SkillKind.VALUE; provide: symbol; metadata: SkillMetadata; -}; +} -export type SkillFileRecord = { +export interface SkillFileRecord { kind: SkillKind.FILE; provide: symbol; metadata: SkillMetadata; /** * Path to the source file (for reloading/hot-reload support). */ filePath: string; -}; +}apps/e2e/demo-e2e-skills/e2e/plugin-skills.e2e.test.ts (1)
161-166: Avoid non-null assertions in testsReplace the
!assertions with explicit guards so the test fails with a clear message when a lookup unexpectedly returnsundefined. As per coding guidelines, avoid non-null assertions in TS.♻️ Suggested refactor
const deploySkill = content.skills.find((s) => s.id === 'deploy-workflow'); expect(deploySkill).toBeDefined(); - expect(deploySkill!.source).toBe('local'); + if (!deploySkill) { + throw new Error('Expected deploy-workflow to be present in search results'); + } + expect(deploySkill.source).toBe('local');const deployTool = content.skill.tools.find((t) => t.name === 'deploy_application'); const rollbackTool = content.skill.tools.find((t) => t.name === 'rollback_deployment'); expect(deployTool).toBeDefined(); - expect(deployTool!.purpose).toContain('Deploy the application'); + if (!deployTool) { + throw new Error('Expected deploy_application tool to be present'); + } + expect(deployTool.purpose).toContain('Deploy the application'); expect(rollbackTool).toBeDefined(); - expect(rollbackTool!.purpose).toContain('Rollback if needed'); + if (!rollbackTool) { + throw new Error('Expected rollback_deployment tool to be present'); + } + expect(rollbackTool.purpose).toContain('Rollback if needed');Also applies to: 219-226
apps/e2e/demo-e2e-skills/e2e/skill-session.e2e.test.ts (1)
15-56: Consider extracting shared interfaces to a common test utilities file.The interfaces
LoadSkillResult,SearchSkillsResult,GitHubPRResult,CommentResult, andSlackResultappear to be duplicated across multiple E2E test files (per the AI summary, similar definitions exist inload-skill.e2e.test.tsand other test files). Consider extracting these to a shared test utilities file to reduce duplication and ensure consistency.libs/sdk/src/skill/flows/load-skill.flow.ts (1)
190-214: Consider adding type guard for scope.skillSession access.The type assertion on line 192 works but could be fragile if the scope interface changes. Consider using a type guard or checking the property directly.
♻️ Optional improvement
// Try to get skill session manager from scope // The manager is optional - it may not be configured - const scope = this.scope as { skillSession?: SkillSessionManager }; - const sessionManager = scope.skillSession; + const sessionManager = 'skillSession' in this.scope + ? (this.scope as { skillSession?: SkillSessionManager }).skillSession + : undefined;Alternatively, define a proper type extension for scopes that support skill sessions.
libs/sdk/src/common/decorators/skill.decorator.ts (1)
141-164: The final type assertion may be unsafe if metadata is partially set.The
return metadata as SkillMetadataon line 163 assumes all required fields were collected. WhileFrontMcpSkillvalidates via Zod schema before storing, if metadata were somehow corrupted or partially set, this could return an object missing required fields.Consider validating the reconstructed metadata before returning, or document that this function is only safe to call on properly decorated classes.
♻️ Safer alternative with validation
// Merge extended metadata const extended = Reflect.getMetadata(extendedSkillMetadata, target); if (extended) { Object.assign(metadata, extended); } - return metadata as SkillMetadata; + // Validate reconstructed metadata matches expected shape + const result = skillMetadataSchema.safeParse(metadata); + return result.success ? result.data as SkillMetadata : undefined; }libs/sdk/src/common/interfaces/skill.interface.ts (1)
39-47: PreferSkillToolRef[]forSkillContent.toolsto avoid drift.Using an inline object type duplicates the existing
SkillToolRefcontract and can diverge from normalization semantics over time.♻️ Proposed refactor
- tools: Array<{ name: string; purpose?: string; required?: boolean }>; + tools: SkillToolRef[];libs/sdk/src/skill/index.ts (1)
119-120: Add@deprecatedJSDoc to deprecated tool exports.The comment won’t surface deprecation warnings to TS consumers; a JSDoc tag will.
♻️ Proposed change
// Tools (deprecated - use flows instead) +/** `@deprecated` Use SearchSkillsFlow / LoadSkillFlow instead. */ export { SearchSkillsTool, LoadSkillTool, getSkillTools } from './tools';
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@libs/sdk/src/elicitation/helpers/fallback.helper.ts`:
- Around line 185-191: The rejection in the subscribeFallbackResult() catch
block incorrectly throws ElicitationTimeoutError; replace that with the original
subscription error (or a new ElicitationSubscriptionError if you prefer) so
callers can distinguish subscription failures from timeouts—locate the catch in
handleWaitingFallback where logger.error('handleWaitingFallback: failed to
subscribe', err) is called and change the reject(...) to either reject(err) or
reject(new ElicitationSubscriptionError(error.elicitId, err)) while keeping the
logging and timeout cleanup intact.
In `@libs/sdk/src/skill/providers/external-skill.provider.ts`:
- Around line 337-344: The syncSkills method in ExternalSkillProvider throws a
PublicMcpError with a custom code 'SKILL_SYNC_MODE_ERROR'; change that to use a
standard MCP error code (e.g., 'INVALID_PARAMS' or 'INVALID_REQUEST') so SDK
error handling is consistent — update the PublicMcpError instantiation in
syncSkills to use the chosen MCP code (keeping the message and HTTP status) to
replace the custom string.
In `@libs/sdk/src/skill/providers/memory-skill.provider.ts`:
- Around line 351-365: The update() path can leave orphaned vector docs when
skillId and skill.id diverge; before mutating the in-memory map and indexing,
normalize and validate the IDs: if skill.id !== skillId, set skill.id = skillId
(or throw) so the same id is used for skills.set(...) and indexSkill(...); also
ensure you remove any existing vector entries by checking/removing both skillId
and the original skill.id via vectorDB.hasDocument/removeDocument to avoid stale
docs (referencing update, add, indexSkill, vectorDB.hasDocument,
vectorDB.removeDocument, and skills.set).
- Around line 125-150: MemorySkillProvider is a newly exported public API but
lacks draft docs; add a new .mdx under docs/draft/docs describing the
MemorySkillProvider class, its options (MemorySkillProviderOptions including
defaultTopK, defaultMinScore, toolValidator), behavior (in-memory storage,
TFIDFVectoria usage), constructor example showing how to instantiate and
configure it, and notes on defaults and intended use; ensure the doc references
the class name MemorySkillProvider and related types
(MemorySkillProviderOptions, SkillContent, SkillDocumentMetadata, TFIDFVectoria)
so the public API is documented alongside other SDK providers.
In `@libs/sdk/src/skill/skill-storage.factory.ts`:
- Around line 221-229: In the case 'external' branch inside SkillStorageFactory
replace the custom error code 'SKILL_STORAGE_CONFIG_ERROR' with the MCP-standard
error code (e.g., INVALID_PARAMS) when throwing PublicMcpError for a missing
externalProvider; keep the existing message and status but swap the code
argument to the MCP constant (or string) used across the SDK so callers can
programmatically handle it (update any necessary import or reference to the MCP
error code constant alongside the existing PublicMcpError usage).
In `@libs/sdk/src/skill/skill.instance.ts`:
- Around line 171-187: The JSDoc for getContentSync is incorrect: it claims the
method "throws if instructions need loading" but the implementation returns
undefined when instructions are not a string; update the comment for
getContentSync (and mention SkillContent return behavior) to state that it
returns the cached SkillContent or builds from inline instructions, and
otherwise returns undefined rather than throwing; reference
metadata.instructions and buildSkillContent in the description so readers know
when undefined is returned.
♻️ Duplicate comments (4)
libs/auth/jest.config.cts (1)
12-22: Documentation for coverage gaps is a positive step; threshold concern already flagged.The added comments explaining the coverage gaps and the
TODO(FrontMCP-Auth-Coverage)tracking ticket are helpful for transparency and accountability. This documents existing technical debt rather than introducing it.However, the underlying issue—coverage thresholds significantly below the 95% requirement—was already raised in a previous review. Please ensure the referenced ticket (
FrontMCP-Auth-Coverage) is created and prioritized so these thresholds can be incrementally raised as tests are added. Based on learnings, the repo requires 95%+ coverage across all metrics.libs/sdk/src/front-mcp/front-mcp.ts (1)
257-272: Removeanyfrom wrapped MCP handlers.The
anyannotations and cast defeat strict TS checks and violate the SDK guideline againstanywithout strong justification. Consider typing the wrapper as the same signature ashandler.handler, then pass it through without casting.🔧 Suggested refinement
// Wrap handler to inject auth context (same pattern as in-memory-server) const originalHandler = handler.handler; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const wrappedHandler = async (request: any, ctx: any) => { + const wrappedHandler: typeof handler.handler = async (request, ctx) => { // Inject auth info into context while preserving MCP SDK context properties // Merge with existing authInfo to avoid clobbering any existing properties const enrichedCtx = { ...ctx, - authInfo: { ...ctx?.authInfo, sessionId }, + authInfo: { ...(ctx.authInfo ?? {}), sessionId }, }; return originalHandler(request, enrichedCtx); }; // Cast required: MCP SDK's handler type expects specific context shape, // but our wrapped handlers preserve all context properties via pass-through - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - mcpServer.setRequestHandler(handler.requestSchema, wrappedHandler as any); + mcpServer.setRequestHandler(handler.requestSchema, wrappedHandler);As per coding guidelines, please verify handler typing with:
#!/bin/bash # Inspect handler signatures and setRequestHandler typing for a safe typed wrapper. rg -nP --type=ts -C3 '\bsetRequestHandler\s*\(' libs/sdk/src rg -nP --type=ts -C3 '\bhandler\s*:\s*' libs/sdk/src/transport/mcp-handlers rg -nP --type=ts -C3 '\bRequestHandler\b|\bMcpHandler\b' libs/sdk/src/transport/mcp-handlerslibs/sdk/src/skill/session/skill-session-store.interface.ts (1)
56-101:activeSkillsMap still not deep-cloned — shared references remain.The past review comment about deep-cloning
activeSkillsand its nested Sets has not been addressed. The current implementation clones top-level Sets but shares theactiveSkillsMap by reference insave,get,update, andlistActive. External code can mutate the stored state.Additionally, in
update(line 100),Object.assign(session, clonedUpdates)mutates the stored session directly rather than replacing it with a cloned copy, which can lead to partial state corruption ifupdatescontains uncloned references.libs/sdk/src/common/metadata/skill.metadata.ts (1)
392-434: Replace genericErrorthrows with SDK/MCP error types.
normalizeToolRefstill throws plainError, which makes it harder for callers to handle validation errors consistently. Please switch toPublicMcpError(or the SDK’s typed error) with an MCP error code (e.g.,INVALID_PARAMS) in all three branches. As per coding guidelines, use MCP-coded SDK errors.🔧 Suggested fix
+import { MCP_ERROR_CODES, PublicMcpError } from '../../errors'; ... - throw new Error( + throw new PublicMcpError( `Invalid tool class '${ref.name ?? 'unknown'}'. ` + 'Tool class must be decorated with `@Tool` and have a name property.', + MCP_ERROR_CODES.INVALID_PARAMS, + 400, ); ... - throw new Error( + throw new PublicMcpError( `Invalid tool class in reference. ` + 'Tool class must be decorated with `@Tool` and have a name property.', + MCP_ERROR_CODES.INVALID_PARAMS, + 400, ); ... - throw new Error(`Invalid tool reference: ${JSON.stringify(ref)}`); + throw new PublicMcpError( + `Invalid tool reference: ${JSON.stringify(ref)}`, + MCP_ERROR_CODES.INVALID_PARAMS, + 400, + );
🧹 Nitpick comments (8)
libs/sdk/src/skill/skill.instance.ts (1)
111-114: Avoid type assertions to mutate objects with undeclared properties.Adding properties via repeated type assertions circumvents type safety and makes the code fragile. Consider defining an extended interface for cached content that includes these metadata fields.
Suggested approach
Define an internal interface for cached content:
interface CachedSkillContent extends SkillContent { tags?: string[]; priority?: number; hideFromDiscovery?: boolean; }Then change the cache field type and assignment:
- /** Cached skill content (built lazily) */ - private _cachedContent?: SkillContent; + /** Cached skill content (built lazily) */ + private _cachedContent?: CachedSkillContent;- // Add additional metadata that's useful for search but not in SkillContent - // We store these as extra properties on the cached content - (this._cachedContent as SkillContent & { tags?: string[]; priority?: number; hideFromDiscovery?: boolean }).tags = - this._tags; - (this._cachedContent as SkillContent & { priority?: number }).priority = this._priority; - (this._cachedContent as SkillContent & { hideFromDiscovery?: boolean }).hideFromDiscovery = this._hidden; + // Add additional metadata that's useful for search but not in SkillContent + this._cachedContent.tags = this._tags; + this._cachedContent.priority = this._priority; + this._cachedContent.hideFromDiscovery = this._hidden;libs/sdk/src/skill/session/skill-session-store.interface.ts (1)
127-140: Clarify cleanup logic — comment says "inactive" but checksstartedAtage.The comment on line 132 says "Clean up sessions that have been inactive" but the logic deletes sessions based on
startedAttimestamp, regardless of activity. Consider updating the comment to reflect the actual behavior, or if the intent is to clean up based on inactivity, use alastActivityAttimestamp instead.📝 Suggested comment fix
async cleanup(maxAge: number): Promise<number> { const now = Date.now(); let cleaned = 0; for (const [sessionId, session] of this.sessions) { - // Clean up sessions that have been inactive + // Clean up sessions older than maxAge based on startedAt timestamp if (session.startedAt > 0 && now - session.startedAt > maxAge) { this.sessions.delete(sessionId); cleaned++; } } return cleaned; }libs/sdk/src/skill/__tests__/skill-session.spec.ts (1)
509-614: Consider adding mutation isolation tests.The store tests verify CRUD operations but don't test that retrieved sessions are properly isolated from the stored state. Adding a test that mutates a retrieved session and verifies the store remains unaffected would catch reference-sharing bugs.
🧪 Suggested test for mutation isolation
it('should isolate retrieved sessions from store mutations', async () => { const session = createSession('session-1'); await store.save(session); const retrieved = await store.get('session-1'); // Mutate the retrieved session retrieved!.allowedTools.add('mutated_tool'); retrieved!.activeSkills.get('skill-1')!.allowedTools.add('another_mutated'); // Verify store is unaffected const freshRetrieved = await store.get('session-1'); expect(freshRetrieved!.allowedTools.has('mutated_tool')).toBe(false); expect(freshRetrieved!.activeSkills.get('skill-1')!.allowedTools.has('another_mutated')).toBe(false); });libs/sdk/src/skill/flows/load-skill.flow.ts (1)
158-162: Consider a more appropriate error type for missing skill registry.
InvalidInputErrorimplies the client sent invalid data, but a missing skill registry is a server configuration issue. Consider using a more semantically appropriate error type (e.g.,InternalErroror a configuration-specific error).♻️ Suggested improvement
+import { InternalError } from '../../errors'; ... if (!skillRegistry) { - throw new InvalidInputError('Skills are not available in this scope'); + throw new InternalError('Skill registry not configured'); }Per coding guidelines, use specific error classes with MCP error codes.
.github/workflows/push.yml (1)
248-266: Consider extracting coverage flag into a reusable approach.The conditional coverage logic is duplicated between the initial run and retry steps. While functional, you could simplify by defining a computed command variable or using a composite action.
♻️ Optional: Use a shell variable to reduce duplication
- name: Run E2E tests (${{ matrix.project }}) id: test continue-on-error: true run: | - if [ "$RUN_COVERAGE" = "true" ]; then - npx nx run ${{ matrix.project }}:test --coverage - else - npx nx run ${{ matrix.project }}:test - fi + COVERAGE_FLAG=$([[ "$RUN_COVERAGE" = "true" ]] && echo "--coverage" || echo "") + npx nx run ${{ matrix.project }}:test $COVERAGE_FLAG # ... reset step ... - name: Retry E2E tests (${{ matrix.project }}) if: steps.test.outcome == 'failure' run: | - if [ "$RUN_COVERAGE" = "true" ]; then - npx nx run ${{ matrix.project }}:test --coverage - else - npx nx run ${{ matrix.project }}:test - fi + COVERAGE_FLAG=$([[ "$RUN_COVERAGE" = "true" ]] && echo "--coverage" || echo "") + npx nx run ${{ matrix.project }}:test $COVERAGE_FLAGlibs/sdk/src/elicitation/helpers/fallback.helper.ts (1)
118-138: Consider typing the error response objects.The error result objects (timeout at lines 128-136, failure at lines 159-167) match the
CallToolResultstructure but are untyped inline objects. For better type safety and consistency with MCP protocol responses, consider using or creating a typed helper.💡 Example typed helper
// Could be added to elicitation.types.ts or a shared location function createErrorResult(message: string): { content: Array<{ type: 'text'; text: string }>; isError: true } { return { content: [{ type: 'text', text: message }], isError: true, }; }Also applies to: 155-168
libs/sdk/src/skill/providers/external-skill.provider.ts (1)
470-472: Return a deeper copy of sync state entries.
getSyncState()currently copies theMapbut reuses entry objects, so callers can mutate internal state via object references. Consider cloning each entry value for better encapsulation.♻️ Suggested tweak
- return this.syncState ? { ...this.syncState, entries: new Map(this.syncState.entries) } : null; + return this.syncState + ? { + ...this.syncState, + entries: new Map( + [...this.syncState.entries].map(([key, value]) => [key, { ...value }]), + ), + } + : null;libs/sdk/src/skill/providers/memory-skill.provider.ts (1)
524-546: Consider a typed metadata wrapper instead of casts.
TheSkillContent & { tags?: ... }casts are a bit brittle; a small wrapper type for stored skills would avoid repeated assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/e2e/demo-e2e-transport-recreation/project.json`:
- Around line 36-42: The test target in the project configuration is missing the
runInBand option which other e2e projects use to force sequential same-process
Jest runs; update the "test" target options (the JSON object under "test" for
this project) to include "runInBand": true alongside the existing "jestConfig"
and "passWithNoTests" keys so Jest runs in-band like other e2e projects and
avoids concurrent port/resource conflicts.
…g in fallback helper
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/draft/docs/servers/memory-skill-provider.mdx`:
- Around line 213-217: In the "See Also" section remove or fix the two broken
links (/servers/external-skill-providers and /servers/skill-search): either
create the missing docs pages or replace those link targets with valid existing
docs (e.g., other relevant pages in the docs set) and ensure the link text
remains meaningful; specifically edit the "See Also" block in
memory-skill-provider’s See Also section so it only contains valid URLs (or
remove the entries) while keeping "Skills Overview" intact.
♻️ Duplicate comments (1)
libs/sdk/src/skill/skill-storage.factory.ts (1)
220-230: HTTP status 500 is incorrect for invalid configuration.The error for missing
externalProvideruses HTTP status 500 (Internal Server Error), but this is a client configuration error that should use 400 (Bad Request). The past review suggested changing to 400.🔧 Suggested fix
throw new PublicMcpError( 'External provider type requires externalProvider instance. ' + 'Create a class extending ExternalSkillProviderBase and pass it via externalProvider option.', String(MCP_ERROR_CODES.INVALID_PARAMS), - 500, + 400, );
🧹 Nitpick comments (11)
libs/sdk/src/skill/skill.instance.ts (2)
95-100: Consider usingpathmodule for cross-platform path handling.The path extraction using
lastIndexOf('/')assumes forward slashes. While SDK paths are typically normalized, usingpath.dirname()would be more robust.♻️ Suggested improvement
+import { dirname } from 'path'; + // For file-based skills, use the directory of the skill file -const filePath = this.record.filePath; -const lastSlash = filePath.lastIndexOf('/'); -basePath = lastSlash > 0 ? filePath.substring(0, lastSlash) : undefined; +basePath = dirname(this.record.filePath) || undefined;
189-201: Consider caching inline-built content for performance.When building content from inline instructions, the result isn't cached. This means repeated
getContentSync()calls beforeload()will rebuild each time. If side-effect-free behavior is intentional, this is fine; otherwise, consider populating the cache.♻️ Optional: Cache inline-built content
// Only works with inline instructions if (typeof this.metadata.instructions === 'string') { - return buildSkillContent(this.metadata, this.metadata.instructions); + const content = buildSkillContent(this.metadata, this.metadata.instructions); + this._cachedContent = { + ...content, + tags: this._tags, + priority: this._priority, + hideFromDiscovery: this._hidden, + }; + return this._cachedContent; }libs/sdk/src/skill/providers/external-skill.provider.ts (1)
251-267: Consider validating tools against registry for external loads.The
loadmethod marks all tools as "available" with an emptymissingToolsarray. While the comment explains validation happens at registry level, this means callers cannot distinguish between "tools validated as available" vs "tools not validated at all."Consider adding an optional
toolValidator(likeMemorySkillProvider) to provide consistent tool validation across providers.libs/sdk/src/skill/skill-storage.factory.ts (1)
242-263: Unreachable catch block in vectordb case.The try-catch block wraps code that doesn't throw any exceptions. The
MemorySkillProviderconstructor is synchronous and doesn't throw. This creates dead code that may confuse maintainers.Consider simplifying since the VectorDB provider is not yet implemented:
♻️ Suggested simplification
case 'vectordb': { - // Lazy require VectorDB provider to avoid bundling when not used - try { - // VectorDB provider will be implemented in Phase 7 - // For now, fall back to memory provider with a warning - logger?.warn('[SkillStorageFactory] VectorDB provider not yet implemented, falling back to memory provider'); - provider = new MemorySkillProvider({ - ...memory, - toolValidator, - }); - type = 'memory'; - } catch (error) { - logger?.warn('[SkillStorageFactory] Failed to create VectorDB provider, falling back to memory', { - error: error instanceof Error ? error.message : String(error), - }); - provider = new MemorySkillProvider({ - ...memory, - toolValidator, - }); - type = 'memory'; - } + // VectorDB provider will be implemented in Phase 7 + // For now, fall back to memory provider with a warning + logger?.warn('[SkillStorageFactory] VectorDB provider not yet implemented, falling back to memory provider'); + provider = new MemorySkillProvider({ + ...memory, + toolValidator, + }); + type = 'memory'; break; }libs/sdk/src/skill/providers/memory-skill.provider.ts (3)
149-172:initializedflag is set but never checked.The
initializedflag is set totrueininitialize()andfalseindispose(), but it's never checked before operations. Consider either:
- Adding guards to methods that require initialization
- Removing the flag if initialization is not required
♻️ Optional: Add initialization guard
+ private ensureInitialized(): void { + if (!this.initialized) { + throw new Error('MemorySkillProvider not initialized. Call initialize() first.'); + } + } + async search(query: string, options: SkillSearchOptions = {}): Promise<SkillSearchResult[]> { + this.ensureInitialized(); const {
422-435: Reindexing after every single add may impact performance.The
indexSkillmethod callsthis.vectorDB.reindex()after adding a single document. For bulk operations, this is already optimized inaddMany, but individualaddandupdatecalls will trigger full reindexing.Consider documenting this behavior or deferring reindex for batch scenarios:
// Consider adding a flag or batch mode async add(skill: SkillContent, skipReindex = false): Promise<void> { this.skills.set(skill.id, skill); this.indexSkillDocument(skill); if (!skipReindex) { this.vectorDB.reindex(); } }This is noted in the documentation under "Performance Considerations" but may warrant a batch API for efficiency.
541-559: Type assertions toStoredSkillContentare safe but could be cleaner.The helper methods cast
SkillContenttoStoredSkillContentto access extended properties. While this works, consider using a type guard or making the internal storage explicitly useStoredSkillContent:♻️ Optional: Use explicit typing
- private skills: Map<string, SkillContent> = new Map(); + private skills: Map<string, StoredSkillContent> = new Map(); // Then remove type assertions in helper methods private getSkillTags(skill: StoredSkillContent): string[] { return skill.tags ?? []; }This would require updating method signatures but provides better type safety throughout the class.
libs/sdk/src/elicitation/helpers/fallback.helper.ts (1)
88-195: Consider tightening the return type to avoidunknownleakage.
handleWaitingFallbackalways resolves to a tool result shape, butPromise<unknown>forces downstream casts. If possible, alignFallbackExecutionResult.resulttoCallToolResultand update this signature accordingly. As per coding guidelines, prefer strictly typed MCP protocol responses where feasible.libs/sdk/src/skill/session/skill-session-store.interface.ts (2)
59-71: Use a top-level import instead of inlineimport()type references.The
import('./skill-session.types').ActiveSkillInfosyntax is repeated three times. ImportActiveSkillInfoat the top of the file alongsideSkillSessionStatefor cleaner, more maintainable code.♻️ Suggested refactor
-import type { SkillSessionState } from './skill-session.types'; +import type { SkillSessionState, ActiveSkillInfo } from './skill-session.types';Then update the method signature:
private cloneActiveSkills( - activeSkills: Map<string, import('./skill-session.types').ActiveSkillInfo>, - ): Map<string, import('./skill-session.types').ActiveSkillInfo> { - const cloned = new Map<string, import('./skill-session.types').ActiveSkillInfo>(); + activeSkills: Map<string, ActiveSkillInfo>, + ): Map<string, ActiveSkillInfo> { + const cloned = new Map<string, ActiveSkillInfo>();
183-206: Factory function type options are inconsistent with the interface.The interface declares
type: 'memory' | 'redis' | 'custom'(line 45), but the factory only accepts'memory' | 'redis'. Additionally, thedefaultcase silently returns a memory store, which could mask errors if new types are added in the future.Consider either:
- Adding
'custom'support to the factory (e.g., accepting a custom store instance), or- Removing
'custom'from the interface if it's not intended to be created via the factory.For the
defaultcase, using a type-narrowing exhaustive check would surface future mismatches at compile time.♻️ Suggested refactor with exhaustive check
switch (type) { case 'memory': return new MemorySkillSessionStore(); case 'redis': // Redis implementation would go here // For now, fall back to memory console.warn('Redis skill session store not implemented, using memory store'); return new MemorySkillSessionStore(); - default: - return new MemorySkillSessionStore(); + default: { + const _exhaustive: never = type; + throw new Error(`Unknown skill session store type: ${_exhaustive}`); + } }libs/sdk/src/skill/__tests__/skill-session.spec.ts (1)
615-637: Replace non-null assertions with proper guards.The coding guidelines specify avoiding non-null assertions (
!) in favor of proper error handling and type guards. In tests, useexpectassertions or guards before accessing properties.As per coding guidelines, consider this pattern:
♻️ Suggested refactor
it('should isolate retrieved sessions from store mutations', async () => { const session = createSession('session-1'); await store.save(session); // Retrieve the session and mutate it const retrieved = await store.get('session-1'); - expect(retrieved).not.toBeNull(); + expect(retrieved).not.toBeNull(); + if (!retrieved) return; // Type guard for TS narrowing // Mutate the retrieved session's Sets and Maps - retrieved!.allowedTools.add('mutated_tool'); - retrieved!.activeSkills.get('skill-1')!.allowedTools.add('another_mutated'); - retrieved!.approvedTools.add('mutated_approved'); - retrieved!.deniedTools.add('mutated_denied'); + retrieved.allowedTools.add('mutated_tool'); + const skill = retrieved.activeSkills.get('skill-1'); + expect(skill).toBeDefined(); + if (skill) skill.allowedTools.add('another_mutated'); + retrieved.approvedTools.add('mutated_approved'); + retrieved.deniedTools.add('mutated_denied'); // Get a fresh copy from the store const freshRetrieved = await store.get('session-1'); + expect(freshRetrieved).not.toBeNull(); + if (!freshRetrieved) return; + const freshSkill = freshRetrieved.activeSkills.get('skill-1'); + expect(freshSkill).toBeDefined(); // Verify the fresh copy is not affected by mutations - expect(freshRetrieved!.allowedTools.has('mutated_tool')).toBe(false); - expect(freshRetrieved!.activeSkills.get('skill-1')!.allowedTools.has('another_mutated')).toBe(false); - expect(freshRetrieved!.approvedTools.has('mutated_approved')).toBe(false); - expect(freshRetrieved!.deniedTools.has('mutated_denied')).toBe(false); + expect(freshRetrieved.allowedTools.has('mutated_tool')).toBe(false); + expect(freshSkill?.allowedTools.has('another_mutated')).toBe(false); + expect(freshRetrieved.approvedTools.has('mutated_approved')).toBe(false); + expect(freshRetrieved.deniedTools.has('mutated_denied')).toBe(false); });
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.