feat: Implement skills HTTP configuration and visibility management #222
Conversation
📝 WalkthroughWalkthroughAdds a Skills HTTP subsystem: new configurable endpoints (/llm.txt, /llm_full.txt, /skills), visibility controls, auth modes (public/api-key/bearer), per-scope caching (memory/Redis), skills-only session support, multi-skill load APIs/tools/flows, registry/provider updates, formatter/auth/cache utilities, and many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server (Flow)
participant Auth as SkillHttpAuthValidator
participant Cache as SkillHttpCache
participant Registry as SkillRegistry
participant Tools as ToolRegistry
Client->>Server: GET /skills?query=...
Server->>Auth: validate(headers) (if auth enabled)
Auth-->>Server: authorized / error
alt authorized
Server->>Cache: getSkillsList(hash)
Cache-->>Server: cached list / null
alt cache miss
Server->>Registry: search/get visible skills (visibility=http)
Registry-->>Server: SkillEntry[]
loop per skill
Server->>Registry: loadSkill(skillId)
Registry-->>Server: SkillContent
Server->>Tools: get availability + schemas
Tools-->>Server: tool availability & schemas
end
Server->>Cache: setSkillsList(hash, result)
end
Server-->>Client: 200 application/json (skills list)
else auth error
Auth-->>Client: 401/403
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/skill/flows/search-skills.flow.ts (1)
199-201: Minor pagination edge case with post-filter visibility.The
hasMorecalculation may be inaccurate when visibility filtering removes HTTP-only skills. If the search returnslimitresults but some are filtered out,hasMorecould incorrectly reportfalseeven when more MCP-visible skills exist.Consider passing
visibility: 'mcp'to the registry search if supported, or document this as a known limitation.libs/sdk/src/skill/index.ts (1)
37-183: Add documentation for new public API exports underdocs/draft/docs.The following newly exported utilities lack documentation in
docs/draft/docs:
- HTTP caching system:
SkillHttpCache,MemorySkillHttpCache,RedisSkillHttpCache,createSkillHttpCache,getSkillHttpCache,invalidateScopeCache,invalidateSkillInCache,disposeAllCaches- HTTP authentication:
SkillHttpAuthValidator,createSkillHttpAuthValidator- HTTP formatting & visibility:
formatSkillsForLlmCompact,formatSkillsForLlmFull,formatSkillForLLMWithSchemas,filterSkillsByVisibility- Session mode utilities:
detectSkillsOnlyMode,isSkillsOnlySessionPer coding guidelines, public API changes require corresponding
docs/draft/docs/**updates. Create documentation covering these HTTP/caching/auth utilities and their usage patterns.
🤖 Fix all issues with AI agents
In `@libs/sdk/src/common/entries/tool.entry.ts`:
- Around line 87-105: The getInputJsonSchema method currently destructures
toJSONSchema from 'zod' which fails in Zod v4; update the dynamic require in
getInputJsonSchema to import only z and call
z.toJSONSchema(z.object(this.inputSchema)) so schema conversion works (refer to
getInputJsonSchema and this.inputSchema); change the types of rawInputSchema and
rawOutputSchema from any to unknown (or a more specific type) to remove any
usage; additionally, in the catch block of getInputJsonSchema add a log call to
surface conversion errors before returning the empty object fallback.
In `@libs/sdk/src/common/types/options/skills-http/schema.ts`:
- Around line 152-154: The prefix value used to build endpoint paths can be
missing a leading slash, producing malformed paths; update the prefix schema
validation (the "prefix" schema in this file) to require either an empty string
or a string that starts with '/' (e.g., add a regex check) and return a clear
validation error message, or alternatively normalize the value before use (in
the code that calls normalizeEndpointConfig for
normalizedLlmTxt/normalizedLlmFullTxt/normalizedApi) by prepending '/' when
missing; reference the "prefix" schema and the calls to normalizeEndpointConfig
(normalizedLlmTxt, normalizedLlmFullTxt, normalizedApi) when making the change.
In `@libs/sdk/src/skill/cache/skill-http-cache.factory.ts`:
- Around line 136-169: The code accepts provider === 'redis' but always
constructs an ioredis client; update the factory to handle the 'redis' provider
explicitly: when provider === 'redis' (the provider variable) lazy-require
node-redis (require('redis')), call createClient(...) with appropriate options
(host/port/password/db or a connection URL), await client.connect(), and return
a RedisSkillHttpCache with getClient async () => client; keep the existing
branch for 'vercel-kv' and the ioredis branch as the default for other providers
or alternatively remove the 'redis' option from the interface—ensure
RedisSkillHttpCache construction, the getClient symbol, and the client variable
are used consistently and that connect is awaited for node-redis.
In `@libs/sdk/src/skill/flows/http/skills-api.flow.ts`:
- Around line 237-251: The visibility check is unreliable because it compares
the loaded SkillLoadResult.skill against skillRegistry.getSkills(true) which may
return a different entry; instead, after obtaining the loaded skill (from
loadSkill or SkillLoadResult.skill) explicitly look up the canonical registry
entry via skillRegistry.getSkills(true).find(...) using the skill's unique id
(skill.id or metadata.id) and use that registry entry's metadata.visibility for
the HTTP gating; if no registry entry exists, treat the skill as not visible via
HTTP (or ensure SkillLoadResult.skill includes the registry metadata by
populating skill.metadata.visibility from the registry) so the existing
visibility check (visibility === 'mcp') reliably blocks MCP-only skills.
In `@libs/sdk/src/skill/tools/search-skills.tool.ts`:
- Around line 157-162: The hasMore flag is computed from the post-filtered array
(skills) so visibility filtering can make it false even when the registry
returned a full page; change hasMore to use the unfiltered registry response
length (results.length) instead of skills.length while keeping total =
skills.length; update the assignment for hasMore (referencing hasMore, skills,
results, and input.limit) so it reflects truncation at the source before
visibility filtering.
In `@libs/testing/src/client/mcp-test-client.ts`:
- Around line 873-879: The code currently appends query params to baseUrl inside
createTransport(), which leads to malformed URLs when StreamableHttpTransport
later constructs request URLs; instead, stop mutating baseUrl in
McpTestClient.createTransport() (leave baseUrl as this.config.baseUrl and ignore
this.config.queryParams there) and move the query string assembly into
StreamableHttpTransport.request() where the request path is built (at the point
it concatenates "/" to baseUrl), by reading the client config.queryParams and
appending ?key=val (or & when needed) after the trailing "/" so query params
come after the path separator; update references to baseUrl and queryParams in
createTransport and StreamableHttpTransport.request accordingly.
🧹 Nitpick comments (22)
libs/sdk/src/skill/cache/skill-http-cache.holder.ts (3)
96-99: Silent fallback to memory cache may hide configuration errors.When Redis cache creation fails, the error is silently swallowed and a memory cache is returned. This could mask misconfigurations (wrong host, auth failures) that the user should be aware of. Consider logging a warning here.
♻️ Suggested improvement
- } catch { + } catch (error) { // Fall back to memory cache on error + // Note: Consider adding logging here if a logger is available return new MemorySkillHttpCache(cacheConfig.ttlMs ?? 60000); }
130-136: Consider awaiting pending cache creations before disposal.If
disposeAllCachesis called while cache creation is in progress, the pending promise may resolve and attempt to use a cleared map. For robust cleanup:♻️ Suggested improvement
export async function disposeAllCaches(): Promise<void> { + // Wait for any pending creations to complete + await Promise.allSettled(pendingCreation.values()); + for (const cache of cacheByScope.values()) { await cache.dispose(); } cacheByScope.clear(); pendingCreation.clear(); }
72-83: Consider reusingSkillHttpCacheRedisOptionsfrom the factory module.The
CacheConfig.redisproperty mirrorsSkillHttpCacheRedisOptionsfromskill-http-cache.factory.ts. Importing and reusing that type would reduce duplication.♻️ Suggested change
+import type { SkillHttpCacheRedisOptions } from './skill-http-cache.factory.js'; -interface CacheConfig { - enabled?: boolean; - redis?: { - provider: 'redis' | 'ioredis' | 'vercel-kv' | '@vercel/kv'; - host?: string; - port?: number; - password?: string; - db?: number; - }; - ttlMs?: number; - keyPrefix?: string; -} +interface CacheConfig { + enabled?: boolean; + redis?: SkillHttpCacheRedisOptions; + ttlMs?: number; + keyPrefix?: string; +}libs/sdk/src/skill/skill-scope.helper.ts (1)
103-107: Consider initializingownerRefinside the loop.The
refproperty is initialized toundefinedwith an unsafe cast, then immediately overwritten in each loop iteration. Moving the object creation inside the loop would be cleaner and avoid the temporary invalid state.♻️ Suggested improvement
- const ownerRef: EntryOwnerRef = { - kind: 'scope', - id: '_skills', - ref: undefined as unknown as new (...args: unknown[]) => unknown, - }; - for (const SkillToolClass of skillTools) { try { const toolRecord = normalizeTool(SkillToolClass); - - // Update owner ref for each tool - ownerRef.ref = SkillToolClass; + const ownerRef: EntryOwnerRef = { + kind: 'scope', + id: '_skills', + ref: SkillToolClass, + }; const toolEntry = new ToolInstance(toolRecord, providers, ownerRef);libs/sdk/src/skill/cache/skill-http-cache.ts (1)
245-278: RedisKEYScommand can block the server on large keyspaces.The
keys()method (line 248, 269) uses the RedisKEYScommand which is O(N) and blocks the Redis server. For production workloads with many keys, consider usingSCANinstead.The codebase already has a SCAN-based implementation in
libs/utils/src/storage/adapters/redis.ts(lines 295-312) that could serve as a reference.Additionally, the empty catch blocks silently swallow all errors during invalidation. While this may be intentional for resilience, consider logging at debug level to aid troubleshooting.
libs/sdk/src/skill/cache/skill-http-cache.factory.ts (2)
105-118: Avoid non-null assertion; narrow the type instead.Line 110 uses
options.redis!after thehasRedisProvidercheck. While logically safe, per coding guidelines, prefer narrowing the type explicitly.♻️ Suggested improvement
// Check if Redis is configured - if (hasRedisProvider(options.redis)) { + const redisConfig = options.redis; + if (redisConfig && hasRedisProvider(redisConfig)) { try { // Lazy-load Redis client factory // Note: This assumes a createRedisClient exists in common/redis // For now, we'll create a simple client wrapper - const cache = await createRedisCache(options.redis!, keyPrefix, ttlMs, logger); + const cache = await createRedisCache(redisConfig, keyPrefix, ttlMs, logger); logger?.verbose('Created Redis-backed skill HTTP cache', { keyPrefix, ttlMs }); return { cache, type: 'redis' };
149-169: Consider adding connection error handling context.The
client.connect()call (line 162) is within the try block, so errors will trigger the fallback to memory cache. However, the error message in the catch (line 114-116) is generic. Adding context about which Redis operation failed would aid debugging.♻️ Suggested improvement in createRedisCache
await client.connect(); + // Connection successful - return Redis cache return new RedisSkillHttpCache({Or wrap connect specifically:
try { await client.connect(); } catch (error) { throw new Error(`Redis connection failed: ${error instanceof Error ? error.message : String(error)}`); }libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
144-144: Minor type refinement suggestion.The type cast could be slightly more precise to match
detectSkillsOnlyMode's expected signature.Optional: Align type cast with function signature
- const query = request.query as Record<string, string | string[]> | undefined; + const query = request.query as Record<string, string | string[] | undefined> | undefined;libs/sdk/src/common/metadata/skill.metadata.ts (1)
351-355: Consider relocating the type alias for better readability.The
SkillVisibilitytype alias is positioned between the schema section header comment and the actual schema. Moving it closer to theSkillMetadatainterface (around line 291) would improve code organization.Suggested relocation
Move the type alias to just after the
SkillMetadatainterface closing brace:// After line 291 (end of SkillMetadata interface): /** * Visibility mode for skill discovery. * Controls which mechanisms can find this skill. */ export type SkillVisibility = 'mcp' | 'http' | 'both';libs/sdk/src/skill/flows/http/skills-api.flow.ts (3)
224-278: Replaceanytypes with proper interfaces.The
skillRegistry: anyandtoolRegistry: anyparameters violate the coding guideline to avoidanytypes. Use the proper interface typesSkillRegistryInterfaceandToolRegistryInterface(or similar) for type safety.♻️ Suggested type improvements
- private async handleGetSkill(skillId: string, skillRegistry: any, toolRegistry: any) { + private async handleGetSkill( + skillId: string, + skillRegistry: SkillRegistryInterface, + toolRegistry: ToolRegistryInterface + ) {Similar changes needed for
handleSearchSkillsandhandleListSkillsmethods.
280-312: Use proper types instead ofanyfor search results.The callback parameters use
anytype which reduces type safety. Consider defining an interface for search results.- private async handleSearchSkills( - query: string, - options: { tags?: string[]; tools?: string[]; limit?: number }, - skillRegistry: any, - ) { + private async handleSearchSkills( + query: string, + options: { tags?: string[]; tools?: string[]; limit?: number }, + skillRegistry: SkillRegistryInterface, + ) {
211-222: Non-null assertions onskillIdandqueryviolate guidelines.Per coding guidelines, avoid non-null assertions (
!) and use proper error handling instead. While the state machine ensures these are set, the assertions could mask bugs if the state logic changes.♻️ Defensive alternative
case 'get': - await this.handleGetSkill(skillId!, skillRegistry, toolRegistry); + if (!skillId) { + this.respond(httpRespond.json({ error: 'Bad Request', message: 'Missing skill ID' }, { status: 400 })); + return; + } + await this.handleGetSkill(skillId, skillRegistry, toolRegistry); break; case 'search': - await this.handleSearchSkills(query!, { tags, tools, limit }, skillRegistry); + if (!query) { + this.respond(httpRespond.json({ error: 'Bad Request', message: 'Missing query' }, { status: 400 })); + return; + } + await this.handleSearchSkills(query, { tags, tools, limit }, skillRegistry); break;apps/e2e/demo-e2e-skills/e2e/skills-http.e2e.test.ts (1)
13-40: Consider importing shared types instead of duplicating.The
SkillApiResponse,SkillsListResponse, andSkillDetailResponseinterfaces are defined locally but appear to duplicate types fromlibs/sdk/src/skill/skill-http.utils.ts. Consider importing from the SDK to prevent type drift if the source definitions change.+import type { SkillApiResponse } from '@frontmcp/sdk'; + +interface SkillsListResponse { + skills: SkillApiResponse[]; + total: number; +} + +interface SkillDetailResponse extends SkillApiResponse { + instructions?: string; + formattedContent?: string; +} -interface SkillApiResponse { - id: string; - name: string; - // ... rest of duplicated definition -}libs/sdk/src/skill/__tests__/skill-http.utils.test.ts (1)
12-46: Consider typing the mock more precisely to avoidas unknown ascast.The
createMockSkillEntryfunction usesas unknown as SkillEntrycast at line 45. While acceptable for tests, a more precise partial mock type would improve maintainability.apps/e2e/demo-e2e-skills/e2e/skills-only-mode.e2e.test.ts (1)
15-27: Consider using shared types instead of local interface definition.The
LoadSkillResultinterface duplicates the output type structure fromLoadSkillsTool. Consider importing the output type from the SDK or a shared types module to keep the test in sync with the implementation.libs/sdk/src/skill/auth/skill-http-auth.ts (2)
131-141: Consider using timing-safe comparison for API key validation.The direct
apiKeys.includes(apiKeyHeader)comparison at lines 131 and 138 is vulnerable to timing attacks. While the risk may be low depending on context, using timing-safe comparison is a security best practice for credential validation.🔒 Proposed fix using timing-safe comparison
+import { timingSafeEqual } from '@frontmcp/utils'; + +// Helper function for timing-safe string comparison +private timingSafeIncludes(keys: string[], candidate: string): boolean { + return keys.some(key => { + if (key.length !== candidate.length) return false; + return timingSafeEqual(Buffer.from(key), Buffer.from(candidate)); + }); +} // In validateApiKey: - if (apiKeyHeader && apiKeys.includes(apiKeyHeader)) { + if (apiKeyHeader && this.timingSafeIncludes(apiKeys, apiKeyHeader)) { return { authorized: true }; }As per coding guidelines, cryptographic operations should be imported from
@frontmcp/utils.
211-217: Header lookup doesn't fully handle case-insensitive matching.The
getHeadermethod checks bothheaders[name]andheaders[name.toLowerCase()], but if the caller passes a lowercase name (e.g.,'authorization'), this won't find headers stored with different casing (e.g.,'Authorization'). Consider normalizing to lowercase consistently.♻️ Proposed fix for consistent case-insensitive header lookup
private getHeader(headers: Record<string, string | string[] | undefined>, name: string): string | undefined { - const value = headers[name] ?? headers[name.toLowerCase()]; + const lowerName = name.toLowerCase(); + // Find header case-insensitively + const key = Object.keys(headers).find(k => k.toLowerCase() === lowerName); + const value = key ? headers[key] : undefined; if (Array.isArray(value)) { return value[0]; } return value; }libs/sdk/src/skill/flows/load-skill.flow.ts (1)
275-317: Avoid repeated tool registry scans during schema enrichment.
getTools(true)is searched per tool; pre-indexing once reduces O(n·m) work.♻️ Suggested refactor
- for (const { loadResult, activationResult } of loadResults) { + const toolEntryByName = toolRegistry + ? new Map(toolRegistry.getTools(true).map((te) => [te.name, te])) + : null; + + for (const { loadResult, activationResult } of loadResults) { const { skill, availableTools, missingTools, isComplete, warning } = loadResult; ... const tools = skill.tools.map((t) => { const isAvailable = availableTools.includes(t.name); const result: { name: string; purpose?: string; available: boolean; inputSchema?: unknown; outputSchema?: unknown; } = { name: t.name, purpose: t.purpose, available: isAvailable, }; // Include schemas for available tools if (isAvailable && toolRegistry) { - const toolEntry = toolRegistry.getTools(true).find((te) => te.name === t.name); + const toolEntry = toolEntryByName?.get(t.name); if (toolEntry) { if (toolEntry.rawInputSchema) { result.inputSchema = toolEntry.rawInputSchema; } const rawOutput = toolEntry.getRawOutputSchema?.() ?? toolEntry.rawOutputSchema; if (rawOutput) { result.outputSchema = rawOutput; } } } return result; });libs/sdk/src/skill/skill.instance.ts (1)
196-204: Align sync content path with enriched metadata.
getContentSyncreturns base content without tags/priority/visibility; mirroringload()keeps search/visibility consistent when the sync path is used.♻️ Suggested refactor
- if (typeof this.metadata.instructions === 'string') { - return buildSkillContent(this.metadata, this.metadata.instructions); - } + if (typeof this.metadata.instructions === 'string') { + const baseContent = buildSkillContent(this.metadata, this.metadata.instructions); + this.cachedContent = { + ...baseContent, + tags: this.tags, + priority: this.priority, + hideFromDiscovery: this.hidden, + visibility: this.skillVisibility, + }; + return this.cachedContent; + }libs/sdk/src/skill/skill-http.utils.ts (1)
122-183: Pre-index tool entries to avoid repeated registry scans.
getTools(true)is searched inside the per-tool loop; caching the map once reduces overhead.♻️ Suggested refactor
export function formatSkillForLLMWithSchemas( skill: SkillContent, availableTools: string[], missingTools: string[], toolRegistry: ToolRegistryInterface, ): string { const parts: string[] = []; + const toolEntryByName = new Map(toolRegistry.getTools(true).map((t) => [t.name, t])); // Header parts.push(`# Skill: ${skill.name}`); ... for (const tool of skill.tools) { const isAvailable = availableTools.includes(tool.name); const status = isAvailable ? '✓' : '✗'; parts.push(`### [${status}] ${tool.name}`); if (tool.purpose) { parts.push(`**Purpose:** ${tool.purpose}`); } // Include full schema if tool is available if (isAvailable) { - const toolEntry = toolRegistry.getTools(true).find((t) => t.name === tool.name); + const toolEntry = toolEntryByName.get(tool.name); if (toolEntry) { const inputSchema = getToolInputSchema(toolEntry); const outputSchema = toolEntry.getRawOutputSchema?.() ?? toolEntry.rawOutputSchema; ... } }libs/sdk/src/common/types/options/skills-http/schema.ts (2)
79-87: Consider renaming the z.infer type to avoid collision with the interface.
SkillsConfigEndpointConfigis defined in bothschema.ts(viaz.infer) andinterfaces.ts(as an interface), but with different semantics:
- Interface version:
enabled?: boolean(optional)- z.infer version:
enabled: boolean(required, due to.default(true))The barrel exports the interface version, so direct imports from
schema.tswould yield a different type. Consider renaming this toSkillsConfigEndpointConfigOutputor similar to distinguish it from the input interface.Suggested rename
/** * Skills HTTP endpoint config type (with defaults applied). */ -export type SkillsConfigEndpointConfig = z.infer<typeof skillsConfigEndpointConfigSchema>; +export type SkillsConfigEndpointConfigOutput = z.infer<typeof skillsConfigEndpointConfigSchema>;
145-148: Redundant null coalescing forauth.The
authfield already has.default('inherit')in the schema (line 59), soparsed.authwill always be defined after parsing. The?? 'inherit'fallback is redundant.Remove redundant fallback
const parsed = skillsConfigOptionsSchema.parse(options ?? {}); const prefix = parsed.prefix ?? ''; - const auth = parsed.auth ?? 'inherit'; + const auth = parsed.auth; const apiKeys = parsed.apiKeys;
…date related tests
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/e2e/demo-e2e-skills/e2e/plugin-skills.e2e.test.ts (1)
227-243: Use proper null safety in tests instead of non-null assertions.Lines 240 and 242 use non-null assertions (
!) after checking the tool is defined. Replace with optional chaining or conditional guards for cleaner test code.Suggested fix
expect(deployTool).toBeDefined(); - expect(deployTool!.purpose).toContain('Deploy the application'); + expect(deployTool?.purpose).toContain('Deploy the application'); expect(rollbackTool).toBeDefined(); - expect(rollbackTool!.purpose).toContain('Rollback if needed'); + expect(rollbackTool?.purpose).toContain('Rollback if needed');Avoid non-null assertions per coding guidelines; optional chaining is more readable here.
apps/e2e/demo-e2e-skills/e2e/load-skill.e2e.test.ts (1)
194-230: Avoid non-null assertions in tool and parameter checks.Use explicit guards for
githubGetPrandskill.parametersto keep strict TypeScript happy and improve error clarity.Suggested fix
const githubGetPr = 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) { + throw new Error('Expected github_get_pr tool to be present'); + } + expect(githubGetPr.purpose).toBeDefined(); + expect(githubGetPr.purpose).toContain('Fetch PR details');- const prUrlParam = skill.parameters!.find((p) => p.name === 'pr_url'); + if (!skill.parameters) { + throw new Error('Expected skill parameters to be present'); + } + const prUrlParam = skill.parameters.find((p) => p.name === 'pr_url'); expect(prUrlParam).toBeDefined(); - expect(prUrlParam!.required).toBe(true); - expect(prUrlParam!.type).toBe('string'); + if (!prUrlParam) { + throw new Error('Expected pr_url parameter to be present'); + } + expect(prUrlParam.required).toBe(true); + expect(prUrlParam.type).toBe('string');Per coding guidelines, avoid non-null assertions (
!) in TypeScript files. Use explicit error guards instead.
🤖 Fix all issues with AI agents
In `@apps/e2e/demo-e2e-skills/e2e/load-skill.e2e.test.ts`:
- Around line 280-292: The test uses a non-null assertion on the optional field
combinedWarnings (e.g., in the test calling loadSkills and checking
content.summary.combinedWarnings!), so change the assertion to safely handle the
optional: destructure combinedWarnings from content.summary (or check
content.summary first) and assert that combinedWarnings is an array and then
that combinedWarnings.some(...) is true (or explicitly fail if combinedWarnings
is undefined). Apply the same pattern to the other occurrences referenced (the
assertions around lines 207–208 and 226–229) to remove all `!` non-null
assertions and replace with a guarded check before calling .some().
🧹 Nitpick comments (5)
libs/sdk/src/skill/flows/load-skill.flow.ts (3)
103-103: Consider usingz.custom<T>()instead of type assertion onz.unknown().The current pattern
z.unknown().optional() as z.ZodType<...>works but the type assertion can be made cleaner withz.custom<T>(), which explicitly signals that runtime validation is intentionally skipped for this internal state field.Suggested change
- loadResults: z.unknown().optional() as z.ZodType<LoadResultWithActivation[] | undefined>, + loadResults: z.custom<LoadResultWithActivation[] | undefined>().optional(),
232-247: MovesetPolicyModecall outside the loop.
setPolicyModeappears to be a session-level setting rather than per-skill. Calling it inside the loop is redundant (same value each iteration) and slightly misleading. Moving it before or after the loop clarifies intent and avoids redundant calls.Suggested refactor
+ // Override policy mode if specified (session-level setting) + if (policyMode) { + sessionManager.setPolicyMode(policyMode as SkillPolicyMode); + } + // Activate each skill for (const item of loadResults) { const { skill } = item.loadResult; const activationResult = sessionManager.activateSkill(skill.id, skill, item.loadResult); - // Override policy mode if specified - if (policyMode) { - sessionManager.setPolicyMode(policyMode as SkillPolicyMode); - } - item.activationResult = activationResult; this.logger.info(`activateSessions: activated skill "${skill.id}"`, { policyMode: activationResult.session.policyMode, allowedTools: activationResult.availableTools, }); }
303-315: Consider caching the tool registry lookup to avoid repeated iteration.
toolRegistry.getTools(true)is called inside the nested loop for every tool in every skill. Building a lookup Map once before the loop would reduce complexity from O(skills × tools × registrySize) to O(registrySize + skills × tools).Suggested optimization
const toolRegistry = (this.scope as Scope).tools; const skillResults: z.infer<typeof skillResultSchema>[] = []; let totalTools = 0; let allToolsAvailable = true; + + // Build tool lookup map once + const toolLookup = toolRegistry + ? new Map(toolRegistry.getTools(true).map((t) => [t.name, t])) + : undefined; for (const { loadResult, activationResult } of loadResults) { // ... existing code ... const tools = skill.tools.map((t) => { const isAvailable = availableTools.includes(t.name); const result: { /* ... */ } = { /* ... */ }; // Include schemas for available tools - if (isAvailable && toolRegistry) { - const toolEntry = toolRegistry.getTools(true).find((te) => te.name === t.name); + if (isAvailable && toolLookup) { + const toolEntry = toolLookup.get(t.name); if (toolEntry) { // ... existing schema extraction ... } } return result; });apps/e2e/demo-e2e-skills/e2e/plugin-skills.e2e.test.ts (1)
27-56: Consider centralizing SkillResult/LoadSkillsResult test typings.These interfaces are repeated across multiple e2e suites; extracting them into a shared test helper reduces drift.
apps/e2e/demo-e2e-skills/e2e/load-skill.e2e.test.ts (1)
14-50: Align SkillParameter typing with SDK metadata to prevent drift.The SDK's
SkillParametertype has a union type fortypefield and includes adefaultfield. Mirroring it here keeps test typings consistent with the SDK.Suggested typing alignment
interface SkillParameter { name: string; description?: string; required?: boolean; - type?: string; + type?: 'string' | 'number' | 'boolean' | 'object' | 'array'; + default?: unknown; }
There was a problem hiding this comment.
Actionable comments posted: 5
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/skill/tools/search-skills.tool.ts (1)
22-43: Update searchSkills output schema in SDK docs.The documented response in
docs/draft/docs/servers/skills.mdxis missing two fields now present in the actual implementation:
canExecute(per skill): Whether all required tools are availableguidance(root level): Suggested next action based on search resultsUpdate the JSON example under the searchSkills section to include these fields and their descriptions to match the current schema.
🤖 Fix all issues with AI agents
In `@libs/sdk/src/common/types/options/skills-http/interfaces.ts`:
- Around line 151-161: The example is ambiguous about whether
skillsConfig.prefix is prepended to child paths or if llmTxt.path is absolute;
update the docs and example to state the precedence explicitly and make the
example consistent: in the FrontMcp sample clarify that skillsConfig.prefix is
applied to relative child paths and that llmTxt.path can be absolute to override
the prefix (or vice versa), and then adjust the llmTxt.path value or add a
comment to show the intended behavior (reference: FrontMcp, skillsConfig,
prefix, llmTxt.path).
- Around line 175-300: The pull request added the SkillsConfigOptions API
(symbols: SkillsConfigOptions, enabled, prefix, auth, apiKeys, jwt, llmTxt,
llmFullTxt, api, mcpTools, cache) but no docs were added; update the docs/draft
site by adding a new section (or file docs/draft/docs/servers/skills.mdx) that
documents each option, example configurations for enabling endpoints and prefix
usage, auth modes ('inherit'|'public'|'api-key'|'bearer') with examples showing
apiKeys and JWT fields, examples for llmTxt/llmFullTxt/api endpoint toggles, how
to disable mcpTools, and cache setup (memory and Redis examples) so the SDK
change is fully documented for publishable changes.
In `@libs/sdk/src/skill/flows/http/skills-api.flow.ts`:
- Around line 171-183: The request query parsing can receive arrays and produce
NaN for numeric params; update the normalization for
query/tags/tools/limit/offset: coerce query to a single string (if
request.query['query'] is string[] join by ',' or take first), ensure
tagsParam/toolsParam are normalized to string[] (if string split by ',' and
trim, filter out empty strings), and parse limitParam/offsetParam only after
validating they are numeric (use Number.isFinite on Number(value) or /^\d+$/
test) falling back to undefined or a default when invalid; adjust the variables
query, tags, tools, limit, offset accordingly so downstream pagination/search
won't receive NaN or unexpected types.
- Around line 272-276: The map callback uses an unnecessary any type; remove the
explicit "(t: any)" and let TypeScript infer the correct type from skill.tools
(typed as Array<{ name: string; purpose?: string; required?: boolean }>), i.e.,
change the mapping in the tools assignment so it uses a properly typed parameter
(or no explicit type) in the arrow function that builds name/purpose/available,
referencing the current mapping expression where tools: skill.tools.map(...) is
defined.
- Around line 214-223: Replace the non-null assertions for skillId and query
with explicit guards that return a 400 response when missing: before calling
handleGetSkill(skillId!, ...) check if skillId is undefined and call the same
error response helper used elsewhere in this file (match the existing validation
pattern) and return; similarly, before calling handleSearchSkills(query!, ...)
check if query is undefined and respond with a 400 error and return; update the
switch cases for 'get' and 'search' to perform these checks and only call
handleGetSkill and handleSearchSkills when the validated parameter is present.
🧹 Nitpick comments (4)
libs/sdk/src/common/entries/tool.entry.ts (1)
87-121: Consider adding runtime validation before castingrawInputSchema.Line 90 casts
rawInputSchemadirectly toRecord<string, unknown>without verifying it's actually an object. IfrawInputSchemais a non-object value (e.g., a string or number), this could cause unexpected behavior downstream.🛠️ Suggested improvement
getInputJsonSchema(): Record<string, unknown> | null { // Prefer rawInputSchema if already in JSON Schema format if (this.rawInputSchema) { + if (typeof this.rawInputSchema === 'object' && this.rawInputSchema !== null) { return this.rawInputSchema as Record<string, unknown>; + } + // rawInputSchema exists but isn't an object - fall through to conversion }libs/sdk/src/skill/cache/skill-http-cache.factory.ts (1)
151-171: Consider documenting connection lifecycle expectations for callers.The Redis client is created with
lazyConnect: truebutconnect()is called immediately. This works correctly, but callers should be aware that the returned cache holds an active Redis connection that should be disposed viacache.dispose()when no longer needed. Consider adding a JSDoc note about connection lifecycle.libs/sdk/src/skill/cache/skill-http-cache.holder.ts (1)
90-107: Inconsistent logging:console.warnvs logger.The factory (
createSkillHttpCache) accepts a logger parameter, butcreateCacheForScopeusesconsole.warndirectly. Consider passing the scope's logger tocreateSkillHttpCachefor consistent logging behavior.♻️ Suggested improvement
-async function createCacheForScope(scopeId: string, cacheConfig: CacheConfig): Promise<SkillHttpCache> { +async function createCacheForScope( + scopeId: string, + cacheConfig: CacheConfig, + logger?: FrontMcpLogger, +): Promise<SkillHttpCache> { try { const { cache } = await createSkillHttpCache({ redis: cacheConfig.redis, ttlMs: cacheConfig.ttlMs, keyPrefix: cacheConfig.keyPrefix ?? `frontmcp:skills:${scopeId}:cache:`, + logger, }); return cache; } catch (error) { - console.warn( - `[SkillHttpCache] Failed to create Redis cache for scope "${scopeId}", falling back to memory cache:`, - error instanceof Error ? error.message : String(error), - ); + logger?.warn(`Failed to create Redis cache for scope "${scopeId}", falling back to memory cache`, { + error: error instanceof Error ? error.message : String(error), + }); return new MemorySkillHttpCache(cacheConfig.ttlMs ?? 60000); } }Then update the call site to pass
scope.logger:- const creationPromise = createCacheForScope(scopeId, cacheConfig); + const creationPromise = createCacheForScope(scopeId, cacheConfig, scope.logger);libs/sdk/src/skill/flows/load-skill.flow.ts (1)
232-241: Move policy mode setup outside the loop to avoid redundant calls.
activateSkilldoesn't use policy mode to computeallowedTools(those come from skill content). However, setting policy mode once before the loop is more efficient than callingsetPolicyModefor each skill activation. This ensures consistent policy across all activations without redundant updates.🛠️ Proposed fix
+ if (policyMode) { + sessionManager.setPolicyMode(policyMode as SkillPolicyMode); + } + // Activate each skill for (const item of loadResults) { const { skill } = item.loadResult; const activationResult = sessionManager.activateSkill(skill.id, skill, item.loadResult); - - // Override policy mode if specified - if (policyMode) { - sessionManager.setPolicyMode(policyMode as SkillPolicyMode); - }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/skill/flows/load-skill.flow.ts`:
- Around line 291-316: When building the tools array in the skill loader,
inputSchema is only assigned from toolEntry.rawInputSchema, so Zod-defined tools
that expose a getInputJsonSchema() are omitted; update the mapping inside the
tools construction (the block that references toolEntry, toolEntryByName, and
result) to set result.inputSchema to toolEntry.rawInputSchema if present,
otherwise call toolEntry.getInputJsonSchema?.() and assign that value when
available, similar to how rawOutput/getRawOutputSchema are handled for
outputSchema.
♻️ Duplicate comments (1)
libs/sdk/src/common/entries/tool.entry.ts (1)
87-118: Fix Zod v4 JSON Schema conversion path.
require('zod/v4/core')is likely to throw in Zod v4 wheretoJSONSchemais exposed onz, causing the method to return the empty fallback for most tools. This breaks schema exposure for HTTP/LLM consumers.🔧 Proposed fix
- const { z } = require('zod'); - let toJSONSchema: (schema: unknown) => Record<string, unknown>; - try { - // Zod v4: toJSONSchema is in 'zod/v4/core' - // eslint-disable-next-line `@typescript-eslint/no-require-imports` - const zodV4 = require('zod/v4/core'); - toJSONSchema = zodV4.toJSONSchema; - } catch { - // Zod v3: toJSONSchema may be available as zod-to-json-schema or not at all - // In this case, return a basic schema - return { type: 'object', properties: {} }; - } - return toJSONSchema(z.object(this.inputSchema)); + const { z } = require('zod'); + if (typeof z.toJSONSchema === 'function') { + return z.toJSONSchema(z.object(this.inputSchema)) as Record<string, unknown>; + } + return { type: 'object', properties: {} };In Zod v4, where is toJSONSchema exported? Is it accessed via z.toJSONSchema or via zod/v4/core?
🧹 Nitpick comments (2)
libs/sdk/src/tool/tool.utils.ts (1)
449-467: Prefertool.getInputJsonSchema()to avoid unsafe casts and missed conversions.
This avoids castingunknownblindly and ensures Zod-based schemas are converted consistently via the new ToolEntry API.♻️ Proposed refactor
- let parameters: Record<string, unknown>; - if (tool.rawInputSchema) { - // Already converted to JSON Schema - parameters = tool.rawInputSchema as Record<string, unknown>; - } else if (tool.inputSchema && Object.keys(tool.inputSchema).length > 0) { - // tool.inputSchema is a ZodRawShape (extracted .shape from ZodObject in ToolInstance constructor) - // Convert to JSON Schema using the same approach as tools-list.flow.ts - try { - parameters = toJSONSchema(z.object(tool.inputSchema)) as Record<string, unknown>; - } catch { - parameters = { type: 'object', properties: {} }; - } - } else { - // No schema defined - use empty object schema - parameters = { type: 'object', properties: {} }; - } + const parameters = + tool.getInputJsonSchema?.() ?? + (tool.inputSchema && Object.keys(tool.inputSchema).length > 0 + ? (toJSONSchema(z.object(tool.inputSchema)) as Record<string, unknown>) + : { type: 'object', properties: {} });libs/sdk/src/skill/flows/load-skill.flow.ts (1)
52-58: TightenSkillParameter.typeto match metadata union.
The metadata defines a fixed set of types; using an enum keeps the output schema aligned and self-validating.♻️ Suggested adjustment
- type: z.string().optional(), + type: z.enum(['string', 'number', 'boolean', 'object', 'array']).optional(),
…or improved flexibility
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.