feat: Implement skills HTTP configuration and visibility management #222
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive HTTP endpoint support for skills with visibility control, authentication, and caching. It introduces skill visibility modes (mcp/http/both), HTTP flows for serving skill content, authentication validators, Redis-backed caching, skills-only mode, and refactors skill loading to handle multiple skills. Includes E2E tests validating visibility filtering, API endpoints, and skills-only mode behavior. Changes
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;
| getInputJsonSchema(): Record<string, unknown> | null { | ||
| // Prefer rawInputSchema if already in JSON Schema format | ||
| if (this.rawInputSchema) { | ||
| return this.rawInputSchema; | ||
| } | ||
|
|
||
| // Convert Zod schema shape to JSON Schema | ||
| if (this.inputSchema && Object.keys(this.inputSchema).length > 0) { | ||
| try { | ||
| // Dynamic import to avoid circular dependencies | ||
| const { z, toJSONSchema } = require('zod'); | ||
| return toJSONSchema(z.object(this.inputSchema)); | ||
| } catch { | ||
| return { type: 'object', properties: {} }; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "package.json" -type f | head -5Repository: agentfront/frontmcp
Length of output: 191
🏁 Script executed:
cat package.json | grep -A 5 -B 5 '"zod"'Repository: agentfront/frontmcp
Length of output: 343
🏁 Script executed:
head -120 libs/sdk/src/common/entries/tool.entry.ts | tail -60Repository: agentfront/frontmcp
Length of output: 2071
🌐 Web query:
Zod 4 toJSONSchema export availability direct import from 'zod'
💡 Result:
In Zod v4, JSON Schema export is built in as z.toJSONSchema() (a function on the Zod namespace), not as a standalone toJSONSchema named export. So you use it like:
import * as z from "zod";
const schema = z.object({ name: z.string() });
const jsonSchema = z.toJSONSchema(schema);[1]
Import path depends on which Zod you have installed:
- If you’re on
zod@^4.0.0, the package root"zod"exports Zod 4, so the code above is correct. [2] - If you’re on the transition period where Zod 4 lived under a subpath (e.g.
zod@3.25.x), you’d import v4 from"zod/v4". [3]
Sources:
[1] [2] [3]
🏁 Script executed:
grep -n "rawInputSchema" libs/sdk/src/common/entries/tool.entry.ts | head -20Repository: agentfront/frontmcp
Length of output: 311
🏁 Script executed:
sed -n '1,100p' libs/sdk/src/common/entries/tool.entry.ts | grep -A 5 -B 5 "rawInputSchema"Repository: agentfront/frontmcp
Length of output: 1181
Fix toJSONSchema usage and remove any types.
The code attempts to destructure toJSONSchema as a named export from 'zod', but in Zod v4 it's only available as z.toJSONSchema() (a method on the z namespace). This will fail silently, causing the method to always return an empty schema fallback instead of converting the input schema.
Change line 97-98 to:
const { z } = require('zod');
return z.toJSONSchema(z.object(this.inputSchema));Additionally, rawInputSchema and rawOutputSchema are typed as any (line 55) without justification. Per coding guidelines, use unknown instead or a more specific type. Consider logging when the fallback is triggered to surface conversion failures.
🤖 Prompt for AI Agents
In `@libs/sdk/src/common/entries/tool.entry.ts` around lines 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.
| normalizedLlmTxt: normalizeEndpointConfig(parsed.llmTxt, `${prefix}/llm.txt`, auth, apiKeys), | ||
| normalizedLlmFullTxt: normalizeEndpointConfig(parsed.llmFullTxt, `${prefix}/llm_full.txt`, auth, apiKeys), | ||
| normalizedApi: normalizeEndpointConfig(parsed.api, `${prefix}/skills`, auth, apiKeys), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the schema file mentioned in the review
wc -l libs/sdk/src/common/types/options/skills-http/schema.tsRepository: agentfront/frontmcp
Length of output: 122
🏁 Script executed:
# Read the schema file to understand prefix definition and validation
head -200 libs/sdk/src/common/types/options/skills-http/schema.ts | tail -100Repository: agentfront/frontmcp
Length of output: 3315
🏁 Script executed:
# Search for normalizeEndpointConfig implementation
rg -n 'normalizeEndpointConfig' --type=ts -g '!node_modules' -A 10Repository: agentfront/frontmcp
Length of output: 9788
🏁 Script executed:
# Search for prefix definition in skills-http related files
rg -n 'prefix.*:' libs/sdk/src/common/types/options/skills-http/ --type=ts -A 2Repository: agentfront/frontmcp
Length of output: 897
🏁 Script executed:
# Check test file for prefix normalization tests
rg -n 'prefix' libs/sdk/src/common/types/options/__tests__/skills-http.options.test.ts -A 5Repository: agentfront/frontmcp
Length of output: 1484
🏁 Script executed:
# Search for any prefix normalization or validation logic
rg -n 'prefix' libs/sdk/src/common/types/options/skills-http/ --type=ts -B 2 -A 2Repository: agentfront/frontmcp
Length of output: 3540
🏁 Script executed:
# Check the full interfaces documentation for prefix
sed -n '180,195p' libs/sdk/src/common/types/options/skills-http/interfaces.tsRepository: agentfront/frontmcp
Length of output: 596
🏁 Script executed:
# Search for actual usage of the normalized paths to see if invalid paths would cause issues
rg -n 'normalizedLlmTxt\|normalizedLlmFullTxt\|normalizedApi' libs/sdk/src --type=ts -B 2 -A 2 | head -100Repository: agentfront/frontmcp
Length of output: 45
Add prefix validation or explicit documentation for required format.
The prefix schema accepts any string without format validation. While the documentation example shows '/api' with a leading slash, a user providing prefix: 'api' would produce malformed paths like 'api/llm.txt' instead of '/api/llm.txt'. Either validate that the prefix is empty or starts with /, or add explicit documentation stating this requirement.
🤖 Prompt for AI Agents
In `@libs/sdk/src/common/types/options/skills-http/schema.ts` around lines 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.
| const provider = redis.provider; | ||
|
|
||
| if (provider === 'vercel-kv' || provider === '@vercel/kv') { | ||
| // Lazy-load Vercel KV - use require for CommonJS compatibility | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { kv } = require('@vercel/kv'); | ||
| return new RedisSkillHttpCache({ | ||
| getClient: async () => kv, | ||
| keyPrefix, | ||
| ttlMs, | ||
| }); | ||
| } | ||
|
|
||
| // Default to ioredis - use require for CommonJS compatibility | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const Redis = require('ioredis'); | ||
| const client = new Redis({ | ||
| host: redis.host ?? 'localhost', | ||
| port: redis.port ?? 6379, | ||
| password: redis.password, | ||
| db: redis.db, | ||
| lazyConnect: true, | ||
| enableReadyCheck: false, | ||
| maxRetriesPerRequest: 3, | ||
| }); | ||
|
|
||
| await client.connect(); | ||
|
|
||
| return new RedisSkillHttpCache({ | ||
| getClient: async () => client, | ||
| keyPrefix, | ||
| ttlMs, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ioredis vs redis npm package API differences
💡 Result:
ioredis vs redis (node-redis) — key API differences
1) Creating a client / connecting
redis(node-redis v4+):createClient(...)then explicitawait client.connect(); common pattern is chaining.connect()offcreateClient(); supports.isReady/.isOpen. [1]- ioredis:
new Redis(...)creates a client and connects automatically;redis.connect()exists but is typically unnecessary. [2]
2) Promises vs callbacks
redis: modern API is Promise-first (examples areawait client.get(...),await client.set(...)). [1]- ioredis: supports both Promises and Node-style callbacks across many APIs (e.g.,
pipeline().exec((err, results) => ...)). [3]
3) Disconnecting / shutting down
redis:await client.quit()= graceful (flush queued commands)await client.disconnect()= immediate (may drop pending work) [4]
- ioredis:
redis.disconnect()= immediate- recommends
quitif you need to wait for pending replies [2]
4) Pub/Sub API shape
redis: subscribing requires a dedicated connection, typically viaconst sub = client.duplicate(); await sub.connect(); await sub.subscribe('channel', cb). [5]- ioredis: once you call
subscribe()/psubscribe(), that connection enters subscriber mode (only subscription-related commands allowed); you usually create a second connection for publishing/regular commands; messages arrive via"message"events. [6]
5) Pipelining & transactions
redis: supports auto-pipelining (commands issued in the same tick can be pipelined automatically; you often usePromise.all(...)to await them). [4]- ioredis: explicit batching via
redis.pipeline();multi()defaults to creating a pipeline-like chain that you finish with.exec(). [3]
6) Defining custom script-backed commands
redis:defineScript(...)(attach scripts underscripts: { ... }increateClientoptions). [4]- ioredis:
defineCommand(name, { numberOfKeys, lua }). [7]
Sources: node-redis (redis) docs on npm [1][4][5], ioredis API/README/GitHub docs [2][3][6][7].
🏁 Script executed:
# Find where hasRedisProvider is defined and what it accepts
rg "hasRedisProvider|'redis'|'ioredis'" --type ts -A 3 -B 1 | head -100Repository: agentfront/frontmcp
Length of output: 5921
🏁 Script executed:
# Check the factory file for context on provider handling
cat -n libs/sdk/src/skill/cache/skill-http-cache.factory.ts | head -200Repository: agentfront/frontmcp
Length of output: 5923
Remove or implement the 'redis' provider option.
The interface accepts provider: 'redis' (line 17) but the implementation (lines 149–160) defaults to ioredis for all non-Vercel providers, offering no distinct handling. The redis npm package (node-redis v4+) and ioredis have incompatible APIs—node-redis uses createClient() and requires explicit await connect(), while ioredis uses new Redis() with automatic connection. Code that passes provider: 'redis' would incorrectly use the ioredis constructor, causing runtime failures.
Either implement proper node-redis support or remove the 'redis' option from the interface to avoid confusion.
🤖 Prompt for AI Agents
In `@libs/sdk/src/skill/cache/skill-http-cache.factory.ts` around lines 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.
| const skillEntry = skillRegistry | ||
| .getSkills(true) | ||
| .find((s: any) => s.name === skill.name || s.metadata.id === skill.id); | ||
| if (skillEntry) { | ||
| const visibility = skillEntry.metadata.visibility ?? 'both'; | ||
| if (visibility === 'mcp') { | ||
| this.respond( | ||
| httpRespond.json( | ||
| { error: 'Skill not found', message: `Skill "${skillId}" not available via HTTP` }, | ||
| { status: 404 }, | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential inconsistency in skill visibility check.
The visibility check compares against skill.name and skill.id but the skillEntry lookup uses .find() which may return a different skill if names collide. Additionally, when skillEntry is not found, the visibility check is skipped, potentially exposing MCP-only skills via HTTP if loadSkill returns a skill that isn't in getSkills(true).
🔧 Suggested fix
- const skillEntry = skillRegistry
- .getSkills(true)
- .find((s: any) => s.name === skill.name || s.metadata.id === skill.id);
- if (skillEntry) {
- const visibility = skillEntry.metadata.visibility ?? 'both';
- if (visibility === 'mcp') {
+ // Check visibility from loaded skill metadata directly
+ const visibility = skill.visibility ?? 'both';
+ if (visibility === 'mcp') {
+ this.respond(
+ httpRespond.json(
+ { error: 'Skill not found', message: `Skill "${skillId}" not available via HTTP` },
+ { status: 404 },
+ ),
+ );
+ return;
+ }This requires the SkillLoadResult.skill to include visibility metadata from the registry.
🤖 Prompt for AI Agents
In `@libs/sdk/src/skill/flows/http/skills-api.flow.ts` around lines 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.
| // Pagination info: | ||
| // - total: number of results returned (search already filtered by query/tags/tools) | ||
| // - hasMore: true if we hit the limit (indicating more results may exist) | ||
| const total = skills.length; | ||
| const hasMore = skills.length >= input.limit; | ||
|
|
There was a problem hiding this comment.
hasMore should reflect unfiltered search truncation.
After visibility filtering, skills.length can drop below the limit even when the registry returned a full page. Using the pre-filter results.length better signals potential additional results.
🐛 Proposed fix
- const total = skills.length;
- const hasMore = skills.length >= input.limit;
+ const total = skills.length;
+ const hasMore = results.length >= input.limit;🤖 Prompt for AI Agents
In `@libs/sdk/src/skill/tools/search-skills.tool.ts` around lines 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.
| private createTransport(): McpTransport { | ||
| // Build URL with query params if provided | ||
| let baseUrl = this.config.baseUrl; | ||
| if (this.config.queryParams && Object.keys(this.config.queryParams).length > 0) { | ||
| const params = new URLSearchParams(this.config.queryParams); | ||
| baseUrl = `${baseUrl}${baseUrl.includes('?') ? '&' : '?'}${params.toString()}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "baseUrl|messageEndpoint|request" libs/testing/src/transport/streamable-http.transport.ts -C 3Repository: agentfront/frontmcp
Length of output: 6992
🏁 Script executed:
# Check how baseUrl from mcp-test-client flows to StreamableHttpTransport
rg -n "new StreamableHttpTransport|createTransport" libs/testing/src/client/mcp-test-client.ts -A 5 -B 2Repository: agentfront/frontmcp
Length of output: 1077
🏁 Script executed:
# Search for tests related to queryParams in mcp-test-client
rg -n "queryParams" libs/testing --type ts -C 3Repository: agentfront/frontmcp
Length of output: 3622
🏁 Script executed:
# Search for test files that use queryParams or withQueryParams
rg -n "withQueryParams|queryParams.*mode|skills_only" libs/testing --type ts -l | head -5Repository: agentfront/frontmcp
Length of output: 162
🏁 Script executed:
# Search for actual usage of queryParams in test files
find . -name "*.test.ts" -o -name "*.spec.ts" | head -10Repository: agentfront/frontmcp
Length of output: 600
🏁 Script executed:
# Search for skills_only usage in test files
rg -n "skills_only|queryParams" ./apps/e2e --type ts -C 2 | head -50Repository: agentfront/frontmcp
Length of output: 4659
🏁 Script executed:
# Check the SSE transport to see if it also has this queryParams issue
rg -n "baseUrl|url = " libs/testing/src/transport/sse.transport.ts -C 2Repository: agentfront/frontmcp
Length of output: 146
🏁 Script executed:
# Find the SSE transport file
find . -name "*sse*transport*" -type fRepository: agentfront/frontmcp
Length of output: 179
🏁 Script executed:
# Check how SSE transport builds URLs
rg -n "baseUrl|url = " libs/sdk/src/transport/adapters/sse-transport.ts -C 2Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Check the SSE transport implementation
head -150 libs/sdk/src/transport/adapters/sse-transport.tsRepository: agentfront/frontmcp
Length of output: 4450
🏁 Script executed:
# Look for how SSE transport is created in the test client
rg -n "case 'sse'|SSETransport" libs/testing/src/client/mcp-test-client.ts -A 5Repository: agentfront/frontmcp
Length of output: 305
🏁 Script executed:
# Create a simple test to see how fetch handles query params after path
python3 << 'EOF'
from urllib.parse import urlparse
# Test URL construction scenarios
test_urls = [
"http://localhost:3000?mode=skills_only/", # Current malformed URL
"http://localhost:3000/?mode=skills_only", # Correct URL
"http://localhost:3000/", # Base URL
]
for url in test_urls:
parsed = urlparse(url)
print(f"URL: {url}")
print(f" scheme: {parsed.scheme}")
print(f" netloc: {parsed.netloc}")
print(f" path: {parsed.path}")
print(f" query: {parsed.query}")
print()
EOFRepository: agentfront/frontmcp
Length of output: 149
🏁 Script executed:
# Let me trace through the actual URL construction step by step
cat -n libs/testing/src/client/mcp-test-client.ts | sed -n '873,890p'Repository: agentfront/frontmcp
Length of output: 941
🏁 Script executed:
# Check what the actual request URL looks like in the transport when it's sent
cat -n libs/testing/src/transport/streamable-http.transport.ts | sed -n '158,165p'Repository: agentfront/frontmcp
Length of output: 408
Move query param injection to after path assembly.
Query parameters are being appended to baseUrl before StreamableHttpTransport constructs the request URL with ${baseUrl}/, creating malformed URLs like http://localhost:3000?mode=skills_only/. Query params must come after the path separator. Instead of modifying baseUrl in createTransport(), pass baseUrl without query params to the transport and append query params during the request URL construction in StreamableHttpTransport.request() at the point where the "/" is concatenated.
🤖 Prompt for AI Agents
In `@libs/testing/src/client/mcp-test-client.ts` around lines 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.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.