feat: support dynamic loading remote tools#277
Open
frontegg-david wants to merge 27 commits intomainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a complete ESM dynamic-loading subsystem: package specifier parsing, registry auth, semver utilities, caching (memory+disk), module fetching/import, manifest normalization, version polling/hot-reload, ESM-backed app/tool/resource/prompt instances and decorators, CLI support, E2E/browser demo, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser/CLI
participant App as AppEsmInstance
participant VR as VersionResolver
participant ML as EsmModuleLoader
participant Cache as EsmCacheManager
participant CDN as Registry / esm.sh
participant VP as VersionPoller
Client->>App: request load(specifier)
App->>VR: resolveVersion(specifier)
VR->>CDN: GET /{package} (registry metadata)
CDN-->>VR: {versions, dist-tags}
VR-->>App: resolvedVersion
App->>ML: load(specifier@resolvedVersion)
ML->>Cache: get(package, version)
alt cache hit
Cache-->>ML: entry
ML-->>App: EsmLoadResult (source: cache)
else cache miss
ML->>CDN: GET /{package}@{version}?bundle
CDN-->>ML: JS bundle (CJS/ESM)
ML->>Cache: put(bundle, meta)
ML-->>App: EsmLoadResult (source: network)
end
App->>App: normalizeEsmExport(manifest)
App->>App: registerFromManifest -> register tools/resources/prompts
alt autoUpdate enabled
App->>VP: addPackage(specifier,currentVersion)
VP->>VR: periodic resolve
VR-->>VP: newVersion?
alt new version detected
VP->>App: onNewVersion(old,new)
App->>ML: load(newVersion) -> replace registrations
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
frontegg-david
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/cli/src/commands/package/types.ts (1)
47-54:⚠️ Potential issue | 🟡 MinorCheck
esm:before git heuristics to prevent source misclassification.Line 47 runs git detection before Line 52. Inputs like
esm:https://esm.sh/pkg.gitwill be classified asgitinstead ofesm.💡 Proposed fix
- if (source.startsWith('github:') || source.startsWith('git+') || source.endsWith('.git')) { - return { type: 'git', ref: source }; - } - // ESM sources: explicit esm.sh URL or esm: prefix if (source.startsWith('https://esm.sh/') || source.startsWith('esm:')) { return { type: 'esm', ref: source.replace(/^esm:/, '') }; } + + if (source.startsWith('github:') || source.startsWith('git+') || source.endsWith('.git')) { + return { type: 'git', ref: source }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/package/types.ts` around lines 47 - 54, The git-detection branch (checks using source.startsWith('github:'), source.startsWith('git+'), source.endsWith('.git')) runs before the ESM detection and causes inputs like "esm:https://esm.sh/pkg.git" to be misclassified; reorder the checks so the ESM detection (source.startsWith('https://esm.sh/') || source.startsWith('esm:') with ref = source.replace(/^esm:/, '')) appears before the git heuristics (the startsWith/endsWith checks) in the parsing routine that reads the source variable, or alternatively add an explicit guard to the git branch to skip when source startsWith('esm:') or 'https://esm.sh/'. Ensure the same return shapes ({ type: 'esm', ref: ... } and { type: 'git', ref: ... }) remain unchanged.libs/sdk/src/skill/skill.utils.ts (1)
98-101:⚠️ Potential issue | 🟠 Major
isSkillRecorddoesn't recognizeSkillKind.ESM.The
validKindsarray only includesCLASS_TOKEN,VALUE, andFILE, but the PR introducesSkillKind.ESM. This could cause ESM skill records to be incorrectly rejected byisSkillRecord().🐛 Proposed fix to include ESM in valid kinds
// Validate kind is one of the allowed values - const validKinds = [SkillKind.CLASS_TOKEN, SkillKind.VALUE, SkillKind.FILE]; + const validKinds = [SkillKind.CLASS_TOKEN, SkillKind.VALUE, SkillKind.FILE, SkillKind.ESM]; if (!validKinds.includes(record['kind'] as SkillKind)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/skill/skill.utils.ts` around lines 98 - 101, The isSkillRecord function currently validates kinds against the validKinds array but omits the new SkillKind.ESM, causing ESM records to be rejected; update the validKinds declaration in skill.utils.ts (the array used by isSkillRecord) to include SkillKind.ESM so record['kind'] as SkillKind will pass validation for ESM skills.
🟡 Minor comments (16)
apps/e2e/demo-e2e-esm/browser-app/main.ts-35-37 (1)
35-37:⚠️ Potential issue | 🟡 MinorGuard the
#applookup before the error path reuses it.If
document.getElementById('app')returnsnull, the first write throws inside thetry, and Line 104 dereferences the samenullagain while handling the failure. That hides the original error and breaks the fallback UI.As per coding guidelines, "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found, rather than using non-null assertion operator (!)".🛠️ Proposed fix
async function main(): Promise<void> { - const app = document.getElementById('app')!; + const app = document.getElementById('app'); try { + if (!app) { + throw new Error('Missing `#app` root element'); + } app.textContent = 'Connecting to FrontMCP...'; @@ } catch (err) { const results: EsmTestResults = { success: false, error: (err as Error).message + '\n' + (err as Error).stack, }; window.__ESM_RESULTS__ = results; - app.textContent = 'Error: ' + (err as Error).message; + if (app) { + app.textContent = 'Error: ' + (err as Error).message; + } console.error('FrontMCP browser ESM error:', err); } }Also applies to: 98-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/browser-app/main.ts` around lines 35 - 37, The lookup of the DOM node using document.getElementById('app') in main currently uses a non-null assertion and can be nullified again in the error path; change this to guard the result: store const app = document.getElementById('app'); if (!app) throw a descriptive Error (e.g. new Error('App container not found')) instead of using !, and update any downstream error-handling/fallback code that references app (the failure UI block that currently dereferences app) to first check for the presence of app before touching it so the original error isn’t masked and the fallback UI can render safely.apps/e2e/demo-e2e-esm/browser-app/main.ts-21-23 (1)
21-23:⚠️ Potential issue | 🟡 MinorUse concrete MCP result types in
EsmTestResults.
echoResult,addResult, andgreetResultall come fromclient.callTool(). Keeping them asunknowndisables compile-time validation of the protocol payloads this E2E is supposed to exercise.As per coding guidelines, "MCP Response Types should use strictly typed MCP protocol types (e.g.,
Promise<GetPromptResult>,Promise<ReadResourceResult>), never useunknownfor MCP protocol types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/browser-app/main.ts` around lines 21 - 23, EsmTestResults currently types echoResult, addResult, and greetResult as unknown which disables compile-time validation; update the EsmTestResults interface so each field uses the concrete MCP response type returned by client.callTool() (not unknown) — import the correct MCP protocol types used by those tools and replace echoResult?: unknown, addResult?: unknown, greetResult?: unknown with the corresponding types (for example echoResult?: Promise<YourEchoResultType> or EchoResultType, addResult?: Promise<YourAddResultType>, greetResult?: Promise<YourGreetResultType>) that match the tool response shapes returned by client.callTool().libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts-52-56 (1)
52-56:⚠️ Potential issue | 🟡 MinorMake the
rmmock directory-aware.
startsWith(p)also matches siblings like<dir>-other/..., so a cache entry whose hashed directory shares a prefix can be deleted by mistake. That can hide invalidate/cleanup bugs.🛠️ Proposed fix
rm: jest.fn(async (p: string) => { + const prefix = p.endsWith(path.sep) ? p : `${p}${path.sep}`; for (const key of [...store.keys()]) { - if (key.startsWith(p)) store.delete(key); + if (key === p || key.startsWith(prefix)) { + store.delete(key); + } } }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts` around lines 52 - 56, The rm mock uses startsWith(p) which incorrectly deletes sibling directories that share a prefix; update the mock inside the jest.fn for rm to only delete keys that are the exact path or are nested under the directory (e.g., key === p or key.startsWith(p + '/') or key.startsWith(p + path.sep) after normalizing paths) so siblings like "<dir>-other/..." aren’t removed; locate the rm mock in the test (the jest.fn assigned to rm that iterates store.keys()) and replace the startsWith(p) check with a directory-aware check that accounts for separators and exact match.apps/e2e/demo-e2e-esm/src/main.ts-3-4 (1)
3-4:⚠️ Potential issue | 🟡 MinorValidate
PORTandESM_SERVER_PORTbefore using them.
parseInt()returnsNaNfor empty or non-numeric env values, which produceshttp://127.0.0.1:NaNand an invalid listen port. Failing fast here makes broken E2E runs much easier to diagnose.🛠️ Proposed fix
-const port = parseInt(process.env['PORT'] ?? '3115', 10); -const esmServerUrl = `http://127.0.0.1:${parseInt(process.env['ESM_SERVER_PORT'] ?? '50410', 10)}`; +function parsePort(name: 'PORT' | 'ESM_SERVER_PORT', fallback: number): number { + const value = process.env[name]; + const parsed = Number.parseInt(value ?? String(fallback), 10); + if (!Number.isInteger(parsed) || parsed <= 0 || parsed > 65535) { + throw new Error(`Invalid ${name}: ${value ?? '<empty>'}`); + } + return parsed; +} + +const port = parsePort('PORT', 3115); +const esmServerPort = parsePort('ESM_SERVER_PORT', 50410); +const esmServerUrl = `http://127.0.0.1:${esmServerPort}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/main.ts` around lines 3 - 4, The code uses parseInt on process.env['PORT'] and process.env['ESM_SERVER_PORT'] without validating results, which can yield NaN and produce invalid ports/URLs; update the initialization of port and esmServerUrl (referencing port, esmServerUrl, and the parseInt calls) to validate that parsed values are finite integers in the valid port range, and throw a clear error (or process.exit) if validation fails so the process fails fast with a descriptive message referencing the offending env var (PORT or ESM_SERVER_PORT).libs/sdk/src/esm-loader/__tests__/helpers/local-esm-server.ts-175-177 (1)
175-177:⚠️ Potential issue | 🟡 MinorDerive fallback
latestsemantically, not by insertion order.When
dist-tagsis omitted, Line 177 picks the last inserted key, not the highest version.VersionResolverresolves@latestfrom this field, so packages built with unsortedversionsobjects can resolve to the wrong release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/helpers/local-esm-server.ts` around lines 175 - 177, The fallback for 'dist-tags' in registryData uses Object.keys(pkg.versions).pop() which picks the last insertion rather than the highest semver; update the logic that sets registryData['dist-tags'] to compute the semantic "latest" by parsing Object.keys(pkg.versions) and selecting the max version via a semver comparison (or using the existing semver utility if available) so VersionResolver will resolve `@latest` correctly for unsorted pkg.versions.libs/sdk/src/esm-loader/__tests__/version-poller.spec.ts-115-120 (1)
115-120:⚠️ Potential issue | 🟡 MinorThis test does not verify the state change in its title.
It never drives
poll()/onNewVersion()or checks a follow-up comparison, so it would still pass ifupdateCurrentVersion()were a no-op.Suggested assertion upgrade
- it('should update current version after successful callback', () => { + it('should use the updated current version on the next check', async () => { + mockResolve.mockResolvedValue({ + resolvedVersion: '1.1.0', + availableVersions: ['1.0.0', '1.1.0'], + }); + poller.addPackage(specifier, '1.0.0'); poller.updateCurrentVersion('@acme/tools', '1.1.0'); - // Internal state updated - no public accessor for current version per package - // but the next check should compare against 1.1.0 - expect(poller.trackedCount).toBe(1); + + const [result] = await poller.checkNow(); + expect(result.currentVersion).toBe('1.1.0'); + expect(result.hasUpdate).toBe(false); });As per coding guidelines, "Maintain 95%+ test coverage across all metrics (statements, branches, functions, lines) in Jest tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/version-poller.spec.ts` around lines 115 - 120, The test title claims it verifies that updateCurrentVersion changes internal state but never drives a follow-up comparison; modify the test to assert behavior by attaching a spy/callback to poller.onNewVersion (or using the poller's version-check path) and calling poller.poll() after poller.updateCurrentVersion('@acme/tools','1.1.0'); specifically, use poller.addPackage(specifier,'1.0.0'), updateCurrentVersion to '1.1.0', then call poller.poll() and assert that the onNewVersion handler is NOT invoked for '1.1.0' (or that a later simulated remote version >1.1.0 triggers onNewVersion) to prove the current version was updated and subsequent comparisons use '1.1.0'.libs/sdk/src/common/records/agent.record.ts-18-19 (1)
18-19:⚠️ Potential issue | 🟡 MinorUpdate the inline documentation to reflect custom loader support and confirm docs are updated.
The comment at line 18 states
AgentKind.ESM = 'ESM'is "Agent loaded from an npm package via esm.sh", but the implementation supports custom package loaders with arbitrary URLs viaPackageLoader.urlandPackageLoader.registryUrl. Generalize the wording to match the actual capability.Additionally,
AgentKindis exported from the public SDK surface (libs/sdk/src/agent/index.ts), and the newAgentEsmRecordtype extends theAgentRecordunion (lines 96–115). Per coding guidelines, publishable SDK changes require matching documentation updates indocs/draft/docs/**. Please confirm these exist in the PR branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/records/agent.record.ts` around lines 18 - 19, Update the inline JSDoc for AgentKind.ESM to reflect that it represents agents loaded via ES module package loaders (including custom loaders using PackageLoader.url and PackageLoader.registryUrl) instead of only "via esm.sh"; update the comment near AgentKind.ESM and any related comment on AgentEsmRecord/AgentRecord to mention custom package loader support and that this type is part of the public SDK surface (exported via the SDK index). Then ensure the public docs for the SDK are updated to match by adding or modifying the corresponding pages under docs/draft/docs/** to document AgentKind and AgentEsmRecord behavior and confirm those doc changes are included in the PR branch.libs/sdk/src/esm-loader/__tests__/esm-record-builders.spec.ts-14-88 (1)
14-88:⚠️ Potential issue | 🟡 MinorAdd regression coverage for custom
outputSchema.Current cases validate input schema routing, but there is no assertion for
outputSchemapassthrough fromEsmToolDefinitionto metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-record-builders.spec.ts` around lines 14 - 88, Test suite missing a regression case for propagating a custom outputSchema from the EsmToolDefinition into the built record metadata; add a unit test that creates an EsmToolDefinition with a non-empty outputSchema, calls buildEsmToolRecord(tool), and asserts that record.metadata.outputSchema equals the provided schema (and optionally that record.metadata.outputSchema is not undefined), referencing buildEsmToolRecord, EsmToolDefinition, and metadata.outputSchema to locate where to add the test in the existing esm-record-builders.spec.ts file.libs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.ts-111-124 (1)
111-124:⚠️ Potential issue | 🟡 MinorRedundant mock setup - second mockFetch.mockResolvedValue overwrites first.
Lines 111-116 set up
mockFetch.mockResolvedValue, but lines 119-124 immediately overwrite it. The first setup is dead code.🧹 Suggested fix
const bundleContent = 'export default { name: "test", version: "1.0.0" }'; - mockFetch.mockResolvedValue({ - ok: true, - status: 200, - text: async () => bundleContent, - headers: new Map([['etag', '"abc"']]), - }); // Mock headers.get mockFetch.mockResolvedValue({ ok: true, status: 200, text: async () => bundleContent, headers: { get: (key: string) => (key === 'etag' ? '"abc"' : null) }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.ts` around lines 111 - 124, The test currently calls mockFetch.mockResolvedValue twice, overwriting the first response setup (the one using headers: new Map([...])) with the second (headers: { get: ... }), so remove the redundant first mockResolvedValue and keep a single mockFetch.mockResolvedValue that returns the intended shape (use the headers object with get or the Map consistently) or merge them into one setup returning { ok: true, status: 200, text: async () => bundleContent, headers: { get: (key: string) => (key === 'etag' ? '"abc"' : null) } } so mockFetch.mockResolvedValue is only invoked once.libs/sdk/src/app/instances/esm-normalize.utils.ts-24-40 (1)
24-40:⚠️ Potential issue | 🟡 MinorInconsistent metadata value checks across decorated class detectors.
isDecoratedToolClassuses strict equality=== true, whileisDecoratedResourceClassandisDecoratedPromptClassuse truthy coercion!!. This could lead to different behavior if metadata values vary (e.g., a non-boolean truthy value).♻️ Proposed fix for consistency
export function isDecoratedResourceClass(raw: unknown): boolean { - return isClass(raw) && !!getMetadata(FrontMcpResourceTokens.type, raw); + return isClass(raw) && getMetadata(FrontMcpResourceTokens.type, raw) === true; } export function isDecoratedPromptClass(raw: unknown): boolean { - return isClass(raw) && !!getMetadata(FrontMcpPromptTokens.type, raw); + return isClass(raw) && getMetadata(FrontMcpPromptTokens.type, raw) === true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/esm-normalize.utils.ts` around lines 24 - 40, The three detectors are inconsistent: isDecoratedToolClass uses strict === true while isDecoratedResourceClass and isDecoratedPromptClass use truthy coercion (!!), which can cause divergent behavior for non-boolean metadata; make them consistent by using the same check (prefer strict equality === true) for getMetadata(FrontMcpToolTokens.type, ...), getMetadata(FrontMcpResourceTokens.type, ...), and getMetadata(FrontMcpPromptTokens.type, ...) so all three functions (isDecoratedToolClass, isDecoratedResourceClass, isDecoratedPromptClass) perform the identical === true comparison.libs/sdk/src/esm-loader/factories/esm-context-factories.ts-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorUnsafe cast of
inputtoRecord<string, unknown>without runtime validation.The
executemethod acceptsunknownbut immediately casts toRecord<string, unknown>without validating the input is actually an object. If a caller passes a primitive or null, the cast silently succeeds but the handler may fail unexpectedly.Consider adding a guard:
🛡️ Proposed defensive check
const cls = class DynamicEsmToolContext extends ToolContext<ToolInputType, ToolOutputType, unknown, CallToolResult> { async execute(input: unknown): Promise<CallToolResult> { + if (!input || typeof input !== 'object') { + throw new Error(`Tool input must be an object, received ${typeof input}`); + } return executeFn(input as Record<string, unknown>); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/factories/esm-context-factories.ts` around lines 44 - 48, The execute method in the DynamicEsmToolContext class currently casts the unknown input to Record<string, unknown> unsafely; add a runtime guard at the start of the execute method to verify input is a non-null object (e.g., typeof input === "object" && input !== null) and, if desired, that it is not an Array, then either throw a clear TypeError or return a failed CallToolResult when validation fails before calling executeFn(input as Record<string, unknown>); update any tests or callers if the method now throws/returns on invalid input.libs/sdk/src/app/instances/esm-normalize.utils.ts-103-117 (1)
103-117:⚠️ Potential issue | 🟡 MinorMissing validation for
argumentsarray structure.The
argumentsfield is cast directly toEsmPromptDefinition['arguments'](which expectsArray<{ name: string; ... }>) without validating that each element has the requirednameproperty. Malformed exports could cause downstream failures.🛡️ Proposed validation
if (typeof obj['execute'] === 'function' && typeof obj['name'] === 'string') { + const rawArgs = obj['arguments']; + const validatedArgs = Array.isArray(rawArgs) + ? rawArgs.filter((arg): arg is { name: string; description?: string; required?: boolean } => + arg && typeof arg === 'object' && typeof arg.name === 'string' + ) + : undefined; return { name: obj['name'] as string, description: obj['description'] as string | undefined, - arguments: obj['arguments'] as EsmPromptDefinition['arguments'], + arguments: validatedArgs, execute: obj['execute'] as EsmPromptDefinition['execute'], }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/esm-normalize.utils.ts` around lines 103 - 117, normalizePromptFromEsmExport currently casts obj['arguments'] directly to EsmPromptDefinition['arguments'] without validating structure; update this function to validate that obj['arguments'] is an array and that every element is an object with a string name (and any other required props) before assigning it to the returned arguments field; if validation fails, omit or sanitize the arguments (e.g., filter out invalid entries) so downstream consumers of EsmPromptDefinition won't receive malformed items. Reference normalizePromptFromEsmExport and EsmPromptDefinition when making the check and handling invalid entries.libs/sdk/src/app/instances/app.esm.instance.ts-432-452 (1)
432-452:⚠️ Potential issue | 🟡 MinorPotential race condition during version update.
If
handleVersionUpdateis called concurrently (e.g., rapid polling triggers), the sequence ofreplaceAll([])followed byregisterFromManifestcould interleave, causing inconsistent registry state. Consider adding a mutex or debouncing.🔒 Proposed fix with update lock
+ private updateInProgress = false; private async handleVersionUpdate(_packageName: string, oldVersion: string, newVersion: string): Promise<void> { const logger = this.scopeProviders.getActiveScope().logger; + + if (this.updateInProgress) { + logger.warn(`Update already in progress for ${this.id}, skipping ${newVersion}`); + return; + } + this.updateInProgress = true; + logger.info(`Updating ESM app ${this.id}: ${oldVersion} → ${newVersion}`); try { // ... existing logic } catch (error) { logger.error(`Failed to update ESM app ${this.id} to ${newVersion}: ${(error as Error).message}`); + } finally { + this.updateInProgress = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 432 - 452, handleVersionUpdate can run concurrently and cause interleaved calls to _tools.replaceAll, _resources.replaceAll, _prompts.replaceAll and registerFromManifest leading to inconsistent state; wrap the entire update sequence in a single async lock (mutex) or debounce so only one execution runs at a time: add a private lock field (e.g., updateLock or an AsyncMutex) on the class, acquire it at the start of handleVersionUpdate, perform loader.load, the three replaceAll calls and registerFromManifest while holding the lock, then release the lock in a finally block to ensure other invocations wait and state remains consistent.libs/sdk/src/esm-loader/esm-manifest.ts-167-185 (1)
167-185:⚠️ Potential issue | 🟡 MinorHardcoded fallback version '0.0.0' may mask configuration issues.
When extracting from a decorated class, if the version is missing from metadata, it defaults to '0.0.0'. This could cause version resolution issues downstream (e.g., cache keys, version comparison). Consider throwing an error or using a more explicit fallback.
💡 Consider logging a warning
function extractFromDecoratedClass(cls: unknown): FrontMcpPackageManifest { const config = Reflect.getMetadata?.('__frontmcp:config', cls as object) as Record<string, unknown> | undefined; if (!config) { throw new Error('Decorated class does not have FrontMcp configuration metadata'); } + const name = ((config['info'] as Record<string, unknown>)?.['name'] as string) ?? 'unknown'; + const version = ((config['info'] as Record<string, unknown>)?.['version'] as string); + if (!version) { + console.warn(`Decorated class "${name}" is missing version in metadata, defaulting to 0.0.0`); + } + return { - name: ((config['info'] as Record<string, unknown>)?.['name'] as string) ?? 'unknown', - version: '0.0.0', + name, + version: version ?? '0.0.0', // ... rest }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-manifest.ts` around lines 167 - 185, The function extractFromDecoratedClass currently hardcodes version: '0.0.0' which can mask missing metadata; change it to read the version from the metadata (config['info']['version'] as string) and validate it is present and a non-empty string, and if not, throw a clear Error indicating the decorated class is missing a FrontMcp version in '__frontmcp:config'; update the return to use that validated version value instead of '0.0.0' (adjust casting/guards around config and info to locate the version).libs/sdk/src/esm-loader/esm-manifest.ts-206-215 (1)
206-215:⚠️ Potential issue | 🟡 MinorUse
error.issuesfor better error formatting in Zod v4.In Zod v4,
error.messageis a JSON-stringified representation of all validation issues and is not ideal for user-facing error messages. Accesserror.issuesdirectly to format cleaner, more structured error output. Consider using Zod's built-in error formatting utilities or constructing a message from individual issue messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-manifest.ts` around lines 206 - 215, The validateManifest function throws a generic error using result.error.message; update it to format and include result.error.issues from the ZodError for clearer, structured validation output (or use Zod's formatting helpers like error.flatten() / error.format()) so that FrontMcpPackageManifest validation failures produced by frontMcpPackageManifestSchema provide readable, per-issue details rather than the JSON-stringified message.libs/sdk/src/esm-loader/esm-module-loader.ts-210-220 (1)
210-220:⚠️ Potential issue | 🟡 MinorSecurity consideration: Function constructor executes arbitrary code from external sources.
The
importBundlemethod usesnew Function()to evaluate bundle content fetched from esm.sh. This executes arbitrary JavaScript from an external CDN, which carries security risk if the package is compromised.Recommendations:
- Document the trust assumptions: esm.sh is a semi-trusted CDN, but package integrity depends on the upstream npm package itself
- For production deployments, consider using import maps with the
integritymetadata field (the approach esm.sh recommends for integrity verification) by pinning exact versions and their hashes- Add CSP directives for browser deployments to restrict script evaluation scope if applicable
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-module-loader.ts` around lines 210 - 220, The importBundle method uses new Function to execute bundleContent, which runs arbitrary external code; update the code and docs to (1) add a clear comment/docstring around importBundle stating the trust assumptions (esm.sh is semi-trusted and upstream packages must be pinned), (2) implement an optional integrity check before executing bundleContent by computing a cryptographic hash (e.g., using Web Crypto in browsers or crypto.createHash in Node) and comparing it to a pinned/whitelisted hash or to integrity metadata supplied with the response, and (3) avoid unconditional new Function execution by providing safer alternatives: if running in Node use the vm module with a restricted context (vm.Script) or in the browser use import maps + integrity metadata or createObjectURL+dynamic import only after integrity verification; also document recommended CSP directives for browser deployments.
🧹 Nitpick comments (12)
libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts (1)
120-128: Avoid wall-clock sleeps in the TTL tests.The
1msTTL plussetTimeout(5)pattern makes these cases scheduler-dependent. Fake timers or injected timestamps will keep the assertions deterministic in CI.Also applies to: 184-207
libs/sdk/src/esm-loader/__tests__/version-resolver.spec.ts (1)
121-170: Consider more robust fetch mocking to avoid unnecessary brittleness.The current test assertions work correctly for the existing implementation, which intentionally uses lowercase
%2fvia manual string replacement and passes a string URL with a plain objectheaders. However, these tests would break if the code refactors to useURLconstructor,Requestobjects, orHeadersclass—equivalent implementations thatfetchfully supports. Making assertions more semantic (checking for the presence of headers and URL prefixes without asserting the exact object shape) would make tests more resilient to refactoring.For example, normalize headers to a
Headersobject for case-insensitive lookups, and extract URLs consistently regardless of whether they arrive as strings orURLobjects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/version-resolver.spec.ts` around lines 121 - 170, Tests in version-resolver.spec.ts are brittle because they assert fetch was called with a specific string URL and a plain headers object; update the test assertions to be resilient to Request/URL/Headers variations by normalizing the fetch call arguments: when inspecting mockFetch.mock.calls[0][0], coerce it to a string (handle URL or Request) before asserting prefixes/encoded segments, and when inspecting the options arg (mockFetch.mock.calls[0][1]) convert any Headers or Request.headers into a Headers instance (or otherwise perform case-insensitive lookups) to assert the Authorization header value; reference the VersionResolver instance and makeSpecifier used in the test to locate where to change assertions.libs/sdk/src/common/records/skill.record.ts (1)
4-4: Avoid importing ESM-loader internals intocommon/records.Line 4 couples a shared record module to
esm-loader. Consider movingParsedPackageSpecifierto a shared common type location and importing from there.As per coding guidelines, "Centralize record types in common/records directory and import from there instead of from module-specific files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/records/skill.record.ts` at line 4, The skill.record.ts file imports ParsedPackageSpecifier from the esm-loader internals, coupling a shared record to module-specific code; move the ParsedPackageSpecifier type definition into a shared common location (e.g., a new or existing common types/records file) and export it from there, then update skill.record.ts to import ParsedPackageSpecifier from that centralized common type instead of '../../esm-loader/package-specifier'; ensure you update any re-exports or barrel files so other modules use the new shared symbol and avoid circular dependencies.libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.ts (1)
3-80: Add a test for the decorated-class default export path.
normalizeEsmExport()includes the decorated-class branch, but this suite currently validates only plain manifest/named-export paths.As per coding guidelines, "
**/*.spec.{ts,tsx}: Maintain 95%+ test coverage across all metrics (statements, branches, functions, lines) in Jest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.ts` around lines 3 - 80, The tests are missing coverage for the "decorated-class default export" branch in normalizeEsmExport—add a new test that exports a class instance as the module default which the function should recognize and normalize; create a mock class (e.g., TestManifestClass) with the same manifest properties either as static fields or decorated/metadata the loader expects, set moduleExport = { default: new TestManifestClass() } (or the decorated class itself if normalizeEsmExport reads metadata from the class), call normalizeEsmExport(moduleExport) and assert expected name, version, tools/prompts, and fallbacks (e.g., unknown/0.0.0 when missing), ensuring the decorated-class code path in normalizeEsmExport is exercised.apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts (2)
17-17: Import ESM_SERVER_PORT from helpers instead of duplicating.The constant
ESM_SERVER_PORT = 50413is already exported from./helpers.ts. Duplicating this value risks divergence if the port needs to change.🔧 Suggested fix
import { test, expect } from '@playwright/test'; import { spawn, type ChildProcess } from 'node:child_process'; +import { ESM_SERVER_PORT } from './helpers'; -const ESM_SERVER_PORT = 50413; let esmServerProcess: ChildProcess | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts` at line 17, Replace the duplicated const ESM_SERVER_PORT in esm-browser.pw.spec.ts with an import from the existing helper export: remove the local declaration "ESM_SERVER_PORT = 50413" and add an import of ESM_SERVER_PORT from ./helpers (ensuring any existing imports are adjusted), then use that imported ESM_SERVER_PORT wherever referenced in this file.
54-59: Add error handling for already-killed process.The
afterAllhook callskill('SIGTERM')without checking if the process is still running. If the process exited unexpectedly, this could throw.🛡️ Suggested improvement
test.afterAll(async () => { if (esmServerProcess) { - esmServerProcess.kill('SIGTERM'); + try { + esmServerProcess.kill('SIGTERM'); + } catch { + // Process may have already exited + } esmServerProcess = null; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts` around lines 54 - 59, The afterAll hook calls esmServerProcess.kill('SIGTERM') unconditionally which can throw if the child already exited; update the test.afterAll cleanup to safely handle that by checking the ChildProcess state (e.g., esmServerProcess && !esmServerProcess.killed && esmServerProcess.exitCode === null) before calling esmServerProcess.kill('SIGTERM'), and/or wrap the kill call in a try/catch to swallow ENOENT/ESRCH errors and then set esmServerProcess = null; target the test.afterAll block and the esmServerProcess variable when making this change.apps/e2e/demo-e2e-esm/browser-app/vite.config.ts (1)
31-39: Consider documenting shimMissingExports risk.
shimMissingExports: truecombined with suppressingMISSING_EXPORTwarnings could mask legitimate issues during development. While acceptable for E2E test builds, consider adding a comment explaining this is intentional for test fixtures only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/browser-app/vite.config.ts` around lines 31 - 39, The build config sets rollupOptions.shimMissingExports = true and suppresses MISSING_EXPORT warnings in onwarn which can hide real issues; add a concise comment above rollupOptions (or next to shimMissingExports/onwarn) stating that shimMissingExports is intentionally enabled only for E2E test fixtures and that suppressing MISSING_EXPORT warnings may mask legitimate missing-export bugs in non-test builds so it should not be used in production/dev builds.apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts (2)
169-210: Consider adding request body size limit for admin publish.The
handleAdminPublishfunction accumulates the entire request body in memory without size limits. While acceptable for controlled E2E tests, a size check would prevent accidental memory exhaustion.🛡️ Optional safeguard
function handleAdminPublish(req: http.IncomingMessage, res: http.ServerResponse): void { let body = ''; + const MAX_BODY_SIZE = 1024 * 1024; // 1MB limit req.on('data', (chunk: Buffer) => { + if (body.length + chunk.length > MAX_BODY_SIZE) { + res.writeHead(413, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Request body too large' })); + req.destroy(); + return; + } body += chunk.toString(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts` around lines 169 - 210, handleAdminPublish currently accumulates the entire request body in memory; add a configurable size limit (e.g. MAX_BODY_SIZE) and enforce it inside the req.on('data') handler by tracking the accumulated byte length, and if a chunk pushes the total past the limit respond with 413 Payload Too Large, end the response, and destroy the request (and remove listeners) to stop further processing; keep the JSON parsing/response logic in the existing req.on('end') path and ensure the error path for oversized bodies is distinct from the existing JSON error handling.
156-156: Version regex may not handle all semver formats.The regex
/^(.+?)@(\d+\.\d+\.\d+.*)$/captures versions starting withX.Y.Zbut the.*at the end is greedy and may match unintended patterns. For E2E tests with controlled versions this is likely fine, but consider using a more precise semver pattern if expanding usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts` at line 156, The current regex assigned to versionMatch (const versionMatch = pathname.match(/^(.+?)@(\d+\.\d+\.\d+.*)$/);) is too permissive and the trailing .* is greedy; replace it with a stricter semver pattern or validate with a semver parser: either change the regex to something like /^(.+?)@(\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?)$/ to properly capture pre-release/build metadata, or use a semver library (e.g., semver.valid/semver.parse) on the captured part to ensure correct version parsing for pathname and keep the captured package name in the first group.libs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.ts (1)
156-158: Avoid non-null assertion; use explicit assertion instead.Line 158 uses
def!.namewhich violates the guideline to use proper error handling instead of non-null assertions. Since this is a test file and the preceding line already assertsdefis defined, consider using an explicit check or Jest's type narrowing.🔧 Suggested fix
// Plain goes through normalizeToolFromEsmExport const def = normalizeToolFromEsmExport(plain[0]); expect(def).toBeDefined(); - expect(def!.name).toBe('plain'); + expect(def?.name).toBe('plain');As per coding guidelines: "Use proper error handling instead of non-null assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.ts` around lines 156 - 158, Replace the non-null assertion on def coming from normalizeToolFromEsmExport by explicitly narrowing/guarding its value before accessing .name: after the existing expect(def).toBeDefined() add an explicit runtime guard (e.g., if (!def) throw new Error('def is undefined')) or use Jest's type-narrowing pattern so you can safely call def.name; update the assertion to use that guarded def (referencing variable def and function normalizeToolFromEsmExport).libs/sdk/src/app/instances/app.esm.instance.ts (2)
56-127: Consider sharing empty registry implementations.The
EmptyPluginRegistry,EmptyAdapterRegistry, andEmptySkillRegistryclasses are defined inline. If multiple ESM apps are instantiated, each creates new instances. These could be shared singletons or moved to a common module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 56 - 127, The three empty registry classes (EmptyPluginRegistry, EmptyAdapterRegistry, EmptySkillRegistry) are created inline causing multiple identical instances; refactor by moving their definitions to a shared module or file and expose single shared singleton instances (e.g., export const EMPTY_PLUGIN_REGISTRY = new EmptyPluginRegistry(), EMPTY_ADAPTER_REGISTRY, EMPTY_SKILL_REGISTRY) and update any code that constructs or references these classes to use the exported singletons instead of creating new instances so all ESM apps reuse the same empty registries.
180-205: Duplicated registry auth derivation logic.The same PackageLoader → EsmRegistryAuth mapping appears twice: once in the constructor (lines 186-192) and again in
initialize()for the poller (lines 242-248). Consider extracting a helper method.♻️ Extract helper method
+ private deriveRegistryAuth(loader?: PackageLoader): EsmRegistryAuth | undefined { + return loader + ? { + registryUrl: loader.registryUrl ?? loader.url, + token: loader.token, + tokenEnvVar: loader.tokenEnvVar, + } + : undefined; + } constructor(record: AppRecord, scopeProviders: ProviderRegistry) { // ... const mergedLoader = appConfig?.loader ?? scopeMetadata.loader; - const registryAuth: EsmRegistryAuth | undefined = mergedLoader - ? { - registryUrl: mergedLoader.registryUrl ?? mergedLoader.url, - token: mergedLoader.token, - tokenEnvVar: mergedLoader.tokenEnvVar, - } - : undefined; + const registryAuth = this.deriveRegistryAuth(mergedLoader); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 180 - 205, The registry auth mapping from PackageLoader to EsmRegistryAuth is duplicated; extract a small helper (e.g., buildEsmRegistryAuth or deriveRegistryAuth) that accepts the package config or mergedLoader and returns the EsmRegistryAuth | undefined, then replace the inline logic used in the constructor (where mergedLoader is built from scopeProviders.getActiveScope().metadata and this.metadata.packageConfig) and the initialize() poller block with calls to that helper; ensure the helper returns the same fields (registryUrl from registryUrl ?? url, token, tokenEnvVar) and is used when constructing EsmModuleLoader and wherever the poller needs registryAuth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b9ae677-b8bd-49de-87ec-8ffa8dfa1e73
📒 Files selected for processing (82)
apps/e2e/demo-e2e-esm/browser-app/index.htmlapps/e2e/demo-e2e-esm/browser-app/main.tsapps/e2e/demo-e2e-esm/browser-app/vite.config.tsapps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.tsapps/e2e/demo-e2e-esm/e2e/browser/helpers.tsapps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.tsapps/e2e/demo-e2e-esm/e2e/esm-hot-reload.e2e.spec.tsapps/e2e/demo-e2e-esm/e2e/esm.e2e.spec.tsapps/e2e/demo-e2e-esm/jest.e2e.config.tsapps/e2e/demo-e2e-esm/jest.perf.config.tsapps/e2e/demo-e2e-esm/playwright.config.tsapps/e2e/demo-e2e-esm/project.jsonapps/e2e/demo-e2e-esm/src/esm-package-server/main.tsapps/e2e/demo-e2e-esm/src/main-hot-reload.tsapps/e2e/demo-e2e-esm/src/main.tsapps/e2e/demo-e2e-esm/tsconfig.app.jsonapps/e2e/demo-e2e-esm/tsconfig.jsonapps/e2e/demo-e2e-esm/webpack.config.jslibs/cli/src/commands/package/esm-update.tslibs/cli/src/commands/package/types.tslibs/cli/src/commands/pm/paths.tslibs/sdk/package.jsonlibs/sdk/src/app/app.registry.tslibs/sdk/src/app/instances/app.esm.instance.tslibs/sdk/src/app/instances/esm-normalize.utils.tslibs/sdk/src/app/instances/index.tslibs/sdk/src/common/interfaces/job.interface.tslibs/sdk/src/common/interfaces/prompt.interface.tslibs/sdk/src/common/interfaces/resource.interface.tslibs/sdk/src/common/interfaces/skill.interface.tslibs/sdk/src/common/interfaces/tool.interface.tslibs/sdk/src/common/interfaces/workflow.interface.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/records/agent.record.tslibs/sdk/src/common/records/job.record.tslibs/sdk/src/common/records/prompt.record.tslibs/sdk/src/common/records/resource.record.tslibs/sdk/src/common/records/skill.record.tslibs/sdk/src/common/records/tool.record.tslibs/sdk/src/common/records/workflow.record.tslibs/sdk/src/common/tokens/app.tokens.tslibs/sdk/src/errors/__tests__/esm.errors.spec.tslibs/sdk/src/errors/esm.errors.tslibs/sdk/src/errors/index.tslibs/sdk/src/esm-loader/__tests__/esm-auth.types.spec.tslibs/sdk/src/esm-loader/__tests__/esm-cache.spec.tslibs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.tslibs/sdk/src/esm-loader/__tests__/esm-context-factories.spec.tslibs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tslibs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.tslibs/sdk/src/esm-loader/__tests__/esm-normalize.utils.spec.tslibs/sdk/src/esm-loader/__tests__/esm-record-builders.spec.tslibs/sdk/src/esm-loader/__tests__/helpers/esm-fixtures.tslibs/sdk/src/esm-loader/__tests__/helpers/local-esm-server.tslibs/sdk/src/esm-loader/__tests__/load-from.spec.tslibs/sdk/src/esm-loader/__tests__/package-specifier.spec.tslibs/sdk/src/esm-loader/__tests__/semver.utils.spec.tslibs/sdk/src/esm-loader/__tests__/version-poller.spec.tslibs/sdk/src/esm-loader/__tests__/version-resolver.spec.tslibs/sdk/src/esm-loader/esm-auth.types.tslibs/sdk/src/esm-loader/esm-cache.tslibs/sdk/src/esm-loader/esm-manifest.tslibs/sdk/src/esm-loader/esm-module-loader.tslibs/sdk/src/esm-loader/factories/esm-context-factories.tslibs/sdk/src/esm-loader/factories/esm-instance-factories.tslibs/sdk/src/esm-loader/factories/esm-record-builders.tslibs/sdk/src/esm-loader/factories/index.tslibs/sdk/src/esm-loader/index.tslibs/sdk/src/esm-loader/load-from.tslibs/sdk/src/esm-loader/package-specifier.tslibs/sdk/src/esm-loader/semver.utils.tslibs/sdk/src/esm-loader/version-poller.tslibs/sdk/src/esm-loader/version-resolver.tslibs/sdk/src/index.tslibs/sdk/src/job/job.utils.tslibs/sdk/src/prompt/prompt.utils.tslibs/sdk/src/resource/resource.utils.tslibs/sdk/src/skill/skill.utils.tslibs/sdk/src/tool/tool.utils.ts
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-16T11:19:07.211Z |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
libs/sdk/src/esm-loader/factories/esm-record-builders.ts (2)
33-38:⚠️ Potential issue | 🟠 MajorHonor
tool.outputSchemain the generated metadata.
EsmToolDefinitionexposesoutputSchema, but Line 106 always writes'json', so caller-provided output metadata is dropped.🔧 Proposed fix
export interface EsmToolDefinition { name: string; description?: string; inputSchema?: Record<string, unknown>; - outputSchema?: unknown; + outputSchema?: ToolMetadata['outputSchema']; execute: EsmToolExecuteHandler; } @@ - outputSchema: 'json' as ToolMetadata['outputSchema'], + outputSchema: tool.outputSchema ?? ('json' as ToolMetadata['outputSchema']),Also applies to: 100-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/factories/esm-record-builders.ts` around lines 33 - 38, The generated metadata currently hardcodes the output schema to 'json' and ignores EsmToolDefinition.outputSchema; update the metadata builder that inspects the tool object (the code writing 'json' for output) to use tool.outputSchema when present and only fall back to 'json' when it's undefined, ensuring any caller-provided outputSchema is preserved in the emitted metadata.
44-49:⚠️ Potential issue | 🟠 MajorValidate
resource.uribefore creatingResourceMetadata.Line 135 persists any string, so schemeless or malformed URIs can enter resource metadata.
As per coding guidelines, "Validate URIs per RFC 3986 at metadata level using schema validation with message 'URI must have a valid scheme (e.g., file://, https://, custom://)'".🔧 Proposed fix
function isZodShape(schema: Record<string, unknown>): boolean { return Object.values(schema).some((v) => v instanceof z.ZodType); } + +const uriWithSchemeSchema = z.string().refine((value) => /^[A-Za-z][A-Za-z\d+.-]*:/.test(value), { + message: 'URI must have a valid scheme (e.g., file://, https://, custom://)', +}); @@ const metadata: ResourceMetadata = { name: resourceName, description: resource.description ?? `ESM resource: ${resource.name}`, - uri: resource.uri, + uri: uriWithSchemeSchema.parse(resource.uri), mimeType: resource.mimeType, };Also applies to: 132-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/factories/esm-record-builders.ts` around lines 44 - 49, Validate the EsmResourceDefinition.uri before constructing ResourceMetadata: inside the builder that consumes EsmResourceDefinition (e.g., the function that creates ResourceMetadata from an EsmResourceDefinition around the ResourceMetadata creation at lines ~132-137), add schema validation per RFC 3986 that ensures the URI includes a valid scheme (examples: file://, https://, custom://) and reject or throw a clear validation error with the message "URI must have a valid scheme (e.g., file://, https://, custom://)"; perform this check for all code paths that create ResourceMetadata from EsmResourceDefinition.name/uri to prevent schemeless or malformed URIs being persisted.libs/cli/src/commands/package/esm-update.ts (1)
75-87:⚠️ Potential issue | 🟠 MajorLoad the exact version you just resolved.
loader.load(specifier)re-resolves the range, so a publish between Lines 75 and 87 can install a different version than the one printed on Line 82.🛠️ Proposed fix
- const result = await loader.resolveVersion(specifier); + const resolvedVersion = await loader.resolveVersion(specifier); - if (result === currentVersion) { + if (resolvedVersion === currentVersion) { console.log(` ${name}: up to date (${currentVersion})`); continue; } - console.log(` ${name}: ${currentVersion} → ${result}`); + console.log(` ${name}: ${currentVersion} → ${resolvedVersion}`); @@ - const loadResult = await loader.load(specifier); + const loadResult = await loader.load( + parsePackageSpecifier(`${specifier.fullName}@${resolvedVersion}`), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/package/esm-update.ts` around lines 75 - 87, The code calls loader.load(specifier) which re-resolves the range and can install a different version than the one returned by loader.resolveVersion(result); instead, load the exact resolved version: build an exact specifier using the package name/range and the resolved version (e.g., `${specifier}@${result}` or replace the range portion of specifier with result) and pass that to loader.loadExact or loader.load (the exact-specifier string) so the version printed from loader.resolveVersion(result) is the one actually installed; update the call site where loadResult is assigned to use the exact resolved version (use symbols loader.resolveVersion, result, specifier, and loader.load/loadExact).libs/sdk/src/esm-loader/esm-cache.ts (1)
68-96:⚠️ Potential issue | 🔴 Critical
require()/node:fsstill break the native-ESM cache path.This module sits on the ESM loader surface, but these branches still use CommonJS
require()andnode:fs/promises. In an emitted ESM build,requireis undefined, soDEFAULT_CACHE_DIRfalls back to''and the disk-cache methods either no-op or throw when they hitreadBundle()/getEntryDir().Verify the runtime assumption against Node's ESM rules:
In native Node.js ES modules, is CommonJS `require()` available without `createRequire`? What happens if an ES module calls `require('node:path')` or `require('@frontmcp/utils')`?As per coding guidelines, "Use
@frontmcp/utilsfor all file system operations. Do NOT usefs/promisesornode:fsdirectly. Import utilities likereadFile,writeFile,mkdir,readdir,stat, etc. from@frontmcp/utils".Also applies to: 145-148, 192-194, 247-250, 297-300, 339-340, 347-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-cache.ts` around lines 68 - 96, The code that computes DEFAULT_CACHE_DIR (and related branches used by readBundle/getEntryDir) uses CommonJS require('node:path'), require('node:fs') and require('node:os') which breaks under native ESM; replace those require() calls with ESM-safe APIs: use dynamic import() for node built-ins (e.g. await import('node:path') / await import('node:os')) or top-level ESM imports, and for filesystem checks use the project-wide `@frontmcp/utils` fs helpers (existence/stat/read/write/mkdir) instead of node:fs or fs/promises; update the cache-path logic (the nodeModulesDir existence check and homedir fallback) to call the imported async helpers so DEFAULT_CACHE_DIR is correctly computed at runtime in ESM environments and ensure readBundle() / getEntryDir() consume the normalized path value.
🧹 Nitpick comments (8)
apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts (1)
66-146: Consider extracting repetitive setup pattern.The
goto+waitForFunction+evaluatepattern is repeated in all 6 tests. A helper function could reduce duplication while maintaining test clarity.♻️ Optional: Extract helper for test setup
// Add at the top of the describe block async function loadAndGetResults(page: Page) { await page.goto(`/?esmServer=http://127.0.0.1:${ESM_SERVER_PORT}`); await page.waitForFunction( () => (window as unknown as { __ESM_RESULTS__?: unknown }).__ESM_RESULTS__, { timeout: 60000 } ); return page.evaluate(() => (window as unknown as { __ESM_RESULTS__: unknown }).__ESM_RESULTS__ ); } // Then in tests: test('loads and reports ESM tools successfully', async ({ page }) => { const r = await loadAndGetResults(page) as { success: boolean; toolNames: string[] }; expect(r.success).toBe(true); expect(r.toolNames).toContain('esm:echo'); // ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts` around lines 66 - 146, Repeated test setup (page.goto + waitForFunction + page.evaluate) should be extracted into a single helper function (e.g., loadAndGetResults) placed at the top of the describe block; implement loadAndGetResults(page: Page) to perform the goto using ESM_SERVER_PORT, wait for window.__ESM_RESULTS__ with the existing 60000ms timeout, and return the evaluated __ESM_RESULTS__; then replace the duplicated blocks in each test (those that currently call page.goto, page.waitForFunction, and page.evaluate) with a single call to loadAndGetResults and cast its return to the appropriate shape (e.g., { success: boolean; toolNames: string[] } or other result shapes).apps/e2e/demo-e2e-esm/browser-app/main.ts (1)
101-111: Consider safer error handling for non-Error thrown values.The catch block assumes
erris always anErrorinstance. While SDK errors should beErrorinstances, defensive handling would be more robust.🛡️ Optional: Safer error extraction
} catch (err) { + const error = err instanceof Error ? err : new Error(String(err)); const results: EsmTestResults = { success: false, - error: (err as Error).message + '\n' + (err as Error).stack, + error: error.message + '\n' + (error.stack ?? ''), }; window.__ESM_RESULTS__ = results; if (app) { - app.textContent = 'Error: ' + (err as Error).message; + app.textContent = 'Error: ' + error.message; } console.error('FrontMCP browser ESM error:', err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/browser-app/main.ts` around lines 101 - 111, The catch block in main.ts assumes the thrown value is an Error; change it to defensively extract message and stack (use e.g. a check like `err instanceof Error` or typeof checks) and populate EsmTestResults.error with a safe string (fallback to String(err) and include stack only if present), update window.__ESM_RESULTS__ and app.textContent to use the extracted message, and keep console.error(err) unchanged so non-Error throws are handled without runtime exceptions; update references around EsmTestResults, window.__ESM_RESULTS__, and the app textContent assignment.libs/sdk/src/app/instances/esm-normalize.utils.ts (2)
55-70: Consider validating optional field types before assignment.The function casts optional fields (
description,inputSchema,outputSchema) without type validation. If the ESM module exports invalid types (e.g.,description: 123), the cast will silently produce incorrect typed data.🛡️ Suggested type validation
if (typeof obj['execute'] === 'function' && typeof obj['name'] === 'string') { + const description = typeof obj['description'] === 'string' ? obj['description'] : undefined; + const inputSchema = obj['inputSchema'] && typeof obj['inputSchema'] === 'object' + ? obj['inputSchema'] as Record<string, unknown> + : undefined; return { name: obj['name'] as string, - description: obj['description'] as string | undefined, - inputSchema: obj['inputSchema'] as Record<string, unknown> | undefined, + description, + inputSchema, outputSchema: obj['outputSchema'], execute: obj['execute'] as EsmToolDefinition['execute'], }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/esm-normalize.utils.ts` around lines 55 - 70, The normalizeToolFromEsmExport function currently casts optional fields (description, inputSchema, outputSchema) without validating their types; update normalizeToolFromEsmExport to check each optional property before assignment: ensure description is a string, inputSchema is an object (or record) and not a function/array, and outputSchema matches expected type (e.g., object or undefined) before setting them on the returned EsmToolDefinition; leave the properties undefined if validation fails so invalid exported values are not silently cast into incorrect types.
103-117: Consider validating theargumentsarray structure.The
argumentsfield is cast toEsmPromptDefinition['arguments'](an array of{ name: string; description?: string; required?: boolean }) without validation. Malformed ESM exports could cause downstream issues when the prompt is invoked.🛡️ Suggested validation helper
+function isValidPromptArguments(args: unknown): args is EsmPromptDefinition['arguments'] { + if (!Array.isArray(args)) return false; + return args.every( + (arg) => typeof arg === 'object' && arg !== null && typeof (arg as Record<string, unknown>)['name'] === 'string', + ); +} + export function normalizePromptFromEsmExport(raw: unknown): EsmPromptDefinition | undefined { if (!raw || typeof raw !== 'object' || isClass(raw)) return undefined; const obj = raw as Record<string, unknown>; if (typeof obj['execute'] === 'function' && typeof obj['name'] === 'string') { + const args = isValidPromptArguments(obj['arguments']) ? obj['arguments'] : undefined; return { name: obj['name'] as string, description: obj['description'] as string | undefined, - arguments: obj['arguments'] as EsmPromptDefinition['arguments'], + arguments: args, execute: obj['execute'] as EsmPromptDefinition['execute'], }; } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/esm-normalize.utils.ts` around lines 103 - 117, The normalizePromptFromEsmExport function currently casts obj['arguments'] without checks; validate that obj['arguments'] is an array and that each element is an object with a string 'name' and optional string 'description' and boolean 'required' before assigning to EsmPromptDefinition.arguments; implement this by using Array.isArray(obj['arguments']), filter/map to only include items that pass typeof checks (item && typeof item === 'object' && typeof item.name === 'string' and optionally typeof item.description === 'string' and typeof item.required === 'boolean'), and assign the sanitized array (or undefined if none valid) instead of the raw cast in normalizePromptFromEsmExport.libs/sdk/src/app/instances/app.esm.instance.ts (3)
160-165: Consider removing underscore prefixes from private fields.The coding guidelines specify using the
privatekeyword without underscore prefix for private fields, exposing via getter methods instead. The current implementation uses bothprivateand underscore prefix (_tools,_resources, etc.).Since these fields are already exposed via getter methods (lines 267-281), the underscores are redundant.
✨ Suggested naming convention
- private readonly _tools: ToolRegistry; - private readonly _resources: ResourceRegistry; - private readonly _prompts: PromptRegistry; - private readonly _plugins: EmptyPluginRegistry; - private readonly _adapters: EmptyAdapterRegistry; - private readonly _skills: EmptySkillRegistry; + private readonly toolsRegistry: ToolRegistry; + private readonly resourcesRegistry: ResourceRegistry; + private readonly promptsRegistry: PromptRegistry; + private readonly pluginsRegistry: EmptyPluginRegistry; + private readonly adaptersRegistry: EmptyAdapterRegistry; + private readonly skillsRegistry: EmptySkillRegistry;As per coding guidelines: "Use
privatekeyword without underscore prefix for private fields".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 160 - 165, Private fields in AppInstance are declared with redundant underscore prefixes (_tools, _resources, _prompts, _plugins, _adapters, _skills) despite using the private keyword and exposing them via getters (tools, resources, prompts, plugins, adapters, skills); remove the underscore from each private field name and update all internal references and assignments to use the new names so the existing getter methods continue to work unchanged. Ensure constructor parameters and any methods that reference _tools/_resources/_prompts/_plugins/_adapters/_skills are updated to tools/resources/prompts/plugins/adapters/skills respectively to keep behavior identical.
336-357: Consider handling individual registration failures gracefully.If one tool fails to normalize or register (e.g., malformed metadata), the entire
registerFromManifestcall fails, potentially leaving the app in a partially registered state. Consider wrapping individual registrations in try-catch to log errors and continue with remaining items.🛡️ Suggested resilient registration
// Register tools if (manifest.tools?.length) { for (const rawTool of manifest.tools) { + try { if (isDecoratedToolClass(rawTool)) { // Real `@Tool-decorated` class → standard normalization (full DI) const record = normalizeTool(rawTool); const prefixedName = namespace ? `${namespace}:${record.metadata.name}` : record.metadata.name; record.metadata.name = prefixedName; record.metadata.id = prefixedName; const instance = new ToolInstance(record, this.scopeProviders, this.appOwner); await instance.ready; this._tools.registerToolInstance(instance); toolCount++; } else { // Plain object → existing path const toolDef = normalizeToolFromEsmExport(rawTool); if (toolDef) { const instance = createEsmToolInstance(toolDef, this.scopeProviders, this.appOwner, namespace); await instance.ready; this._tools.registerToolInstance(instance); toolCount++; } } + } catch (error) { + logger.warn(`Failed to register tool from ESM manifest: ${(error as Error).message}`); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 336 - 357, The loop that registers tools can throw and abort the whole registerFromManifest run; wrap each tool's processing (both the isDecoratedToolClass branch using normalizeTool → new ToolInstance → await instance.ready → this._tools.registerToolInstance and the plain-object branch using normalizeToolFromEsmExport → createEsmToolInstance → await instance.ready → this._tools.registerToolInstance) in a try-catch so a failure for one tool logs the error (with identifying metadata) and continues to the next tool, and only increments toolCount on successful registration.
315-317: Consider disposing registries and cache indispose().The
disposemethod only stops the version poller. IfAppEsmInstanceis created and disposed frequently, this could lead to resource accumulation since registries and cache aren't cleaned up.🧹 Suggested cleanup
async dispose(): Promise<void> { this.poller?.stop(); + // Clear registries to release references + this._tools.replaceAll([], this.appOwner); + this._resources.replaceAll([], this.appOwner); + this._prompts.replaceAll([], this.appOwner); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 315 - 317, The dispose() method in AppEsmInstance currently only stops this.poller, but should also clean up registries and cache to avoid resource leaks; update AppEsmInstance.dispose to stop the poller, iterate and dispose/close each entry in this.registries (or clear the registry map) and call clear/close on this.cache (and any nested resources), then null out these fields (this.registries = undefined; this.cache = undefined) so references are released. Ensure you call appropriate disposal methods on registry objects if they expose a dispose/close API or otherwise remove listeners and entries before clearing.libs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.ts (1)
162-176: Consider expanding Resource and Prompt detection tests.The current tests verify detection works but don't validate the normalization path for plain resources/prompts. Consider adding tests similar to the tool tests that verify
normalizeResourceFromEsmExportandnormalizePromptFromEsmExporthandle plain objects correctly.🧪 Suggested additional tests
it('normalizes plain resource export', () => { const plainResource = { name: 'status', uri: 'status://current', read: jest.fn(), }; expect(isDecoratedResourceClass(plainResource)).toBe(false); const def = normalizeResourceFromEsmExport(plainResource); expect(def).toBeDefined(); expect(def?.name).toBe('status'); expect(def?.uri).toBe('status://current'); }); it('normalizes plain prompt export', () => { const plainPrompt = { name: 'greet', execute: jest.fn(), }; expect(isDecoratedPromptClass(plainPrompt)).toBe(false); const def = normalizePromptFromEsmExport(plainPrompt); expect(def).toBeDefined(); expect(def?.name).toBe('greet'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.ts` around lines 162 - 176, Add tests that verify normalization for plain object exports by creating a plainResource object (with name, uri, and read) and asserting isDecoratedResourceClass(plainResource) is false, then calling normalizeResourceFromEsmExport(plainResource) and asserting the returned def is defined and has name === 'status' and uri === 'status://current'; similarly create a plainPrompt object (with name and execute), assert isDecoratedPromptClass(plainPrompt) is false, call normalizePromptFromEsmExport(plainPrompt) and assert the returned def is defined and has name === 'greet'. Ensure you add these new it blocks alongside the existing Resource and Prompt detection tests and use the same helper functions normalizeResourceFromEsmExport, normalizePromptFromEsmExport, isDecoratedResourceClass, and isDecoratedPromptClass for locating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts`:
- Around line 21-51: The startup promise in test.beforeAll assigns
esmServerProcess but on timeout or error it rejects without killing the spawned
process; update the handler to ensure the child is terminated: in the timeout
callback and in esmServerProcess.on('error') (and optionally in stderr 'data'
error cases) call esmServerProcess.kill() (or spawn-specific cleanup) before
rejecting, and ensure you clear the timeout/mark started appropriately; locate
this logic around the test.beforeAll block and the esmServerProcess variable to
implement the cleanup so the child cannot become a zombie when the promise
rejects.
In `@libs/cli/src/commands/package/esm-update.ts`:
- Around line 67-71: The code is passing URL-backed ESM refs (source.ref) into
parsePackageSpecifier which only handles npm-style specs; update the logic
around where specifier is derived so that when source.type === 'esm' (or the ref
looks like an HTTP(s) URL) you call parseInstallSource(sourceRef) or otherwise
skip parsePackageSpecifier, and only call parsePackageSpecifier(sourceRef) for
npm-style refs; reference parsePackageSpecifier, parseInstallSource, source.type
and source.ref when making the change so URL-backed ESM entries no longer fall
into the catch on every update.
In `@libs/sdk/src/agent/agent.instance.ts`:
- Around line 521-522: The thrown error in the AgentKind.ESM branch currently
uses AgentNotConfiguredError with a full sentence, which misformats the message;
replace this with a proper error that accepts a custom message: either throw a
generic InternalMcpError with a descriptive message like `ESM agent "<name>"
cannot be created via AgentInstance.create()` or add a new EsmAgentCreationError
class (e.g., export class EsmAgentCreationError extends InternalMcpError {
constructor(agentName: string) { super(`ESM agent "${agentName}" cannot be
created via AgentInstance.create(). Use AppEsm runtime instead.`,
'ESM_AGENT_CREATION_ERROR') } }) in libs/sdk/src/errors/agent.errors.ts and
throw new EsmAgentCreationError(this.name) inside the AgentKind.ESM case
(replace the current throw of AgentNotConfiguredError).
In `@libs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.ts`:
- Around line 216-221: The test currently uses Parameters<typeof
EsmModuleLoader>[0] to infer the constructor arg type which is incorrect for a
class; change it to ConstructorParameters<typeof EsmModuleLoader>[0] and then
access the logger property type (e.g. ConstructorParameters<typeof
EsmModuleLoader>[0]['logger']) so the logger variable is cast to the actual
constructor option shape used by EsmModuleLoader.
In `@libs/sdk/src/esm-loader/esm-module-loader.ts`:
- Around line 119-124: The code evaluates in-memory bundle text using
importBundle (which currently uses new Function and treats bundles as CommonJS)
causing SyntaxError for ESM bundles; change importBundle to create a data: or
blob: URL from the entry.bundleContent and use dynamic import() to evaluate the
text as a proper ESM module, update all call sites that pass entry.bundleContent
(the branches that call importBundle in esm-module-loader where
entry.bundleContent is present) to rely on the new importBundle behavior, and
ensure the result is still passed to normalizeEsmExport for consistent export
normalization.
---
Duplicate comments:
In `@libs/cli/src/commands/package/esm-update.ts`:
- Around line 75-87: The code calls loader.load(specifier) which re-resolves the
range and can install a different version than the one returned by
loader.resolveVersion(result); instead, load the exact resolved version: build
an exact specifier using the package name/range and the resolved version (e.g.,
`${specifier}@${result}` or replace the range portion of specifier with result)
and pass that to loader.loadExact or loader.load (the exact-specifier string) so
the version printed from loader.resolveVersion(result) is the one actually
installed; update the call site where loadResult is assigned to use the exact
resolved version (use symbols loader.resolveVersion, result, specifier, and
loader.load/loadExact).
In `@libs/sdk/src/esm-loader/esm-cache.ts`:
- Around line 68-96: The code that computes DEFAULT_CACHE_DIR (and related
branches used by readBundle/getEntryDir) uses CommonJS require('node:path'),
require('node:fs') and require('node:os') which breaks under native ESM; replace
those require() calls with ESM-safe APIs: use dynamic import() for node
built-ins (e.g. await import('node:path') / await import('node:os')) or
top-level ESM imports, and for filesystem checks use the project-wide
`@frontmcp/utils` fs helpers (existence/stat/read/write/mkdir) instead of node:fs
or fs/promises; update the cache-path logic (the nodeModulesDir existence check
and homedir fallback) to call the imported async helpers so DEFAULT_CACHE_DIR is
correctly computed at runtime in ESM environments and ensure readBundle() /
getEntryDir() consume the normalized path value.
In `@libs/sdk/src/esm-loader/factories/esm-record-builders.ts`:
- Around line 33-38: The generated metadata currently hardcodes the output
schema to 'json' and ignores EsmToolDefinition.outputSchema; update the metadata
builder that inspects the tool object (the code writing 'json' for output) to
use tool.outputSchema when present and only fall back to 'json' when it's
undefined, ensuring any caller-provided outputSchema is preserved in the emitted
metadata.
- Around line 44-49: Validate the EsmResourceDefinition.uri before constructing
ResourceMetadata: inside the builder that consumes EsmResourceDefinition (e.g.,
the function that creates ResourceMetadata from an EsmResourceDefinition around
the ResourceMetadata creation at lines ~132-137), add schema validation per RFC
3986 that ensures the URI includes a valid scheme (examples: file://, https://,
custom://) and reject or throw a clear validation error with the message "URI
must have a valid scheme (e.g., file://, https://, custom://)"; perform this
check for all code paths that create ResourceMetadata from
EsmResourceDefinition.name/uri to prevent schemeless or malformed URIs being
persisted.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-esm/browser-app/main.ts`:
- Around line 101-111: The catch block in main.ts assumes the thrown value is an
Error; change it to defensively extract message and stack (use e.g. a check like
`err instanceof Error` or typeof checks) and populate EsmTestResults.error with
a safe string (fallback to String(err) and include stack only if present),
update window.__ESM_RESULTS__ and app.textContent to use the extracted message,
and keep console.error(err) unchanged so non-Error throws are handled without
runtime exceptions; update references around EsmTestResults,
window.__ESM_RESULTS__, and the app textContent assignment.
In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts`:
- Around line 66-146: Repeated test setup (page.goto + waitForFunction +
page.evaluate) should be extracted into a single helper function (e.g.,
loadAndGetResults) placed at the top of the describe block; implement
loadAndGetResults(page: Page) to perform the goto using ESM_SERVER_PORT, wait
for window.__ESM_RESULTS__ with the existing 60000ms timeout, and return the
evaluated __ESM_RESULTS__; then replace the duplicated blocks in each test
(those that currently call page.goto, page.waitForFunction, and page.evaluate)
with a single call to loadAndGetResults and cast its return to the appropriate
shape (e.g., { success: boolean; toolNames: string[] } or other result shapes).
In `@libs/sdk/src/app/instances/app.esm.instance.ts`:
- Around line 160-165: Private fields in AppInstance are declared with redundant
underscore prefixes (_tools, _resources, _prompts, _plugins, _adapters, _skills)
despite using the private keyword and exposing them via getters (tools,
resources, prompts, plugins, adapters, skills); remove the underscore from each
private field name and update all internal references and assignments to use the
new names so the existing getter methods continue to work unchanged. Ensure
constructor parameters and any methods that reference
_tools/_resources/_prompts/_plugins/_adapters/_skills are updated to
tools/resources/prompts/plugins/adapters/skills respectively to keep behavior
identical.
- Around line 336-357: The loop that registers tools can throw and abort the
whole registerFromManifest run; wrap each tool's processing (both the
isDecoratedToolClass branch using normalizeTool → new ToolInstance → await
instance.ready → this._tools.registerToolInstance and the plain-object branch
using normalizeToolFromEsmExport → createEsmToolInstance → await instance.ready
→ this._tools.registerToolInstance) in a try-catch so a failure for one tool
logs the error (with identifying metadata) and continues to the next tool, and
only increments toolCount on successful registration.
- Around line 315-317: The dispose() method in AppEsmInstance currently only
stops this.poller, but should also clean up registries and cache to avoid
resource leaks; update AppEsmInstance.dispose to stop the poller, iterate and
dispose/close each entry in this.registries (or clear the registry map) and call
clear/close on this.cache (and any nested resources), then null out these fields
(this.registries = undefined; this.cache = undefined) so references are
released. Ensure you call appropriate disposal methods on registry objects if
they expose a dispose/close API or otherwise remove listeners and entries before
clearing.
In `@libs/sdk/src/app/instances/esm-normalize.utils.ts`:
- Around line 55-70: The normalizeToolFromEsmExport function currently casts
optional fields (description, inputSchema, outputSchema) without validating
their types; update normalizeToolFromEsmExport to check each optional property
before assignment: ensure description is a string, inputSchema is an object (or
record) and not a function/array, and outputSchema matches expected type (e.g.,
object or undefined) before setting them on the returned EsmToolDefinition;
leave the properties undefined if validation fails so invalid exported values
are not silently cast into incorrect types.
- Around line 103-117: The normalizePromptFromEsmExport function currently casts
obj['arguments'] without checks; validate that obj['arguments'] is an array and
that each element is an object with a string 'name' and optional string
'description' and boolean 'required' before assigning to
EsmPromptDefinition.arguments; implement this by using
Array.isArray(obj['arguments']), filter/map to only include items that pass
typeof checks (item && typeof item === 'object' && typeof item.name === 'string'
and optionally typeof item.description === 'string' and typeof item.required ===
'boolean'), and assign the sanitized array (or undefined if none valid) instead
of the raw cast in normalizePromptFromEsmExport.
In `@libs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.ts`:
- Around line 162-176: Add tests that verify normalization for plain object
exports by creating a plainResource object (with name, uri, and read) and
asserting isDecoratedResourceClass(plainResource) is false, then calling
normalizeResourceFromEsmExport(plainResource) and asserting the returned def is
defined and has name === 'status' and uri === 'status://current'; similarly
create a plainPrompt object (with name and execute), assert
isDecoratedPromptClass(plainPrompt) is false, call
normalizePromptFromEsmExport(plainPrompt) and assert the returned def is defined
and has name === 'greet'. Ensure you add these new it blocks alongside the
existing Resource and Prompt detection tests and use the same helper functions
normalizeResourceFromEsmExport, normalizePromptFromEsmExport,
isDecoratedResourceClass, and isDecoratedPromptClass for locating the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d096b68f-ad60-4872-9106-a9c544fef1e0
📒 Files selected for processing (17)
apps/e2e/demo-e2e-esm/browser-app/main.tsapps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.tslibs/cli/src/commands/package/esm-update.tslibs/cli/src/commands/package/install.tslibs/cli/src/commands/package/types.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/app/instances/app.esm.instance.tslibs/sdk/src/app/instances/esm-normalize.utils.tslibs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.tslibs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.tslibs/sdk/src/esm-loader/esm-cache.tslibs/sdk/src/esm-loader/esm-module-loader.tslibs/sdk/src/esm-loader/factories/esm-record-builders.tslibs/sdk/src/esm-loader/version-resolver.tslibs/sdk/src/skill/skill.utils.tslibs/sdk/src/transport/adapters/transport.sse.adapter.ts
💤 Files with no reviewable changes (1)
- libs/sdk/src/transport/adapters/transport.sse.adapter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/skill/skill.utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts (1)
120-140:⚠️ Potential issue | 🟡 MinorTest is non-asserting for cache behavior.
This test only checks that
node_modulesexists (which is always true in the monorepo), but doesn't verify that the cache directory (projectCacheDir) was actually created or populated. The test will pass regardless of whether caching works.Consider triggering a cache operation and verifying artifacts exist in
projectCacheDir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` around lines 120 - 140, The test "cache directory follows environment-aware logic" is non-asserting about cache creation; update the test to perform a cache-triggering operation (e.g., import or require a module via the ESM loader or perform the same operation that causes caching in your harness) and then assert that projectCacheDir (the variable projectCacheDir) contains the expected cache artifacts by using fileExists(path.join(projectCacheDir, ...)) and expect(...).toBe(true); if needed, remove or recreate the cache before the test to ensure deterministic results and fallback to checking homedirCache when hasNodeModules is false.
🧹 Nitpick comments (3)
libs/sdk/src/esm-loader/version-poller.ts (1)
103-108: Consider returning a boolean or throwing when package not found.
updateCurrentVersionsilently ignores updates for unknown packages. This could hide bugs if a caller expects the version to be updated after a hot-reload but the package was removed from tracking.💡 Suggested improvement
- updateCurrentVersion(packageName: string, newVersion: string): void { + updateCurrentVersion(packageName: string, newVersion: string): boolean { const entry = this.packages.get(packageName); if (entry) { entry.currentVersion = newVersion; + return true; } + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/version-poller.ts` around lines 103 - 108, The method updateCurrentVersion currently silently ignores unknown packages; change its signature to return boolean (e.g., updateCurrentVersion(packageName: string, newVersion: string): boolean) and return true after updating entry.currentVersion, or return false (or optionally throw) when this.packages.get(packageName) is undefined so callers can detect failures; update any callers of updateCurrentVersion to handle the boolean result accordingly.libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts (1)
365-382: Consider wrapping with try/finally for consistency.The previous test at lines 321-363 uses
try/finallyto ensurepoller.stop()runs even on assertion failure. This test placespoller.stop()on the happy path only. While not critical (no shared state mutation), wrapping intry/finallywould maintain consistency and prevent potential timer leaks if assertions fail.♻️ Suggested refactor for consistency
const poller = new VersionPoller({ onNewVersion, registryAuth: { registryUrl }, }); const specifier = parsePackageSpecifier('@test/simple-tools@latest'); poller.addPackage(specifier, '1.0.0'); - const results = await poller.checkNow(); - - expect(results).toHaveLength(1); - expect(results[0].hasUpdate).toBe(false); - - poller.stop(); + try { + const results = await poller.checkNow(); + + expect(results).toHaveLength(1); + expect(results[0].hasUpdate).toBe(false); + } finally { + poller.stop(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 365 - 382, The test for VersionPoller should ensure poller.stop() runs even if assertions fail; wrap the main test logic (creating the VersionPoller, adding the package via parsePackageSpecifier and calling poller.checkNow()) in a try/finally and call poller.stop() in the finally block so timers are always cleaned up; locate the test using the VersionPoller constructor, parsePackageSpecifier('@test/simple-tools@latest') and the poller.stop() call and move the existing stop() into a finally to mirror the surrounding tests.apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts (1)
87-151: Consider extracting repeated setup pattern.Each test repeats the same
goto→waitForFunction→evaluatesequence. A helper would reduce duplication:async function getEsmResults(page: Page) { await page.goto(`/?esmServer=http://127.0.0.1:${ESM_SERVER_PORT}`); await page.waitForFunction( () => (window as unknown as { __ESM_RESULTS__?: unknown }).__ESM_RESULTS__, { timeout: 60000 } ); return page.evaluate( () => (window as unknown as { __ESM_RESULTS__: unknown }).__ESM_RESULTS__ ); }This is optional since the current approach is readable and explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts` around lines 87 - 151, Tests repeat the same page setup and retrieval logic (page.goto, page.waitForFunction, page.evaluate); add a helper function getEsmResults(page: Page) that performs the goto(`/?esmServer=http://127.0.0.1:${ESM_SERVER_PORT}`), waits for window.__ESM_RESULTS__ with the 60000ms timeout, and returns the evaluated __ESM_RESULTS__; then replace the repeated blocks in each test (the ESM echo/add/greet/resources/prompts tests) with a single await getEsmResults(page) call and cast the returned object as needed (e.g., { success: boolean; echoResult: ... }).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/schemas/annotated-class.schema.ts`:
- Around line 194-197: The validation now accepts string specifiers (variable v)
for agents, so update the error text emitted in the agents validation (in
annotated-class.schema.ts, around the existing error at Line 224) to mention
both accepted forms: string package specifiers (e.g., '@acme/agents@^1.0.0') and
the decorator/builder forms; modify the message to clearly list "string
specifier" alongside "decorator/builder" so it matches the actual validation
logic.
- Around line 143-145: Replace the permissive "typeof v === 'string'" checks in
the annotated-class schema guards with a strict package-specifier validation:
import and reuse the same parser/validator used by the ESM loader path (e.g.,
the project's parsePackageSpecifier/validateSpecifier utility) and call it on v
to ensure it is non-empty and correctly formed; if the parser/validator throws
or returns false, have the guard return false (otherwise return true). Apply
this change to both guards that currently accept any string (the checks around v
at the two locations noted) so malformed specifiers fail schema validation
early.
- Around line 140-145: Add documentation describing that the SDK now accepts ESM
package specifier strings for tools and agents (e.g., '@acme/tools@^1.0.0') to
match the updated zod union in annotatedFrontMcpToolsSchema; update
docs/frontmcp/sdk-reference/registries/tool-registry.mdx and
docs/frontmcp/sdk-reference/registries/agent-registry.mdx to include a short
explanation of package specifier support, show example registrations using a
specifier string and the equivalent direct Type import, and note validation
behavior consistent with annotatedFrontMcpToolsSchema so users know both string
specifiers and Type objects are accepted.
---
Duplicate comments:
In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts`:
- Around line 120-140: The test "cache directory follows environment-aware
logic" is non-asserting about cache creation; update the test to perform a
cache-triggering operation (e.g., import or require a module via the ESM loader
or perform the same operation that causes caching in your harness) and then
assert that projectCacheDir (the variable projectCacheDir) contains the expected
cache artifacts by using fileExists(path.join(projectCacheDir, ...)) and
expect(...).toBe(true); if needed, remove or recreate the cache before the test
to ensure deterministic results and fallback to checking homedirCache when
hasNodeModules is false.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.ts`:
- Around line 87-151: Tests repeat the same page setup and retrieval logic
(page.goto, page.waitForFunction, page.evaluate); add a helper function
getEsmResults(page: Page) that performs the
goto(`/?esmServer=http://127.0.0.1:${ESM_SERVER_PORT}`), waits for
window.__ESM_RESULTS__ with the 60000ms timeout, and returns the evaluated
__ESM_RESULTS__; then replace the repeated blocks in each test (the ESM
echo/add/greet/resources/prompts tests) with a single await getEsmResults(page)
call and cast the returned object as needed (e.g., { success: boolean;
echoResult: ... }).
In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts`:
- Around line 365-382: The test for VersionPoller should ensure poller.stop()
runs even if assertions fail; wrap the main test logic (creating the
VersionPoller, adding the package via parsePackageSpecifier and calling
poller.checkNow()) in a try/finally and call poller.stop() in the finally block
so timers are always cleaned up; locate the test using the VersionPoller
constructor, parsePackageSpecifier('@test/simple-tools@latest') and the
poller.stop() call and move the existing stop() into a finally to mirror the
surrounding tests.
In `@libs/sdk/src/esm-loader/version-poller.ts`:
- Around line 103-108: The method updateCurrentVersion currently silently
ignores unknown packages; change its signature to return boolean (e.g.,
updateCurrentVersion(packageName: string, newVersion: string): boolean) and
return true after updating entry.currentVersion, or return false (or optionally
throw) when this.packages.get(packageName) is undefined so callers can detect
failures; update any callers of updateCurrentVersion to handle the boolean
result accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9e57b82-2e48-4ffc-9ecf-ecad33213917
📒 Files selected for processing (12)
apps/e2e/demo-e2e-esm/browser-app/main.tsapps/e2e/demo-e2e-esm/e2e/browser/esm-browser.pw.spec.tsapps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.tslibs/sdk/src/common/interfaces/tool.interface.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/errors/esm.errors.tslibs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.tslibs/sdk/src/esm-loader/factories/esm-record-builders.tslibs/sdk/src/esm-loader/semver.utils.tslibs/sdk/src/esm-loader/version-poller.tslibs/sdk/src/tool/tool.utils.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/sdk/src/esm-loader/tests/esm-module-loader.spec.ts
- libs/sdk/src/common/interfaces/tool.interface.ts
- libs/sdk/src/tool/tool.utils.ts
- libs/sdk/src/esm-loader/factories/esm-record-builders.ts
- libs/sdk/src/esm-loader/semver.utils.ts
- apps/e2e/demo-e2e-esm/browser-app/main.ts
… in version polling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/sdk/src/esm-loader/version-poller.ts (2)
85-90:⚠️ Potential issue | 🟠 MajorUse a range-aware key for tracked packages.
The Map is still keyed only by
specifier.fullName, so adding the same package with different ranges (e.g.,@acme/tools@^1and@acme/tools@next) silently overwrites the first entry. Key by the full specifier (e.g.,specifier.raw) or reject duplicates explicitly.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/version-poller.ts` around lines 85 - 90, The addPackage method currently keys this.packages by specifier.fullName which causes entries with the same package but different ranges to overwrite each other; change the key to a range-aware identifier (e.g., use specifier.raw or another full specifier string) when calling this.packages.set in addPackage, or alternatively detect duplicates and throw/reject if a different range is already tracked for the same package; update any lookup logic that expects the old key accordingly so addPackage, this.packages, and any get/remove methods remain consistent.
208-224:⚠️ Potential issue | 🟠 MajorUpdate detection breaks for dist-tagged packages.
When
entry.specifier.rangeis a dist-tag likebetaornext,satisfiesRange()passes it tosemver.satisfies(), which returnsfalsefor non-semver range strings. This causes the poller to never report updates for packages specified with dist-tag ranges.Consider extending
satisfiesRange()to detect dist-tag names (non-semver strings that successfully resolved) and treat them as satisfying, or handle this case explicitly incheckPackage().,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/version-poller.ts` around lines 208 - 224, checkPackage currently uses satisfiesRange(resolution.resolvedVersion, entry.specifier.range) which fails for dist-tag ranges like "beta"/"next"; modify checkPackage to treat a non-semver dist-tag as satisfying the range by detecting when entry.specifier.range is a tag (e.g., contains no semver operators/versions) and the resolver returned that tag (resolution.resolvedVersion or a resolution.distTag field), or update satisfiesRange to consider dist-tags valid: compute rangeMatch = satisfiesRange(...) || (isDistTag(entry.specifier.range) && resolution.resolvedTag === entry.specifier.range); then use rangeMatch in the returned hasUpdate/satisfiesRange fields and refer to functions/values resolution.resolvedVersion, resolution.resolvedTag (or equivalent), entry.specifier.range, checkPackage, and satisfiesRange to locate the change.
🧹 Nitpick comments (1)
libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts (1)
333-349: AssertonNewVersionbehavior too.These cases only inspect
checkNow()results. If the poller stops dispatching the callback, both tests still pass even though the update signal is broken.🧪 Tighten the poller assertions
expect(results[0].hasUpdate).toBe(true); expect(results[0].latestVersion).toBe('2.0.0'); expect(results[0].currentVersion).toBe('1.0.0'); + expect(onNewVersion).toHaveBeenCalledTimes(1); @@ expect(results).toHaveLength(1); expect(results[0].hasUpdate).toBe(false); + expect(onNewVersion).not.toHaveBeenCalled();Also applies to: 365-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 333 - 349, The tests currently only assert return values from VersionPoller.checkNow() but not that the callback was invoked; update the tests that create VersionPoller and mock onNewVersion (the ones using VersionPoller, onNewVersion, parsePackageSpecifier, addPackage, and checkNow) to also assert the mocked onNewVersion was called (e.g., expect(onNewVersion).toHaveBeenCalledTimes(1) and optionally toHaveBeenCalledWith the expected payload or specifier/latestVersion/currentVersion) so the poller dispatch behavior is verified; add similar assertions to the other failing test block around lines 365–380 that uses the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/sdk/src/esm-loader/version-poller.ts`:
- Around line 85-90: The addPackage method currently keys this.packages by
specifier.fullName which causes entries with the same package but different
ranges to overwrite each other; change the key to a range-aware identifier
(e.g., use specifier.raw or another full specifier string) when calling
this.packages.set in addPackage, or alternatively detect duplicates and
throw/reject if a different range is already tracked for the same package;
update any lookup logic that expects the old key accordingly so addPackage,
this.packages, and any get/remove methods remain consistent.
- Around line 208-224: checkPackage currently uses
satisfiesRange(resolution.resolvedVersion, entry.specifier.range) which fails
for dist-tag ranges like "beta"/"next"; modify checkPackage to treat a
non-semver dist-tag as satisfying the range by detecting when
entry.specifier.range is a tag (e.g., contains no semver operators/versions) and
the resolver returned that tag (resolution.resolvedVersion or a
resolution.distTag field), or update satisfiesRange to consider dist-tags valid:
compute rangeMatch = satisfiesRange(...) || (isDistTag(entry.specifier.range) &&
resolution.resolvedTag === entry.specifier.range); then use rangeMatch in the
returned hasUpdate/satisfiesRange fields and refer to functions/values
resolution.resolvedVersion, resolution.resolvedTag (or equivalent),
entry.specifier.range, checkPackage, and satisfiesRange to locate the change.
---
Nitpick comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts`:
- Around line 333-349: The tests currently only assert return values from
VersionPoller.checkNow() but not that the callback was invoked; update the tests
that create VersionPoller and mock onNewVersion (the ones using VersionPoller,
onNewVersion, parsePackageSpecifier, addPackage, and checkNow) to also assert
the mocked onNewVersion was called (e.g.,
expect(onNewVersion).toHaveBeenCalledTimes(1) and optionally
toHaveBeenCalledWith the expected payload or
specifier/latestVersion/currentVersion) so the poller dispatch behavior is
verified; add similar assertions to the other failing test block around lines
365–380 that uses the same symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dae448df-a77d-41ac-8c61-6f66898bed05
📒 Files selected for processing (3)
libs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/version-poller.ts
…d improve related tests
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts (1)
22-29:⚠️ Potential issue | 🟠 MajorDrive the happy-path suite through
EsmModuleLoader.This helper is the root of a suite-wide gap: the success cases below manually resolve/fetch/write/import bundles instead of calling the public loader. That means URL construction, cache reuse, and
esmShBaseUrlwiring can break while this file still stays green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 22 - 29, The tests currently bypass the public loader by using loadBundleFromDisk which delete/require's bundles; change the happy-path test helper and tests to exercise the EsmModuleLoader instead: stop using loadBundleFromDisk's require/cache manipulation and invoke the EsmModuleLoader public API (instantiate EsmModuleLoader or call its load/resolve method) to fetch/resolve/load the bundle so URL construction, cache reuse and esmShBaseUrl wiring are exercised by the suite; update tests that referenced loadBundleFromDisk to call EsmModuleLoader methods and use its cache/teardown APIs rather than touching require.cache directly.
🧹 Nitpick comments (2)
libs/sdk/src/esm-loader/semver.utils.ts (1)
62-77: Consider documenting the throwing behavior.
semver.compareandsemver.gtthrow aTypeErrorif either argument is an invalid semver string. Since this wrapper is part of the public SDK surface (re-exported viaesm-loader/index.ts), consumers may benefit from explicit@throwsJSDoc annotations.📝 Suggested documentation improvement
/** * Compare two versions. Returns: * - negative if v1 < v2 * - 0 if v1 === v2 * - positive if v1 > v2 + * `@throws` {TypeError} If either version is not a valid semver string */ export function compareVersions(v1: string, v2: string): number { return semver.compare(v1, v2); } /** * Check if v1 is greater than v2. + * `@throws` {TypeError} If either version is not a valid semver string */ export function isNewerVersion(v1: string, v2: string): boolean { return semver.gt(v1, v2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/semver.utils.ts` around lines 62 - 77, The exported helpers compareVersions and isNewerVersion can throw a TypeError when passed invalid semver strings; add JSDoc `@throws` annotations to both functions (compareVersions and isNewerVersion) documenting that a TypeError is thrown for invalid version arguments and include a short note about expected input (valid semver strings) so consumers of the public SDK surface are aware of the throwing behavior.libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts (1)
231-235: Replace the manifest non-null assertions with explicit guards.
tools!,resources!, andprompts!sidestep the repo rule and turn missing primitives into type escapes instead of clear test failures. Pull each collection into a local after an explicit guard/helper assertion before indexing into it.As per coding guidelines, "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found, rather than using non-null assertion operator (!)".
Also applies to: 279-317, 401-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 231 - 235, The test uses non-null assertions on manifest.tools/resources/prompts which bypass proper failure reporting; replace each use of manifest.tools!, manifest.resources!, and manifest.prompts! by first asserting the collection exists (e.g., if (!manifest.tools) throw new Error('manifest.tools missing') or use a small helper assertDefined) and assign it to a local variable (const tools = manifest.tools) before indexing into it; do the same for resources and prompts and update all occurrences referenced in this spec (including the blocks around lines noted) so missing collections produce explicit, descriptive test errors instead of runtime type-escape assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts`:
- Around line 22-29: The tests currently bypass the public loader by using
loadBundleFromDisk which delete/require's bundles; change the happy-path test
helper and tests to exercise the EsmModuleLoader instead: stop using
loadBundleFromDisk's require/cache manipulation and invoke the EsmModuleLoader
public API (instantiate EsmModuleLoader or call its load/resolve method) to
fetch/resolve/load the bundle so URL construction, cache reuse and esmShBaseUrl
wiring are exercised by the suite; update tests that referenced
loadBundleFromDisk to call EsmModuleLoader methods and use its cache/teardown
APIs rather than touching require.cache directly.
---
Nitpick comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts`:
- Around line 231-235: The test uses non-null assertions on
manifest.tools/resources/prompts which bypass proper failure reporting; replace
each use of manifest.tools!, manifest.resources!, and manifest.prompts! by first
asserting the collection exists (e.g., if (!manifest.tools) throw new
Error('manifest.tools missing') or use a small helper assertDefined) and assign
it to a local variable (const tools = manifest.tools) before indexing into it;
do the same for resources and prompts and update all occurrences referenced in
this spec (including the blocks around lines noted) so missing collections
produce explicit, descriptive test errors instead of runtime type-escape
assertions.
In `@libs/sdk/src/esm-loader/semver.utils.ts`:
- Around line 62-77: The exported helpers compareVersions and isNewerVersion can
throw a TypeError when passed invalid semver strings; add JSDoc `@throws`
annotations to both functions (compareVersions and isNewerVersion) documenting
that a TypeError is thrown for invalid version arguments and include a short
note about expected input (valid semver strings) so consumers of the public SDK
surface are aware of the throwing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f40b4909-d03e-42c6-9423-95b54bf81710
📒 Files selected for processing (3)
libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/__tests__/semver.utils.spec.tslibs/sdk/src/esm-loader/semver.utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/esm-loader/tests/semver.utils.spec.ts
…elated references
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts (1)
142-166:⚠️ Potential issue | 🟡 MinorTest doesn't verify explicit
cacheDiroverride is used.The test is named "explicit cacheDir option overrides default" but
testCacheDir(created at line 28) is never passed to theconnect()call. The test only verifies that tools are listed, not that caching occurred in the custom directory.To validate the override, pass the explicit cache directory in the loader config and assert that artifacts exist in
testCacheDirafter loading.🐛 Proposed fix
// Use explicit cacheDir pointing to temp directory const customClient = await connect( { info: { name: 'ESM CLI Custom Cache', version: '0.1.0' }, - loader: { url: esmServerUrl }, + loader: { url: esmServerUrl, cacheDir: testCacheDir }, apps: [ loadFrom('@test/esm-tools@^1.0.0', { namespace: 'custom', cacheTTL: 60000, }), ], logging: { level: LogLevel.Warn }, }, { mode: 'cli' }, ); // Tools should still work regardless of cache location const tools = await customClient.listTools(); const toolNames = tools.tools.map((t) => t.name); expect(toolNames).toContain('custom:echo'); + + // Verify cache was written to explicit directory + const cacheExists = await fileExists(testCacheDir); + expect(cacheExists).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` around lines 142 - 166, The test titled "explicit cacheDir option overrides default" never passes the created testCacheDir into connect(), so update the loader config passed to connect() (the loadFrom call) to include cacheDir: testCacheDir; after creating customClient and invoking an operation that triggers caching (e.g., await customClient.listTools() or another loader action), assert that artifacts were written to testCacheDir (use the existing test helper or fs.existsSync/assertions to verify expected files are present) to prove the explicit cacheDir override was used. Ensure you reference the connect, loadFrom, customClient, listTools, and testCacheDir identifiers when making the change.libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts (2)
322-363:⚠️ Potential issue | 🟡 MinorStart the cleanup guard before mutating
@test/simple-tools.
server.addPackage()flipslatestto2.0.0before thetrystarts. If poller/specifier setup throws, the restore block never runs and later cases inherit the mutated registry metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 322 - 363, Move the cleanup/restore guard to run before you mutate the test registry: create the poller/specifier and register a finally-based restore (or save the original package metadata) prior to calling server.addPackage that flips '@test/simple-tools' to 2.0.0 so any thrown setup errors still trigger restoration; specifically, ensure the restore call that re-adds the original dist-tags runs in a finally block that is registered before invoking server.addPackage, referencing VersionPoller (poller), parsePackageSpecifier (specifier), and server.addPackage so teardown always executes even if poller/specifier setup fails.
22-29:⚠️ Potential issue | 🟠 MajorThis suite still bypasses the real loader path.
These cases go through raw
fetchorfetch→writeFile→require(), soEsmModuleLoaderURL construction, cache population/reuse, and customesmShBaseUrlwiring can break while this file stays green. Please drive at least one end-to-end case through the production loader API instead of manually loading the bundle from disk.Also applies to: 220-229, 444-458, 479-488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 22 - 29, The tests currently bypass the real loader by using the helper loadBundleFromDisk; replace or add at least one end-to-end test that exercises the production loader API (EsmModuleLoader) instead of require(): instantiate EsmModuleLoader with the test esmShBaseUrl, call its public load method (e.g., loadModule / load) with a file:// or server URL to the built bundle, and assert the module loads and the loader's cache population/reuse behavior; remove the direct use of loadBundleFromDisk from that case so URL construction, esmShBaseUrl wiring and cache logic are exercised end-to-end.
🧹 Nitpick comments (5)
libs/sdk/src/esm-loader/semver.utils.ts (1)
62-77: Consider adding JSDoc@throwsannotations or input validation.Both
compareVersionsandisNewerVersiondelegate directly tosemver.compare/semver.gt, which throwTypeErrorif either version string is invalid. Callers must validate inputs withisValidVersion()first, but this contract isn't documented.📝 Option 1: Document the throwing behavior
/** * Compare two versions. Returns: * - negative if v1 < v2 * - 0 if v1 === v2 * - positive if v1 > v2 + * + * `@throws` {TypeError} If either version is not a valid semver string */ export function compareVersions(v1: string, v2: string): number { return semver.compare(v1, v2); } /** * Check if v1 is greater than v2. + * + * `@throws` {TypeError} If either version is not a valid semver string */ export function isNewerVersion(v1: string, v2: string): boolean { return semver.gt(v1, v2); }📝 Option 2: Add defensive validation (returns safe defaults)
export function compareVersions(v1: string, v2: string): number { + if (!semver.valid(v1) || !semver.valid(v2)) { + throw new TypeError(`Invalid semver version(s): ${v1}, ${v2}`); + } return semver.compare(v1, v2); } export function isNewerVersion(v1: string, v2: string): boolean { + if (!semver.valid(v1) || !semver.valid(v2)) { + return false; + } return semver.gt(v1, v2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/semver.utils.ts` around lines 62 - 77, The exported functions compareVersions and isNewerVersion currently call semver.compare/semver.gt directly and can throw for invalid inputs; add input validation using isValidVersion for both v1 and v2 at the start of each function and if validation fails throw a clear TypeError (e.g., "Invalid semver: <value>") so callers get explicit errors, and update the JSDoc for both functions to include an `@throws` annotation describing the TypeError; ensure you reference the same function names (compareVersions, isNewerVersion) and the isValidVersion helper when implementing this check.apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts (2)
120-140: Cache directory test doesn't verify actual caching behavior.This test only checks whether
node_modulesexists in the current directory and logs the expected cache path. It doesn't confirm that the ESM loader actually created or used that cache directory.Consider asserting that
projectCacheDirexists (or contains files) after tools are loaded, to catch regressions in cache resolution logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` around lines 120 - 140, The test "cache directory follows environment-aware logic" currently only checks for node_modules and logs paths; modify it to assert the ESM loader actually created/used the cache by checking that projectCacheDir (and fallback homedirCache when hasNodeModules is false) exists or contains files after the loader runs: use fileExists(projectCacheDir) (and/or inspect contents) and add expect(fileExists(projectCacheDir)).toBe(true) (or corresponding assertions for homedirCache) so the test fails if the cache resolution/creation logic in the ESM loader does not run.
68-68: Avoid non-null assertion when a guard improves test clarity.If
esmServeris unexpectedlynull(e.g., due to a startup failure that didn't throw), the!assertion will produce a confusing runtime error. A defensive check or throwing an explicit error would provide clearer diagnostics.🛡️ Suggested improvement
- const esmServerUrl = `http://127.0.0.1:${esmServer!.info.port}`; + if (!esmServer) { + throw new Error('ESM server failed to start in beforeAll'); + } + const esmServerUrl = `http://127.0.0.1:${esmServer.info.port}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` at line 68, Replace the non-null assertion on esmServer when building esmServerUrl with a clear guard: check that the esmServer variable is defined (e.g., if (!esmServer) throw new Error("ESM server not started: esmServer is null/undefined")) and then use esmServer.info.port to construct esmServerUrl; reference the esmServer variable and the line creating esmServerUrl to locate where to add the guard and error.apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts (1)
169-210: Consider adding a body size limit for robustness.The admin publish handler accumulates request body chunks without any size limit. While this is a local E2E test server, adding a reasonable limit prevents accidental hangs from malformed requests during development.
🛡️ Optional: Add body size limit
function handleAdminPublish(req: http.IncomingMessage, res: http.ServerResponse): void { let body = ''; + const MAX_BODY_SIZE = 1024 * 1024; // 1MB limit req.on('data', (chunk: Buffer) => { + if (body.length + chunk.length > MAX_BODY_SIZE) { + res.writeHead(413, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Payload too large' })); + req.destroy(); + return; + } body += chunk.toString(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts` around lines 169 - 210, The handler handleAdminPublish currently accumulates request body chunks into body without any size checks; add a maximum body size (e.g., const MAX_BODY_BYTES = 1_000_000) and track received bytes inside the req.on('data') callback, and if the running total exceeds the limit immediately respond with a 413 Payload Too Large (res.writeHead(413,...); res.end(...)) and stop processing (remove listeners or call req.destroy()/return) to prevent further accumulation; keep the existing JSON parsing flow unchanged for acceptable-sized bodies and reference handleAdminPublish, the req.on('data') callback, the body variable, and the req.on('end') JSON.parse block when making the change.libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts (1)
231-235: Avoid!on optional manifest collections in these tests.These casts sidestep the optionality in
FrontMcpPackageManifest; in the execution cases they turn missing exports into TypeErrors, and in the assertion cases they're unnecessary aftertoHaveLength(). Prefer an explicit narrowing step before indexing.As per coding guidelines, "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found, rather than using non-null assertion operator (!)"
Also applies to: 279-317, 401-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts` around lines 231 - 235, The tests currently use non-null assertions on the optional manifest.tools (e.g., (manifest.tools![0] as { name: string }).name) which bypasses optionality; instead explicitly narrow or fail early: after asserting expect(manifest.tools).toHaveLength(1) grab a local const tools = manifest.tools and check it is defined (or throw a clear Error) before indexing, then assert expect(tools[0].name).toBe('echo'); apply the same explicit narrowing/early-fail pattern to the other test blocks mentioned (lines ~279-317 and ~401-415) to remove all `!` usages on manifest collections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts`:
- Around line 142-166: The test titled "explicit cacheDir option overrides
default" never passes the created testCacheDir into connect(), so update the
loader config passed to connect() (the loadFrom call) to include cacheDir:
testCacheDir; after creating customClient and invoking an operation that
triggers caching (e.g., await customClient.listTools() or another loader
action), assert that artifacts were written to testCacheDir (use the existing
test helper or fs.existsSync/assertions to verify expected files are present) to
prove the explicit cacheDir override was used. Ensure you reference the connect,
loadFrom, customClient, listTools, and testCacheDir identifiers when making the
change.
In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts`:
- Around line 322-363: Move the cleanup/restore guard to run before you mutate
the test registry: create the poller/specifier and register a finally-based
restore (or save the original package metadata) prior to calling
server.addPackage that flips '@test/simple-tools' to 2.0.0 so any thrown setup
errors still trigger restoration; specifically, ensure the restore call that
re-adds the original dist-tags runs in a finally block that is registered before
invoking server.addPackage, referencing VersionPoller (poller),
parsePackageSpecifier (specifier), and server.addPackage so teardown always
executes even if poller/specifier setup fails.
- Around line 22-29: The tests currently bypass the real loader by using the
helper loadBundleFromDisk; replace or add at least one end-to-end test that
exercises the production loader API (EsmModuleLoader) instead of require():
instantiate EsmModuleLoader with the test esmShBaseUrl, call its public load
method (e.g., loadModule / load) with a file:// or server URL to the built
bundle, and assert the module loads and the loader's cache population/reuse
behavior; remove the direct use of loadBundleFromDisk from that case so URL
construction, esmShBaseUrl wiring and cache logic are exercised end-to-end.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts`:
- Around line 120-140: The test "cache directory follows environment-aware
logic" currently only checks for node_modules and logs paths; modify it to
assert the ESM loader actually created/used the cache by checking that
projectCacheDir (and fallback homedirCache when hasNodeModules is false) exists
or contains files after the loader runs: use fileExists(projectCacheDir) (and/or
inspect contents) and add expect(fileExists(projectCacheDir)).toBe(true) (or
corresponding assertions for homedirCache) so the test fails if the cache
resolution/creation logic in the ESM loader does not run.
- Line 68: Replace the non-null assertion on esmServer when building
esmServerUrl with a clear guard: check that the esmServer variable is defined
(e.g., if (!esmServer) throw new Error("ESM server not started: esmServer is
null/undefined")) and then use esmServer.info.port to construct esmServerUrl;
reference the esmServer variable and the line creating esmServerUrl to locate
where to add the guard and error.
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 169-210: The handler handleAdminPublish currently accumulates
request body chunks into body without any size checks; add a maximum body size
(e.g., const MAX_BODY_BYTES = 1_000_000) and track received bytes inside the
req.on('data') callback, and if the running total exceeds the limit immediately
respond with a 413 Payload Too Large (res.writeHead(413,...); res.end(...)) and
stop processing (remove listeners or call req.destroy()/return) to prevent
further accumulation; keep the existing JSON parsing flow unchanged for
acceptable-sized bodies and reference handleAdminPublish, the req.on('data')
callback, the body variable, and the req.on('end') JSON.parse block when making
the change.
In `@libs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.ts`:
- Around line 231-235: The tests currently use non-null assertions on the
optional manifest.tools (e.g., (manifest.tools![0] as { name: string }).name)
which bypasses optionality; instead explicitly narrow or fail early: after
asserting expect(manifest.tools).toHaveLength(1) grab a local const tools =
manifest.tools and check it is defined (or throw a clear Error) before indexing,
then assert expect(tools[0].name).toBe('echo'); apply the same explicit
narrowing/early-fail pattern to the other test blocks mentioned (lines ~279-317
and ~401-415) to remove all `!` usages on manifest collections.
In `@libs/sdk/src/esm-loader/semver.utils.ts`:
- Around line 62-77: The exported functions compareVersions and isNewerVersion
currently call semver.compare/semver.gt directly and can throw for invalid
inputs; add input validation using isValidVersion for both v1 and v2 at the
start of each function and if validation fails throw a clear TypeError (e.g.,
"Invalid semver: <value>") so callers get explicit errors, and update the JSDoc
for both functions to include an `@throws` annotation describing the TypeError;
ensure you reference the same function names (compareVersions, isNewerVersion)
and the isValidVersion helper when implementing this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a915ab7-4ef7-4662-8e4e-19a903519ef6
📒 Files selected for processing (10)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.tsapps/e2e/demo-e2e-esm/e2e/esm-hot-reload.e2e.spec.tsapps/e2e/demo-e2e-esm/e2e/esm.e2e.spec.tsapps/e2e/demo-e2e-esm/src/esm-package-server/main.tsapps/e2e/demo-e2e-esm/src/main-hot-reload.tsapps/e2e/demo-e2e-esm/src/main.tslibs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/__tests__/semver.utils.spec.tslibs/sdk/src/esm-loader/semver.utils.tslibs/testing/src/server/port-registry.ts
✅ Files skipped from review due to trivial changes (1)
- apps/e2e/demo-e2e-esm/e2e/esm.e2e.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sdk/src/esm-loader/tests/semver.utils.spec.ts
- apps/e2e/demo-e2e-esm/e2e/esm-hot-reload.e2e.spec.ts
- apps/e2e/demo-e2e-esm/src/main.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/scope/__tests__/scope-init-perf.spec.ts`:
- Around line 138-143: The test currently only logs liteMs and fullMs when
FRONTMCP_PERF=1 but has no assertion, so add a real expectation using the
existing variables (liteMs and fullMs) to enforce the stated performance
guarantee: compute ratio = liteMs / fullMs and assert
expect(ratio).toBeLessThanOrEqual(<acceptableFactor>) (or
expect(liteMs).toBeLessThanOrEqual(fullMs * <acceptableFactor>)) so regressions
fail; keep the optional console.log gated by process.env['FRONTMCP_PERF'] and
choose an appropriate numeric <acceptableFactor> that matches the performance
guarantee declared earlier in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ac0b3e6-d4be-43a3-a233-ac1645b7c80e
📒 Files selected for processing (1)
libs/sdk/src/scope/__tests__/scope-init-perf.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/sdk/src/app/__tests__/app.utils.spec.ts (2)
52-55: Make the invalid-URI assertion specific.
toThrow()is too broad and can pass on unrelated exceptions. Assert a concrete error type to lock the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/__tests__/app.utils.spec.ts` around lines 52 - 55, The test currently uses a broad expect(...).toThrow() which can mask unrelated exceptions; update the assertion to check for a specific error from normalizeApp when given an invalid URI (e.g., expect(() => normalizeApp(input as any)).toThrowError(/invalid uri/i) or toThrowError(SomeSpecificError) so the test asserts the concrete error type/message indicating an invalid URI from normalizeApp).
9-55: Replace repeatedas anycasts with a typed helper function.The test contains 7 instances of
as anycasts (lines 9, 17, 25, 33, 41, 46, 55) that bypass type-checking and can mask regressions. Extract a single helper function to eliminate the repetition:♻️ Suggested refactor
import 'reflect-metadata'; import { normalizeApp } from '../app.utils'; import { AppKind } from '../../common'; import { MissingProvideError } from '../../errors'; +const asAppType = (value: unknown): Parameters<typeof normalizeApp>[0] => + value as Parameters<typeof normalizeApp>[0]; + describe('normalizeApp', () => { it('should recognize npm remote app with package specifier (no URI scheme)', () => { const input = { name: '@test/esm-tools', urlType: 'npm', url: '@test/esm-tools@^1.0.0', standalone: false }; - const result = normalizeApp(input as any); + const result = normalizeApp(asAppType(input));This approach uses
unknownas the source type and replaces all 7 occurrences with the typed helper, improving type safety per coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/__tests__/app.utils.spec.ts` around lines 9 - 55, Tests repeatedly bypass TypeScript with seven "as any" casts when calling normalizeApp; add a single typed helper function (e.g., castToAppInput(input: unknown): YourAppInputType) and replace every "normalizeApp(input as any)" with "normalizeApp(castToAppInput(input))" to preserve runtime behavior while restoring type-safety; locate uses around normalizeApp, AppKind, and MissingProvideError in the spec and ensure the helper returns the same structure (unknown -> typed input) so all assertions remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/app/app.utils.ts`:
- Around line 33-41: isRemoteAppConfig currently accepts any string urlType if
the url has a valid scheme; update the type guard to explicitly allow only the
supported urlType values. In the isRemoteAppConfig function, replace the final
unconditional isValidMcpUri(obj['url']) return with a whitelist check: return
true for 'npm' or 'esm'; for 'url' and 'worker' call isValidMcpUri(obj['url']);
for any other obj['urlType'] return false so unsupported transport types (e.g.,
"foo") are rejected and the result safely matches RemoteAppMetadata.
---
Nitpick comments:
In `@libs/sdk/src/app/__tests__/app.utils.spec.ts`:
- Around line 52-55: The test currently uses a broad expect(...).toThrow() which
can mask unrelated exceptions; update the assertion to check for a specific
error from normalizeApp when given an invalid URI (e.g., expect(() =>
normalizeApp(input as any)).toThrowError(/invalid uri/i) or
toThrowError(SomeSpecificError) so the test asserts the concrete error
type/message indicating an invalid URI from normalizeApp).
- Around line 9-55: Tests repeatedly bypass TypeScript with seven "as any" casts
when calling normalizeApp; add a single typed helper function (e.g.,
castToAppInput(input: unknown): YourAppInputType) and replace every
"normalizeApp(input as any)" with "normalizeApp(castToAppInput(input))" to
preserve runtime behavior while restoring type-safety; locate uses around
normalizeApp, AppKind, and MissingProvideError in the spec and ensure the helper
returns the same structure (unknown -> typed input) so all assertions remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba6e7884-0916-47a1-91b8-0b592e3043f5
📒 Files selected for processing (3)
libs/sdk/src/app/__tests__/app.utils.spec.tslibs/sdk/src/app/app.utils.tslibs/sdk/src/scope/__tests__/scope-init-perf.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/scope/tests/scope-init-perf.spec.ts
…improved error handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/testing/src/server/test-server.ts`:
- Around line 111-113: The catch block in the startup routine currently does
`await server.stop()` which can throw and mask the original startup error
(preventing the EADDRINUSE retry path); change it to perform cleanup as
best-effort by calling `server.stop()` inside its own try/catch (or not awaiting
it) and ensure the original caught error is rethrown afterwards so the
EADDRINUSE logic (and original error) are preserved; reference the
`server.stop()` call in the catch within the startup flow in test-server.ts and
ensure any errors from `server.stop()` are caught/logged but do not replace the
original exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da1034db-a08c-403d-8572-feecc6de1c40
📒 Files selected for processing (3)
libs/sdk/src/app/__tests__/app.utils.spec.tslibs/sdk/src/app/app.utils.tslibs/testing/src/server/test-server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/app/tests/app.utils.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/sdk/src/esm-loader/esm-cache.ts (1)
87-110:⚠️ Potential issue | 🟠 MajorReplace runtime
require(...)/node:fsusage with ESM imports +@frontmcp/utilsabstractions.Line 87/163/210/265/315/357/365 use
require(...), and Line 267/317 importsnode:fs/promises. This is fragile for native ESM output and bypasses the project’s FS abstraction rule.Proposed direction
-import { sha256Hex } from '@frontmcp/utils'; +import { + sha256Hex, + fileExists, + readJSON, + writeFile, + ensureDir, + writeJSON, + rm, + readdir, + // pathJoin, homedir, tmpdir (or equivalent utils exports) +} from '@frontmcp/utils'; +import * as path from 'node:path'; // only if no utils path helpers exist -const path = require('node:path'); -const fs = require('node:fs'); -const os = require('node:os'); -const { readdir } = await import('node:fs/promises'); +// use static imports + utils wrappers onlyVerification (read-only): confirm remaining CJS runtime imports and direct Node FS usage.
#!/bin/bash set -euo pipefail echo "== esm-cache.ts: CJS runtime imports / direct Node FS usage ==" rg -n --type ts "require\\(|node:fs/promises|node:fs|node:path|node:os" libs/sdk/src/esm-loader/esm-cache.ts echo echo "== SDK package export shape (check ESM surface) ==" fd package.json libs/sdk -x sh -c 'echo "--- {} ---"; jq "{type:.type,exports:.exports}" "{}"'As per coding guidelines: "Use
@frontmcp/utilsfor all file system operations. Do NOT usefs/promisesornode:fsdirectly."Also applies to: 163-168, 210-212, 265-268, 315-318, 357-358, 365-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-cache.ts` around lines 87 - 110, The file esm-cache.ts currently uses runtime require(...) and direct Node fs calls (identifiers: require('node:path'), require('node:fs'), fs.existsSync, require('node:os'), os.homedir, tmpdir) which violates the ESM/abstraction rules; replace all runtime require and direct node:fs or node:fs/promises usages (occurrences around nodeModulesDir, fs, os, tmpdir) with ESM imports and the `@frontmcp/utils` filesystem/path/os abstractions (import the utils equivalents and call their path join, exists/check, homedir/tmpdir helpers) so the module emits native ESM and routes all FS operations through `@frontmcp/utils` instead of node:fs or require.
🧹 Nitpick comments (1)
libs/testing/src/server/test-server.ts (1)
167-172: Apply same best-effort cleanup pattern as instart()for consistency.The
startNxmethod still uses plainawait server.stop()which can throw and mask the original startup error. This is the same issue that was fixed in thestart()method at lines 112-119.♻️ Proposed fix for consistency
const server = new TestServer(serverOptions, port, release); try { await server.startProcess(); } catch (error) { - await server.stop(); // Clean up spawned process to prevent leaks + try { + await server.stop(); // Clean up spawned process to prevent leaks + } catch (cleanupError) { + if (options.debug || DEBUG_SERVER) { + const msg = cleanupError instanceof Error ? cleanupError.message : String(cleanupError); + console.warn(`[TestServer] Cleanup failed after startup error: ${msg}`); + } + } throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 167 - 172, In startNx(), the catch block awaits server.stop() directly which can throw and mask the original startup error; change it to perform best-effort cleanup like start() does by calling server.stop() inside its own try/catch (e.g., try { await server.stop(); } catch (cleanupErr) { /* log or ignore cleanupErr */ }) and then rethrow the original error, referencing the startNx function and server.stop method to locate where to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/esm-loader/esm-cache.ts`:
- Line 217: The cache writes wrapCjsForEsm(bundleContent) to disk but saves the
raw bundleContent in memory, causing readBundle to sometimes return unwrapped
CJS; change the flow to normalize once into a single variable (e.g., const
normalized = wrapCjsForEsm(bundleContent)), write normalized with
writeFile(bundlePath, normalized), and store normalized in the in-memory cache
instead of raw bundleContent (apply the same change in the other affected blocks
around the 236-244 and 350-358 ranges) so disk and memory caches are consistent
and readBundle always returns the wrapped ESM content.
- Around line 197-203: In the async put method (function put) validate the
packageUrl per RFC 3986 before creating/persisting the EsmCacheEntry:
parse/validate the URI scheme and if missing or invalid throw or reject with the
exact message "URI must have a valid scheme (e.g., file://, https://,
custom://)"; ensure this validation runs at schema/metadata level in
put(packageName, version, bundleContent, packageUrl, etag?) and prevents storing
entries with invalid or scheme-less URIs.
---
Duplicate comments:
In `@libs/sdk/src/esm-loader/esm-cache.ts`:
- Around line 87-110: The file esm-cache.ts currently uses runtime require(...)
and direct Node fs calls (identifiers: require('node:path'), require('node:fs'),
fs.existsSync, require('node:os'), os.homedir, tmpdir) which violates the
ESM/abstraction rules; replace all runtime require and direct node:fs or
node:fs/promises usages (occurrences around nodeModulesDir, fs, os, tmpdir) with
ESM imports and the `@frontmcp/utils` filesystem/path/os abstractions (import the
utils equivalents and call their path join, exists/check, homedir/tmpdir
helpers) so the module emits native ESM and routes all FS operations through
`@frontmcp/utils` instead of node:fs or require.
---
Nitpick comments:
In `@libs/testing/src/server/test-server.ts`:
- Around line 167-172: In startNx(), the catch block awaits server.stop()
directly which can throw and mask the original startup error; change it to
perform best-effort cleanup like start() does by calling server.stop() inside
its own try/catch (e.g., try { await server.stop(); } catch (cleanupErr) { /*
log or ignore cleanupErr */ }) and then rethrow the original error, referencing
the startNx function and server.stop method to locate where to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d489af65-a141-4d65-99bb-5b343cfab49b
📒 Files selected for processing (2)
libs/sdk/src/esm-loader/esm-cache.tslibs/testing/src/server/test-server.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts (1)
114-115: Replace non-null assertions with explicit guards in test flow.Line 114, Line 115, and Line 144 use
!. Please keep assertions explicit and fail fast with a guard instead of non-null assertions.Suggested fix
const entry = await cache.get('@acme/tools', '1.0.0'); expect(entry).toBeDefined(); - expect(entry!.packageName).toBe('@acme/tools'); - expect(entry!.resolvedVersion).toBe('1.0.0'); + if (!entry) throw new Error('Expected cached entry'); + expect(entry.packageName).toBe('@acme/tools'); + expect(entry.resolvedVersion).toBe('1.0.0'); @@ const result = await cache.get('@acme/tools', '1.0.0'); expect(result).toBeDefined(); - expect(result!.bundleContent).toBe('code'); + if (!result) throw new Error('Expected in-memory cache hit'); + expect(result.bundleContent).toBe('code');As per coding guidelines: "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found, rather than using non-null assertion operator (!)".
Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts` around lines 114 - 115, Replace the non-null assertions on the test variable entry with an explicit guard that fails fast: check expect(entry).toBeDefined() (or if you prefer, if (!entry) throw new Error('expected cache entry to be present')) before accessing entry.packageName and entry.resolvedVersion; do the same for the other occurrence at the later assertion (previously using entry!) so the test uses a clear assertion/throw when entry is missing rather than the `!` operator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts`:
- Around line 13-27: Replace hardcoded '/' and raw startsWith() logic in the
directory-listing helper and hasEntriesUnder with platform-safe path handling:
import Node's path, normalize keys and dirPath, ensure a trailing separator
using path.sep (or compare both exact equality and separator-prefixed prefix)
and only treat a key as "under" the dir when it is exactly the dir or begins
with the dir plus the separator to avoid over-matching (fix the function that
builds entries from store.keys() and hasEntriesUnder(dirPath)). Also remove
non-null assertions used after toBeDefined() (lines flagged) by assigning the
result to a variable, use expect(...).toBeDefined(), then reference the variable
(or add an explicit runtime null/undefined check) instead of using the `!`
operator so tests handle missing values safely.
---
Nitpick comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts`:
- Around line 114-115: Replace the non-null assertions on the test variable
entry with an explicit guard that fails fast: check expect(entry).toBeDefined()
(or if you prefer, if (!entry) throw new Error('expected cache entry to be
present')) before accessing entry.packageName and entry.resolvedVersion; do the
same for the other occurrence at the later assertion (previously using entry!)
so the test uses a clear assertion/throw when entry is missing rather than the
`!` operator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d4c5023-dd36-424d-931d-7c0ef940acc2
📒 Files selected for processing (3)
libs/sdk/src/esm-loader/__tests__/esm-cache.spec.tslibs/sdk/src/esm-loader/esm-cache.tslibs/testing/src/server/test-server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/esm-loader/esm-cache.ts
…form compatibility
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts (2)
148-176:⚠️ Potential issue | 🟠 MajorTest doesn't actually pass
testCacheDirto the client configuration.The test is named "explicit cacheDir option overrides default" but
testCacheDir(created inbeforeAll) is never passed to theconnectcall. The test currently only verifies that tools load with a different namespace, which doesn't prove the cacheDir override works.🧪 Suggested fix — wire the explicit cacheDir
const customClient = await connect( { info: { name: 'ESM CLI Custom Cache', version: '0.1.0' }, - loader: { url: esmServerUrl }, + loader: { url: esmServerUrl, cacheDir: testCacheDir }, apps: [ loadFrom('@test/esm-tools@^1.0.0', { namespace: 'custom', cacheTTL: 60000, }), ], logging: { level: LogLevel.Warn }, }, { mode: 'cli' }, ); try { const tools = await customClient.listTools(); const toolNames = tools.map((t) => t.name); expect(toolNames).toContain('custom:echo'); + + // Verify cache artifacts were written to the explicit directory + const cacheHasFiles = await fileExists(testCacheDir); + expect(cacheHasFiles).toBe(true); } finally { await customClient.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` around lines 148 - 176, The test never passes the prepared testCacheDir into the SDK connect call, so update the connect invocation inside the "explicit cacheDir option overrides default" test (the connect call that returns customClient) to include the explicit cacheDir option (e.g., add cacheDir: testCacheDir) in the client options object passed as the second parameter (the same options object that currently contains mode: 'cli'); keep the rest of the call (info, loader, apps, logging) unchanged and ensure customClient.close() remains in the finally block.
126-146:⚠️ Potential issue | 🟠 MajorCache directory test does not assert cache functionality.
This test only verifies that
node_modulesexists (which is always true in the monorepo) and then assertsexpect(hasNodeModules).toBe(true)— a tautology. It doesn't verify that the ESM loader actually created cache artifacts in the expected location.To make this test meaningful, trigger an operation that populates the cache, then verify that
projectCacheDirexists or contains files.🧪 Suggested approach
it('cache directory follows environment-aware logic', async () => { const projectCacheDir = path.join(process.cwd(), 'node_modules', '.cache', 'frontmcp-esm'); const hasNodeModules = await fileExists(path.join(process.cwd(), 'node_modules')); if (hasNodeModules) { - log('[TEST] Expected project-local cache dir:', projectCacheDir); - expect(hasNodeModules).toBe(true); + // The client connected and loaded ESM tools, which should populate the cache + const cacheExists = await fileExists(projectCacheDir); + log('[TEST] Project-local cache dir exists:', cacheExists, projectCacheDir); + expect(cacheExists).toBe(true); } else { const homedirCache = path.join(os.homedir(), '.frontmcp', 'esm-cache'); log('[TEST] Expected homedir cache dir:', homedirCache); + const homedirCacheExists = await fileExists(homedirCache); + expect(homedirCacheExists).toBe(true); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` around lines 126 - 146, The test currently only asserts the tautology hasNodeModules and doesn't verify cache creation; update the test to perform an action that causes the ESM loader to write cache (e.g., import or execute a module under the loader) and then assert on the cache artifacts using projectCacheDir: after triggering the loader, call fileExists(projectCacheDir) (or fs.readdir on projectCacheDir) and replace expect(hasNodeModules).toBe(true) with an assertion that projectCacheDir exists and/or contains files; keep the homedirCache branch similar (assert homedirCache exists when hasNodeModules is false) and reference the existing symbols projectCacheDir, hasNodeModules, fileExists, and homedirCache to locate where to change the test.libs/sdk/src/esm-loader/esm-module-loader.ts (2)
116-149:⚠️ Potential issue | 🟠 MajorFallback path (line 131) shares the ESM evaluation issue.
When
importFromPath()fails andentry.bundleContentis available, the code falls back toimportBundle(). If the cached content is ESM format, this will fail with the sameSyntaxErroras the browser path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-module-loader.ts` around lines 116 - 149, The fallback path in loadFromCache currently calls importBundle(entry.bundleContent) which can throw the same ESM SyntaxError as the browser path; change loadFromCache so that when importFromPath(entry.bundlePath) fails but entry.bundleContent exists, you first detect or handle ESM evaluation failures: try importBundle(entry.bundleContent) but if it throws a SyntaxError (or if bundleContent looks like ESM by quick heuristic), write bundleContent to a temporary file and call importFromPath(tempPath) to let Node evaluate the module natively (cleaning up the temp file afterward); ensure this retry path is used for importBundle failures so loadFromCache, importFromPath, importBundle and entry.bundleContent are referenced and retried appropriately.
227-232:⚠️ Potential issue | 🔴 Critical
importBundle()cannot evaluate ESM content returned by esm.sh.The
importBundle()method usesnew Function('module', 'exports', bundleContent)which only handles CommonJS format. However, esm.sh?bundleresponses return ESM format withexportstatements (confirmed:export * from "..."). In browser mode wherebundlePathis empty, this code path receives raw ESM content and throwsSyntaxError: Unexpected token 'export'.For browser compatibility with ESM bundles from esm.sh, use a Blob URL with dynamic import instead of the Function constructor:
Proposed fix using Blob URL for ESM evaluation
private async importBundle(bundleContent: string): Promise<unknown> { + // Detect if content is ESM (has export/import statements) + const isEsm = /\bexport\s+(default\b|{|\*|async\b|const\b|class\b|function\b|let\b|var\b)/.test(bundleContent) || + /\bimport\s+/.test(bundleContent); + + if (isEsm && typeof Blob !== 'undefined' && typeof URL !== 'undefined') { + // Browser ESM: use Blob URL with dynamic import + const blob = new Blob([bundleContent], { type: 'application/javascript' }); + const blobUrl = URL.createObjectURL(blob); + try { + return await import(/* webpackIgnore: true */ blobUrl); + } finally { + URL.revokeObjectURL(blobUrl); + } + } + + // CJS fallback (or Node.js without bundlePath) const module = { exports: {} as Record<string, unknown> }; const fn = new Function('module', 'exports', bundleContent); fn(module, module.exports); return module.exports; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-module-loader.ts` around lines 227 - 232, importBundle currently uses new Function to evaluate bundleContent which only supports CommonJS and fails on ESM `export` syntax returned by esm.sh; change importBundle (the method named importBundle) to evaluate ESM by creating a Blob from bundleContent (include a sourceURL comment if helpful), create an object URL via URL.createObjectURL, perform a dynamic import(importUrl) to load the module namespace, revoke the object URL, and return the imported module namespace (or its default export if your callers expect that), falling back to the existing Function approach only if you must support non-ESM content.
🧹 Nitpick comments (6)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts (2)
68-68: Avoid non-null assertion; add a guard or throw a descriptive error.
esmServer!assumes the server started successfully. IfbeforeAllfails silently or is skipped, this will produce a confusing runtime error. Consider adding an explicit check.As per coding guidelines, "Use proper error handling instead of non-null assertions. Throw specific error types when values are not found."
♻️ Proposed fix
- const esmServerUrl = `http://127.0.0.1:${esmServer!.info.port}`; + if (!esmServer) { + throw new Error('ESM server not initialized - beforeAll may have failed'); + } + const esmServerUrl = `http://127.0.0.1:${esmServer.info.port}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` at line 68, Replace the non-null assertion on esmServer when building esmServerUrl with an explicit guard: check that esmServer (set in beforeAll) is defined and if not throw a descriptive Error (or fail the test) explaining the server did not start, then use esmServer.info.port; update references around esmServerUrl and any usages in the test file (esmServer and esmServerUrl) to rely on this guard so you no longer use the `!` assertion.
150-150: Same non-null assertion issue as above.Add a guard for
esmServerhere as well.♻️ Proposed fix
+ if (!esmServer) { + throw new Error('ESM server not initialized'); + } const esmServerUrl = `http://127.0.0.1:${esmServer!.info.port}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` at line 150, The non-null assertion on esmServer when building esmServerUrl is unsafe; add a guard that verifies esmServer is defined before using esmServer.info.port (e.g., if (!esmServer) throw or fail the test) and only construct the const esmServerUrl = `http://127.0.0.1:${esmServer.info.port}` after that check so you don't rely on a `!` assertion.apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts (1)
170-211: Consider adding a body size limit for the admin endpoint.While this is test infrastructure, unbounded body accumulation could cause issues if a malformed test sends excessive data. A simple limit would make the server more robust.
♻️ Optional: Add body size limit
function handleAdminPublish(req: http.IncomingMessage, res: http.ServerResponse): void { let body = ''; + const MAX_BODY_SIZE = 1024 * 1024; // 1MB limit for test payloads req.on('data', (chunk: Buffer) => { + if (body.length + chunk.length > MAX_BODY_SIZE) { + res.writeHead(413, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Payload too large' })); + req.destroy(); + return; + } body += chunk.toString(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts` around lines 170 - 211, In handleAdminPublish, prevent unbounded request body accumulation by enforcing a size limit (e.g., 1MB): track the cumulative byte length while handling req.on('data'), and if it exceeds the limit immediately respond with 413 Payload Too Large (res.writeHead(413, ...); res.end(...)) and destroy the request (req.destroy()) or remove listeners to stop further processing; ensure the 'end' handler only attempts JSON.parse when the size check passed and include the limit constant near handleAdminPublish for easy adjustment.libs/sdk/src/esm-loader/__tests__/esm-instance-factories.spec.ts (1)
31-34: Avoidas nevercasts; usesatisfiesfor type-safe test inputs instead.These casts bypass the type contracts the tests validate. Use
satisfieswith inferred parameter types to ensure contract drift fails at compile time:♻️ Suggested refactor
- const parsed = instance.parseInput({ + const parseInputArg = { name: 'esm:echo', arguments: { message: 'hello' }, - } as never); + } satisfies Parameters<typeof instance.parseInput>[0]; + const parsed = instance.parseInput(parseInputArg); - const ctx = instance.create(parsed, { authInfo: {} as never } as never); + const createArg = { + authInfo: {}, + } satisfies Parameters<typeof instance.create>[1]; + const ctx = instance.create(parsed, createArg);Also applies to lines 65.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-instance-factories.spec.ts` around lines 31 - 34, Tests use an unsafe "as never" cast when calling instance.parseInput which bypasses TypeScript checks; replace those casts with a type-safe test payload using the "satisfies" operator so the input object conforms to the expected parameter type for parseInput (i.e., construct the argument object and append "satisfies Parameters<typeof instance.parseInput>[0]" or otherwise infer the input type) for both occurrences (the call to instance.parseInput with name: 'esm:echo' and the similar call around line 65) so the compiler will catch contract drift instead of silencing it with as never.libs/testing/src/server/test-server.ts (1)
608-663: Consolidate repeatedrequire()calls and file system operations.The functions
findWorkspaceProtocolDir,findInstalledProtocolPackageDir, andfindWorkspaceScopeDirshare nearly identical traversal logic. Consider extracting a shared helper:♻️ Proposed refactor
+function findAncestorPath( + startDir: string, + relativeCheck: string, + existsCheck: (candidate: string) => boolean, +): string | undefined { + const fs = require('node:fs'); + const path = require('node:path'); + + let currentDir = startDir; + while (true) { + const candidate = path.join(currentDir, relativeCheck); + if (existsCheck(candidate)) { + return candidate; + } + const parentDir = path.dirname(currentDir); + if (parentDir === currentDir) { + return undefined; + } + currentDir = parentDir; + } +} + function findWorkspaceProtocolDir(startDir: string): string | undefined { - const fs = require('node:fs'); - const path = require('node:path'); - - let currentDir = startDir; - while (true) { - const candidate = path.join(currentDir, 'libs', 'protocol'); - if (fs.existsSync(path.join(candidate, 'dist', 'index.js'))) { - return candidate; - } - - const parentDir = path.dirname(currentDir); - if (parentDir === currentDir) { - return undefined; - } - currentDir = parentDir; - } + const fs = require('node:fs'); + const path = require('node:path'); + return findAncestorPath(startDir, path.join('libs', 'protocol'), (candidate) => + fs.existsSync(path.join(candidate, 'dist', 'index.js')), + ); }Also, these still need to use
@frontmcp/utilsper the coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 608 - 663, Extract the duplicated upward traversal into a single helper (e.g., findUp(startDir: string, testFn: (candidate: string) => boolean): string | undefined) and refactor findWorkspaceProtocolDir, findInstalledProtocolPackageDir, and findWorkspaceScopeDir to call it with small predicate functions that check the specific target (e.g., fs.existsSync(path.join(candidate, 'dist', 'index.js')) for findWorkspaceProtocolDir, fs.existsSync(path.join(candidate, 'package.json')) for findInstalledProtocolPackageDir, and fs.existsSync(candidate) for findWorkspaceScopeDir); also replace the inline require('node:fs') and require('node:path') with the approved imports from `@frontmcp/utils` (or use its provided fs/path helpers) and remove duplicated fs/path declarations inside each function.libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts (1)
1-6: Consider using@frontmcp/utilsfor filesystem operations in test helper.Lines 3-6 import
node:fs/promises,node:child_process, etc. for theimportWrappedModulehelper. While this is isolated test infrastructure for subprocess-based ESM import validation, the coding guidelines specify using@frontmcp/utilsfor all filesystem operations.However, since this helper spawns a separate Node process to test actual ESM imports (which is a valid testing technique), the direct usage may be justified here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts` around lines 1 - 6, The test imports native Node APIs (fs/promises, child_process.execFile, url.pathToFileURL, util.promisify) for the importWrappedModule test helper, which conflicts with the guideline to use `@frontmcp/utils` for filesystem ops; either replace filesystem calls with the `@frontmcp/utils` equivalents or, if the helper must spawn a separate Node process to validate real ESM behavior, add a short justificatory comment above the importWrappedModule helper and the native imports explaining why native APIs are required for subprocess-based ESM validation and that this is an intentional exception to the rule. Ensure the comment references importWrappedModule so reviewers can easily find the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts`:
- Around line 56-60: The rm mock (jest.fn in the test) currently deletes entries
by key.startsWith(p) which over-deletes when paths share prefixes; update the rm
mock in esm-cache.spec.ts to compare paths using a proper path containment check
(e.g., resolve both paths with path.resolve and ensure either exact equality or
that the relative path does not traverse up — rather than using startsWith) so
only the intended keys in store are removed; locate the rm mock and replace the
startsWith logic with a path-based containment/equality check that uses the
store and p variables.
In `@libs/testing/src/server/test-server.ts`:
- Around line 584-606: Replace direct node:fs/node:os usage and long hex buffer
names by using `@frontmcp/utils` filesystem and crypto helpers: use the utils FS
API instead of fs.mkdirSync/fs.existsSync/fs.symlinkSync and get temp dir from
utils (or os via utils wrapper) to create aliasRoot, scopeDir and
aliasPackageDir; replace Buffer.from(cwd).toString('hex') with a short
deterministic hash from `@frontmcp/utils` crypto (truncate to e.g. 8–12 chars) to
avoid Windows 260-char path limits, and preserve the symlink creation logic (use
junction on Windows) when creating aliasPackageDir. Ensure you update NODE_PATH
assembly to use the same path.delimiter logic and dedupe entries as before.
---
Duplicate comments:
In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts`:
- Around line 148-176: The test never passes the prepared testCacheDir into the
SDK connect call, so update the connect invocation inside the "explicit cacheDir
option overrides default" test (the connect call that returns customClient) to
include the explicit cacheDir option (e.g., add cacheDir: testCacheDir) in the
client options object passed as the second parameter (the same options object
that currently contains mode: 'cli'); keep the rest of the call (info, loader,
apps, logging) unchanged and ensure customClient.close() remains in the finally
block.
- Around line 126-146: The test currently only asserts the tautology
hasNodeModules and doesn't verify cache creation; update the test to perform an
action that causes the ESM loader to write cache (e.g., import or execute a
module under the loader) and then assert on the cache artifacts using
projectCacheDir: after triggering the loader, call fileExists(projectCacheDir)
(or fs.readdir on projectCacheDir) and replace expect(hasNodeModules).toBe(true)
with an assertion that projectCacheDir exists and/or contains files; keep the
homedirCache branch similar (assert homedirCache exists when hasNodeModules is
false) and reference the existing symbols projectCacheDir, hasNodeModules,
fileExists, and homedirCache to locate where to change the test.
In `@libs/sdk/src/esm-loader/esm-module-loader.ts`:
- Around line 116-149: The fallback path in loadFromCache currently calls
importBundle(entry.bundleContent) which can throw the same ESM SyntaxError as
the browser path; change loadFromCache so that when
importFromPath(entry.bundlePath) fails but entry.bundleContent exists, you first
detect or handle ESM evaluation failures: try importBundle(entry.bundleContent)
but if it throws a SyntaxError (or if bundleContent looks like ESM by quick
heuristic), write bundleContent to a temporary file and call
importFromPath(tempPath) to let Node evaluate the module natively (cleaning up
the temp file afterward); ensure this retry path is used for importBundle
failures so loadFromCache, importFromPath, importBundle and entry.bundleContent
are referenced and retried appropriately.
- Around line 227-232: importBundle currently uses new Function to evaluate
bundleContent which only supports CommonJS and fails on ESM `export` syntax
returned by esm.sh; change importBundle (the method named importBundle) to
evaluate ESM by creating a Blob from bundleContent (include a sourceURL comment
if helpful), create an object URL via URL.createObjectURL, perform a dynamic
import(importUrl) to load the module namespace, revoke the object URL, and
return the imported module namespace (or its default export if your callers
expect that), falling back to the existing Function approach only if you must
support non-ESM content.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts`:
- Line 68: Replace the non-null assertion on esmServer when building
esmServerUrl with an explicit guard: check that esmServer (set in beforeAll) is
defined and if not throw a descriptive Error (or fail the test) explaining the
server did not start, then use esmServer.info.port; update references around
esmServerUrl and any usages in the test file (esmServer and esmServerUrl) to
rely on this guard so you no longer use the `!` assertion.
- Line 150: The non-null assertion on esmServer when building esmServerUrl is
unsafe; add a guard that verifies esmServer is defined before using
esmServer.info.port (e.g., if (!esmServer) throw or fail the test) and only
construct the const esmServerUrl = `http://127.0.0.1:${esmServer.info.port}`
after that check so you don't rely on a `!` assertion.
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 170-211: In handleAdminPublish, prevent unbounded request body
accumulation by enforcing a size limit (e.g., 1MB): track the cumulative byte
length while handling req.on('data'), and if it exceeds the limit immediately
respond with 413 Payload Too Large (res.writeHead(413, ...); res.end(...)) and
destroy the request (req.destroy()) or remove listeners to stop further
processing; ensure the 'end' handler only attempts JSON.parse when the size
check passed and include the limit constant near handleAdminPublish for easy
adjustment.
In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts`:
- Around line 1-6: The test imports native Node APIs (fs/promises,
child_process.execFile, url.pathToFileURL, util.promisify) for the
importWrappedModule test helper, which conflicts with the guideline to use
`@frontmcp/utils` for filesystem ops; either replace filesystem calls with the
`@frontmcp/utils` equivalents or, if the helper must spawn a separate Node process
to validate real ESM behavior, add a short justificatory comment above the
importWrappedModule helper and the native imports explaining why native APIs are
required for subprocess-based ESM validation and that this is an intentional
exception to the rule. Ensure the comment references importWrappedModule so
reviewers can easily find the rationale.
In `@libs/sdk/src/esm-loader/__tests__/esm-instance-factories.spec.ts`:
- Around line 31-34: Tests use an unsafe "as never" cast when calling
instance.parseInput which bypasses TypeScript checks; replace those casts with a
type-safe test payload using the "satisfies" operator so the input object
conforms to the expected parameter type for parseInput (i.e., construct the
argument object and append "satisfies Parameters<typeof instance.parseInput>[0]"
or otherwise infer the input type) for both occurrences (the call to
instance.parseInput with name: 'esm:echo' and the similar call around line 65)
so the compiler will catch contract drift instead of silencing it with as never.
In `@libs/testing/src/server/test-server.ts`:
- Around line 608-663: Extract the duplicated upward traversal into a single
helper (e.g., findUp(startDir: string, testFn: (candidate: string) => boolean):
string | undefined) and refactor findWorkspaceProtocolDir,
findInstalledProtocolPackageDir, and findWorkspaceScopeDir to call it with small
predicate functions that check the specific target (e.g.,
fs.existsSync(path.join(candidate, 'dist', 'index.js')) for
findWorkspaceProtocolDir, fs.existsSync(path.join(candidate, 'package.json'))
for findInstalledProtocolPackageDir, and fs.existsSync(candidate) for
findWorkspaceScopeDir); also replace the inline require('node:fs') and
require('node:path') with the approved imports from `@frontmcp/utils` (or use its
provided fs/path helpers) and remove duplicated fs/path declarations inside each
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 306f7d6c-d18c-4582-ad30-79d054ff1c8e
📒 Files selected for processing (11)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.tsapps/e2e/demo-e2e-esm/e2e/esm-hot-reload.e2e.spec.tsapps/e2e/demo-e2e-esm/src/esm-package-server/main.tslibs/sdk/src/esm-loader/__tests__/esm-cache.spec.tslibs/sdk/src/esm-loader/__tests__/esm-instance-factories.spec.tslibs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.tslibs/sdk/src/esm-loader/esm-cache.tslibs/sdk/src/esm-loader/esm-module-loader.tslibs/sdk/src/tool/tool.instance.tslibs/testing/src/server/test-server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/esm-loader/tests/esm-module-loader.spec.ts
- apps/e2e/demo-e2e-esm/e2e/esm-hot-reload.e2e.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
libs/sdk/src/esm-loader/esm-module-loader.ts (1)
223-231:⚠️ Potential issue | 🔴 CriticalLoad in-memory bundles with
import(), notFunction.Line 227 evaluates fetched
?bundleoutput as CommonJS. esm.sh returns ESM with top-levelimport/export, so the browser path and the in-memory fallback inloadFromCache()fail beforenormalizeEsmExport()runs. This is not just theoretical here—AppEsmInstancecurrently createsEsmCacheManagerwithout acacheDir, so normal in-process loads can fall through this path too.Can JavaScript's `Function` constructor parse module source that contains top-level `import`/`export` statements, and is `import()` from a `data:` or `blob:` URL the supported way to evaluate ESM source text?🔧 Suggested direction
- private async importBundle(bundleContent: string): Promise<unknown> { - const module = { exports: {} as Record<string, unknown> }; - const fn = new Function('module', 'exports', bundleContent); - fn(module, module.exports); - return module.exports; - } + private async importBundle(bundleContent: string): Promise<unknown> { + const url = `data:text/javascript;charset=utf-8,${encodeURIComponent(bundleContent)}`; + return import(/* webpackIgnore: true */ url); + }
🧹 Nitpick comments (1)
libs/sdk/src/app/instances/app.esm.instance.ts (1)
159-165: Rename the private registries without_prefixes.These fields are already
private, so the underscore form just breaks the repo convention and makes this new SDK type inconsistent with the rest of the library. As per coding guidelines, "Use private keyword without underscore prefix for private fields in classes, expose via getter methods instead of exposing private fields directly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/app/instances/app.esm.instance.ts` around lines 159 - 165, The private registry fields (_tools, _resources, _prompts, _plugins, _adapters, _skills) should be renamed to remove the leading underscore (tools, resources, prompts, plugins, adapters, skills); update their declarations in class AppInstance (or app.esm.instance) as private tools: ToolRegistry, etc., then update all constructor assignments, property getters, and internal references/method calls that currently reference the underscored names to use the new names; ensure any exported types or interfaces that referenced the old field names are updated and run a quick compile to catch remaining usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/app/instances/app.esm.instance.ts`:
- Around line 181-188: The app incorrectly picks either the app-level loader or
the scope-level loader instead of merging them, causing overrides like url to
drop scope-level fields (e.g., registryUrl/token); update the logic that
constructs mergedLoader and pollerLoader to perform a shallow merge of
scopeProviders.getActiveScope().metadata.loader with
this.metadata.packageConfig?.loader (preserving scope fields when app overrides
are partial), then use that mergedLoader for deriveRegistryAuth and esmBaseUrl
(and wherever pollerLoader is built) so registryAuth and polling retain scope
credentials while allowing app-level overrides like url.
- Around line 300-309: The reload() method re-registers on top of live
registries (via registerFromManifest) causing duplicate entries, and elsewhere
code clears live registries before fully instantiating the new manifest, risking
partial/empty state; change reload()/registerFromManifest flow to perform a
staged registry swap: instantiate the new manifest into temporary
registries/structures, fully validate/instantiate all tools/resources/prompts,
then atomically replace the live registries and only after success call
poller.updateCurrentVersion(this.specifier.fullName,
this.loadResult.resolvedVersion); ensure registerFromManifest (and any code
paths around lines ~429-445) support returning the new temporary registry object
and do not mutate live registries until replacement is complete.
- Around line 452-463: The deriveRegistryAuth function currently falls back
registryUrl to loader.url which mixes the CDN (esmBaseUrl) with the registry
endpoint; update deriveRegistryAuth to stop using loader.url as a fallback for
registryUrl so registryUrl is only set from loader.registryUrl (or left
undefined) while loader.url remains the CDN/esmBaseUrl; ensure callers that need
both registry and CDN set both loader.registryUrl and loader.url explicitly
(e.g., when a self-hosted deployment serves metadata and bundles from the same
origin, populate both registryUrl and url).
---
Nitpick comments:
In `@libs/sdk/src/app/instances/app.esm.instance.ts`:
- Around line 159-165: The private registry fields (_tools, _resources,
_prompts, _plugins, _adapters, _skills) should be renamed to remove the leading
underscore (tools, resources, prompts, plugins, adapters, skills); update their
declarations in class AppInstance (or app.esm.instance) as private tools:
ToolRegistry, etc., then update all constructor assignments, property getters,
and internal references/method calls that currently reference the underscored
names to use the new names; ensure any exported types or interfaces that
referenced the old field names are updated and run a quick compile to catch
remaining usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 909106c4-7446-4272-92c6-a6c82a968021
📒 Files selected for processing (4)
libs/sdk/src/app/instances/app.esm.instance.tslibs/sdk/src/esm-loader/__tests__/esm-loader.e2e.spec.tslibs/sdk/src/esm-loader/__tests__/esm-module-loader.spec.tslibs/sdk/src/esm-loader/esm-module-loader.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
libs/testing/src/server/test-server.ts (1)
555-557:⚠️ Potential issue | 🟠 MajorReplace direct
node:fsusage with@frontmcp/utilsFS APIs.Lines 556, 589, 631, 640, and 649 import
node:fsdirectly and use sync filesystem operations. This violates the repository TS guideline and bypasses the project FS abstraction.#!/bin/bash # Verify direct node:fs/fs-promises usage in this file rg -n "require\('node:fs'\)|from 'node:fs'|from 'fs/promises'" libs/testing/src/server/test-server.tsAs per coding guidelines: "Use
@frontmcp/utilsfor all file system operations. Do NOT usefs/promisesornode:fsdirectly."Also applies to: 589-590, 631-632, 640-641, 649-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 555 - 557, The file uses direct node:fs requires and synchronous fs calls inside ensureWorkspaceProtocolLink (and the other occurrences noted) which violates the TS guideline; replace require('node:fs') and any direct sync calls (e.g., existsSync, mkdirSync, writeFileSync, copyFileSync, etc.) with the corresponding async or sync APIs exported by `@frontmcp/utils` (import the FS wrapper from `@frontmcp/utils` and call its methods instead), update ensureWorkspaceProtocolLink to use those utility functions for checking/creating directories and writing files, and remove any direct node:fs imports so all filesystem operations go through `@frontmcp/utils`.
🧹 Nitpick comments (2)
libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts (2)
276-280: ExercisereadBundle()with a cold entry.All of these cases pass the hot entry returned by
put(), soreadBundle()short-circuits onentry.bundleContentand never reachesreadFile(entry.bundlePath). That means the persisted.mjs/.cjsartifacts are not actually being read in this block.Suggested adjustment
it('reads content from disk', async () => { - const entry = await cache.put('@acme/tools', '1.0.0', 'export default 42;', 'https://esm.sh/x'); - const content = await cache.readBundle(entry); + await cache.put('@acme/tools', '1.0.0', 'export default 42;', 'https://esm.sh/x'); + const coldCache = new EsmCacheManager({ cacheDir, maxAgeMs: 60_000 }); + const entry = await coldCache.get('@acme/tools', '1.0.0'); + expect(entry).toBeDefined(); + if (!entry) throw new Error('expected cache entry'); + const content = await coldCache.readBundle(entry); expect(content).toBe('export default 42;'); });Apply the same pattern to the CJS bridge cases so the suite covers the real disk fallback instead of the in-memory wrapper path.
As per coding guidelines,
**/*.spec.{ts,tsx}: Maintain 95%+ test coverage across all metrics (statements, branches, functions, lines) in Jest tests.Also applies to: 282-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts` around lines 276 - 280, The tests currently pass a "hot" entry from cache.put() so readBundle() returns entry.bundleContent without reading disk; change the tests to exercise the disk fallback by simulating a cold entry: call cache.put() to create the persisted bundle then create a new entry object (or clone the returned entry and delete/undefined entry.bundleContent) so that readBundle(entry) must call readFile(entry.bundlePath); apply this pattern for the ES module and the CJS bridge test cases (references: cache.put, readBundle, entry.bundleContent, entry.bundlePath) to ensure the real on-disk .mjs/.cjs artifacts are read.
153-161: Add a cold-cacheget()case.This only proves the in-memory fast path:
put()seedsmemoryStore, soget()returns before it touchesmeta.jsonorbundlePath. Using a freshEsmCacheManagerinstance here would cover the disk-backed retrieval branch as well.Suggested adjustment
it('returns entry when cached and fresh', async () => { await cache.put('@acme/tools', '1.0.0', 'code', 'https://esm.sh/x'); - const entry = await cache.get('@acme/tools', '1.0.0'); + const coldCache = new EsmCacheManager({ cacheDir, maxAgeMs: 60_000 }); + const entry = await coldCache.get('@acme/tools', '1.0.0'); expect(entry).toBeDefined(); if (!entry) throw new Error('expected cache entry'); expect(entry.packageName).toBe('@acme/tools'); expect(entry.resolvedVersion).toBe('1.0.0'); + expect(entry.bundleContent).toBeUndefined(); });As per coding guidelines,
**/*.spec.{ts,tsx}: Maintain 95%+ test coverage across all metrics (statements, branches, functions, lines) in Jest tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts` around lines 153 - 161, The test currently exercises only the in-memory fast path because cache.put() seeds memoryStore; to cover the disk-backed retrieval branch, after calling cache.put('@acme/tools', '1.0.0', ...) recreate a fresh EsmCacheManager instance (or construct a new EsmCacheManager pointing at the same cache root) and call its get('@acme/tools','1.0.0') so the code reads meta.json and bundlePath from disk instead of memoryStore; update the test to assert the returned entry is defined and has packageName '@acme/tools' and resolvedVersion '1.0.0' when retrieved from the new EsmCacheManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 213-214: When republishing the same version you must derive and
store a per-version ETag from the bundle content instead of using the static
"package@version" tag: compute a content-derived hash (or revision) for
data.bundle, assign it onto pkg.versions[data.version] (e.g., add an etag or rev
property alongside bundle) and update the response ETag generation to include
that per-version value (for example "package@version:<hash>"); apply the same
change wherever pkg.versions[data.version].bundle is overwritten (also the other
block referenced around the 269-272 area) and ensure responses read
pkg.versions[data.version].etag when setting the ETag header.
- Around line 235-241: The registry currently sets time[ver] = new
Date().toISOString() on every GET which makes timestamps non-deterministic;
instead read and serve a stored publish timestamp for each version (e.g.
pkg.time or a per-version field) and only create and persist a timestamp when
the version is first published. Modify the loop that iterates
Object.keys(pkg.versions) (the code referencing versions and time) to: for each
ver, look up an existing timestamp from pkg.time[ver] or
pkg.versions[ver].publishedAt (or similar), use that value for time[ver], and if
no timestamp exists set one once and persist it back to the package metadata
during the publish/save path so subsequent GETs return a stable value.
- Around line 147-149: The call to decodeURIComponent on urlObj.pathname can
throw a URIError for malformed percent-encoding; wrap the decoding in a
try/catch around decodeURIComponent(urlObj.pathname) when computing pathname
(variable pathname derived from urlObj) and on catch respond with an HTTP 400
(bad request) and return from the request handler to avoid crashing the server;
ensure you only catch URIError (or test for it) and keep the existing base URL
construction using new URL(url, `http://127.0.0.1:${port}`) intact.
In `@libs/testing/src/server/test-server.ts`:
- Around line 600-602: The pre-check using fs.existsSync before calling
fs.symlinkSync (with aliasPackageDir and workspacePackageDir) races under
parallel startup; update the block around fs.symlinkSync to call it inside a
try/catch and swallow only the EEXIST error (ignore when err.code === 'EEXIST')
so concurrent creations don't fail, mirroring the error-handling pattern used by
ensureWorkspaceProtocolLink; do not suppress other errors—rethrow them.
---
Duplicate comments:
In `@libs/testing/src/server/test-server.ts`:
- Around line 555-557: The file uses direct node:fs requires and synchronous fs
calls inside ensureWorkspaceProtocolLink (and the other occurrences noted) which
violates the TS guideline; replace require('node:fs') and any direct sync calls
(e.g., existsSync, mkdirSync, writeFileSync, copyFileSync, etc.) with the
corresponding async or sync APIs exported by `@frontmcp/utils` (import the FS
wrapper from `@frontmcp/utils` and call its methods instead), update
ensureWorkspaceProtocolLink to use those utility functions for checking/creating
directories and writing files, and remove any direct node:fs imports so all
filesystem operations go through `@frontmcp/utils`.
---
Nitpick comments:
In `@libs/sdk/src/esm-loader/__tests__/esm-cache.spec.ts`:
- Around line 276-280: The tests currently pass a "hot" entry from cache.put()
so readBundle() returns entry.bundleContent without reading disk; change the
tests to exercise the disk fallback by simulating a cold entry: call cache.put()
to create the persisted bundle then create a new entry object (or clone the
returned entry and delete/undefined entry.bundleContent) so that
readBundle(entry) must call readFile(entry.bundlePath); apply this pattern for
the ES module and the CJS bridge test cases (references: cache.put, readBundle,
entry.bundleContent, entry.bundlePath) to ensure the real on-disk .mjs/.cjs
artifacts are read.
- Around line 153-161: The test currently exercises only the in-memory fast path
because cache.put() seeds memoryStore; to cover the disk-backed retrieval
branch, after calling cache.put('@acme/tools', '1.0.0', ...) recreate a fresh
EsmCacheManager instance (or construct a new EsmCacheManager pointing at the
same cache root) and call its get('@acme/tools','1.0.0') so the code reads
meta.json and bundlePath from disk instead of memoryStore; update the test to
assert the returned entry is defined and has packageName '@acme/tools' and
resolvedVersion '1.0.0' when retrieved from the new EsmCacheManager.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cc29b3b-3fc7-4898-bf5d-7a8faf718895
📒 Files selected for processing (6)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.tsapps/e2e/demo-e2e-esm/src/esm-package-server/main.tslibs/sdk/src/esm-loader/__tests__/esm-cache.spec.tslibs/sdk/src/esm-loader/__tests__/esm-instance-factories.spec.tslibs/sdk/src/esm-loader/esm-module-loader.tslibs/testing/src/server/test-server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sdk/src/esm-loader/tests/esm-instance-factories.spec.ts
- apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts
- libs/sdk/src/esm-loader/esm-module-loader.ts
…n request processing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/testing/src/server/test-server.ts (1)
157-177:⚠️ Potential issue | 🟠 Major
startNx()still bypasses the newEADDRINUSEretry path.
start()now retries the reserve/release/spawn race, but this Nx entrypoint still does a single reserve/start attempt and rethrows immediately. BecausestartProcess()releases the reserved port here too,startNx()can hit the same handoff race and remain flaky.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 157 - 177, The startNx() path currently does a single reservePort + startProcess attempt and rethrows on failure, bypassing the EADDRINUSE retry logic implemented in start(); update startNx() (the Nx entrypoint that constructs TestServer and calls startProcess()) to use the same retry behavior as start(): either delegate to the existing start() method or copy its reserve/release/spawn retry loop so that reservePort, server.startProcess(), and release are retried on EADDRINUSE instead of failing immediately; ensure any cleanup still calls server.stop() on non-retryable errors and that the reserved port (release) is handled the same way as in start().
♻️ Duplicate comments (1)
libs/testing/src/server/test-server.ts (1)
580-582: 🛠️ Refactor suggestion | 🟠 MajorPlease keep these helpers on the
@frontmcp/utilsfilesystem layer.The note here explains why the code dropped to
node:fs/node:os, but that still reintroduces a repo-wide guideline violation inlibs/testing. If sync tempdir/symlink support is missing today, I'd rather add it to@frontmcp/utils(or a shared wrapper there) than bypass the package in this file.Verify whether
@frontmcp/utilsalready exposes the needed FS/tempdir primitives, or whether the missing capability should be added there instead of using built-ins here:#!/bin/bash set -euo pipefail echo "== Built-in FS/OS usage in test-server.ts ==" rg -n -C2 "require\\('node:(fs|os)'\\)|from 'node:(fs|os)'" libs/testing/src/server/test-server.ts echo echo "== Candidate utils-package files mentioning FS/temp helpers ==" fd . libs -t f -e ts -e js | rg "/utils/" | xargs -r rg -n "readFile|writeFile|mkdir|readdir|stat|symlink|mkdtemp|tmpdir" echo echo "== Existing repo imports from `@frontmcp/utils` for FS/temp helpers ==" rg -n -C1 "from '@frontmcp/utils'|from 'frontmcp/utils'" libs -g '*.ts' -g '*.tsx' | rg "readFile|writeFile|mkdir|readdir|stat|symlink|mkdtemp|tmpdir" || trueAs per coding guidelines, "Use
@frontmcp/utilsfor all file system operations. Do NOT usefs/promisesornode:fsdirectly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 580 - 582, The file-level helpers in test-server.ts currently call node:fs/node:os sync APIs (e.g., existsSync, mkdirSync, symlinkSync, mkdtemp/tmpdir) which violates the repo guideline; instead, check `@frontmcp/utils` for equivalent sync/temp helpers and, if missing, add sync wrappers (or a small shared temp/symlink utility) to `@frontmcp/utils` that expose the needed primitives, then replace the direct node:fs/node:os usage in test-server.ts with imports from `@frontmcp/utils` (do not keep any require/from 'node:fs' or 'node:os' references). Ensure the helpers in test-server.ts call the newly exported functions (same names like existsSync/mkdirSync/symlinkSync or clearly named wrappers) so the file no longer imports node:fs/node:os directly.
🧹 Nitpick comments (1)
apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts (1)
312-319: Consider waiting for server.close() callback before exiting.
server.close()is asynchronous; callingprocess.exit(0)immediately may terminate before pending connections are properly closed. This could potentially cause flaky test teardowns.Suggested improvement
process.on('SIGINT', () => { - server.close(); - process.exit(0); + server.close(() => { + process.exit(0); + }); }); process.on('SIGTERM', () => { - server.close(); - process.exit(0); + server.close(() => { + process.exit(0); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts` around lines 312 - 319, The SIGINT/SIGTERM handlers call server.close() but immediately call process.exit(0), which can kill the process before connections are closed; update the handlers for process.on('SIGINT') and process.on('SIGTERM') to wait for server.close to finish (use the server.close(callback) callback or await a promisified serverClose) and only call process.exit after close completes, handling errors (log and exit non‑zero on error) and ensuring server variable is referenced (server.close -> callback -> process.exit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 193-208: In handleAdminPublish, avoid converting each Buffer chunk
to string (which can split multi-byte UTF-8 sequences); instead collect incoming
chunks (e.g., push chunk Buffers into an array and track total bodyBytes or use
Node's StringDecoder) and only decode once after the 'end' event (use
Buffer.concat(buffers, totalLength).toString() or decoder.end()) before
JSON.parse/processing; keep the existing MAX_ADMIN_BODY_BYTES check by
incrementing bodyBytes from chunk.length and aborting when exceeded, and ensure
req.on('end') handles the final decode and response logic.
In `@libs/testing/src/server/test-server.ts`:
- Around line 559-569: findWorkspaceScopeDir() can return undefined and cause
ensureWorkspaceProtocolLink() to return early; instead, when scopeDir is falsy
set a default (e.g. scopeDir = path.join(cwd, 'node_modules', '@frontmcp')) and
continue so the function can create the scope and link. Concretely: in the block
using findWorkspaceScopeDir(cwd) (and the analogous block at 653-659) replace
the early return with assigning a default scopeDir = path.join(cwd,
'node_modules', '@frontmcp'), then compute aliasPackageDir = path.join(scopeDir,
'protocol'), check fs.existsSync(aliasPackageDir) and only then call
fs.mkdirSync(scopeDir, { recursive: true }) and proceed with the rest of
ensureWorkspaceProtocolLink() logic.
---
Outside diff comments:
In `@libs/testing/src/server/test-server.ts`:
- Around line 157-177: The startNx() path currently does a single reservePort +
startProcess attempt and rethrows on failure, bypassing the EADDRINUSE retry
logic implemented in start(); update startNx() (the Nx entrypoint that
constructs TestServer and calls startProcess()) to use the same retry behavior
as start(): either delegate to the existing start() method or copy its
reserve/release/spawn retry loop so that reservePort, server.startProcess(), and
release are retried on EADDRINUSE instead of failing immediately; ensure any
cleanup still calls server.stop() on non-retryable errors and that the reserved
port (release) is handled the same way as in start().
---
Duplicate comments:
In `@libs/testing/src/server/test-server.ts`:
- Around line 580-582: The file-level helpers in test-server.ts currently call
node:fs/node:os sync APIs (e.g., existsSync, mkdirSync, symlinkSync,
mkdtemp/tmpdir) which violates the repo guideline; instead, check
`@frontmcp/utils` for equivalent sync/temp helpers and, if missing, add sync
wrappers (or a small shared temp/symlink utility) to `@frontmcp/utils` that expose
the needed primitives, then replace the direct node:fs/node:os usage in
test-server.ts with imports from `@frontmcp/utils` (do not keep any require/from
'node:fs' or 'node:os' references). Ensure the helpers in test-server.ts call
the newly exported functions (same names like existsSync/mkdirSync/symlinkSync
or clearly named wrappers) so the file no longer imports node:fs/node:os
directly.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 312-319: The SIGINT/SIGTERM handlers call server.close() but
immediately call process.exit(0), which can kill the process before connections
are closed; update the handlers for process.on('SIGINT') and
process.on('SIGTERM') to wait for server.close to finish (use the
server.close(callback) callback or await a promisified serverClose) and only
call process.exit after close completes, handling errors (log and exit non‑zero
on error) and ensuring server variable is referenced (server.close -> callback
-> process.exit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 566eb9e0-22fc-4e91-ab47-024ca4631fc7
📒 Files selected for processing (3)
apps/e2e/demo-e2e-esm/src/esm-package-server/main.tslibs/sdk/src/esm-loader/__tests__/esm-cache.spec.tslibs/testing/src/server/test-server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/esm-loader/tests/esm-cache.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
libs/testing/src/server/test-server.ts (1)
156-203: Consider extracting shared retry logic.The retry loop, cleanup handling, and EADDRINUSE detection (lines 158-203) are nearly identical to
start()(lines 105-142). Consider extracting this to a private helper method to reduce duplication and ensure consistent behavior.♻️ Suggested approach
private static async startWithRetry( project: string, options: TestServerOptions, createServer: (port: number, release: () => Promise<void>) => TestServer, ): Promise<TestServer> { const maxAttempts = 3; for (let attempt = 1; attempt <= maxAttempts; attempt++) { const { port, release } = await reservePort(project, options.port); const server = createServer(port, release); try { await server.startProcess(); return server; } catch (error) { // ... shared cleanup and retry logic } } throw new Error(`[TestServer] Failed to start after ${maxAttempts} attempts`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 156 - 203, Extract the duplicated retry/startup logic into a private helper (e.g., TestServer.startWithRetry) that accepts (project: string, options: TestServerOptions, createServer: (port: number, release: () => Promise<void>) => TestServer) and implements the shared loop: call reservePort, create server via createServer(port, release), call server.startProcess(), on failure run the same cleanup (server.stop()) and EADDRINUSE detection using error.message and server.getLogs(), retry with incremental sleep, and finally throw after maxAttempts; update both existing start(...) and the other caller to invoke this new helper and pass a lambda that constructs the TestServer instance.apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts (1)
193-247: Consider addingreq.on('error')handler to avoid hanging responses on client abort.If the client disconnects mid-request, the
endevent may never fire, leaving the response open. For a test server this is low-risk, but could cause flaky test hangs.♻️ Optional improvement
function handleAdminPublish(req: http.IncomingMessage, res: http.ServerResponse): void { const chunks: Buffer[] = []; let bodyBytes = 0; let aborted = false; + req.on('error', () => { + aborted = true; + if (!res.headersSent) { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Request error' })); + } + }); req.on('data', (chunk: Buffer) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts` around lines 193 - 247, handleAdminPublish currently listens to req 'data' and 'end' but lacks a req.on('error') (and optionally 'close') handler so a client abort can leave the response hanging; add req.on('error', (err) => { if (!aborted) { aborted = true; res.writeHead(499, {'Content-Type':'application/json'}); res.end(JSON.stringify({ error: 'Client aborted', details: String(err) })); } req.destroy(); }) (and/or handle 'close' similarly) near the top of handleAdminPublish so aborted is set and the response is closed when the request errors or disconnects, preserving existing logic around the aborted flag, MAX_ADMIN_BODY_BYTES, packages, and createVersionEntry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 302-311: The server.listen block lacks an 'error' event handler so
port-in-use or other listen errors become unhandled; add a server.on('error',
...) handler for the created server (variable server) that checks for err.code
=== 'EADDRINUSE' and logs a clear message including the port and package list
(use port and packages variables) and exits with a non-zero status, otherwise
log the error and rethrow or exit—attach this handler alongside the existing
server.listen callback that references handleRequest and packages.
- Line 18: The current port parsing using parseInt on process.env['PORT'] /
process.env['ESM_SERVER_PORT'] can yield NaN for invalid env values; validate
the parsed value for NaN and ensure a sane fallback or fail-fast before calling
server.listen. Update the code around the const port = parseInt(...) expression
to check Number.isInteger(port) and port > 0 (or use Number.isFinite and range
checks), and if invalid either use the default 50400 or throw a clear error;
ensure server.listen is only called with a validated numeric port variable.
In `@libs/testing/src/server/test-server.ts`:
- Around line 575-577: The catch block currently swallows errors silently;
update the catch to capture the thrown error (e.g., catch (err)) and when the
debug flag DEBUG_SERVER is enabled log the error (using the existing logger or
console.error) with a clear message before returning env; modify the catch in
test-server.ts around the code that returns env so it conditionally logs the
error when DEBUG_SERVER is true and preserves the existing fallback behavior.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts`:
- Around line 193-247: handleAdminPublish currently listens to req 'data' and
'end' but lacks a req.on('error') (and optionally 'close') handler so a client
abort can leave the response hanging; add req.on('error', (err) => { if
(!aborted) { aborted = true; res.writeHead(499,
{'Content-Type':'application/json'}); res.end(JSON.stringify({ error: 'Client
aborted', details: String(err) })); } req.destroy(); }) (and/or handle 'close'
similarly) near the top of handleAdminPublish so aborted is set and the response
is closed when the request errors or disconnects, preserving existing logic
around the aborted flag, MAX_ADMIN_BODY_BYTES, packages, and createVersionEntry.
In `@libs/testing/src/server/test-server.ts`:
- Around line 156-203: Extract the duplicated retry/startup logic into a private
helper (e.g., TestServer.startWithRetry) that accepts (project: string, options:
TestServerOptions, createServer: (port: number, release: () => Promise<void>) =>
TestServer) and implements the shared loop: call reservePort, create server via
createServer(port, release), call server.startProcess(), on failure run the same
cleanup (server.stop()) and EADDRINUSE detection using error.message and
server.getLogs(), retry with incremental sleep, and finally throw after
maxAttempts; update both existing start(...) and the other caller to invoke this
new helper and pass a lambda that constructs the TestServer instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd6b8d8d-dcaa-4804-b72f-7724183e51fc
📒 Files selected for processing (2)
apps/e2e/demo-e2e-esm/src/esm-package-server/main.tslibs/testing/src/server/test-server.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/frontmcp/getting-started/cli-reference.mdx`:
- Line 58: The CLI table entry is inconsistent with the examples; update the
command name in the table so it matches the actual usage shown in examples
(i.e., use the fully namespaced command string used elsewhere). Replace the
table cell value `esm-update [app-name]` with the fully-qualified command
`frontmcp package esm-update [app-name]` (or alternatively update the examples
to remove the `frontmcp package` prefix), ensuring the table entry and the
examples all use the same command namespace (`frontmcp package esm-update`) to
avoid command-not-found confusion.
In `@docs/frontmcp/sdk-reference/errors/overview.mdx`:
- Around line 135-137: The ESM Errors overview Card is missing two error types;
update the Card with title "ESM Errors" (the Card component instance) to include
EsmCacheError and EsmInvalidSpecifierError in the list alongside
EsmPackageLoadError, EsmVersionResolutionError, EsmManifestInvalidError, and
EsmRegistryAuthError so the card matches the esm-errors.mdx documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af3d0c3c-1cbe-4f7f-9b72-1e095930c1af
📒 Files selected for processing (12)
docs/docs.jsondocs/frontmcp/deployment/browser-compatibility.mdxdocs/frontmcp/features/esm-dynamic-loading.mdxdocs/frontmcp/features/multi-app-composition.mdxdocs/frontmcp/features/overview.mdxdocs/frontmcp/getting-started/cli-reference.mdxdocs/frontmcp/guides/publishing-esm-packages.mdxdocs/frontmcp/sdk-reference/errors/esm-errors.mdxdocs/frontmcp/sdk-reference/errors/overview.mdxdocs/frontmcp/servers/apps.mdxdocs/frontmcp/servers/esm-packages.mdxdocs/frontmcp/servers/server.mdx
✅ Files skipped from review due to trivial changes (4)
- docs/frontmcp/features/esm-dynamic-loading.mdx
- docs/frontmcp/sdk-reference/errors/esm-errors.mdx
- docs/frontmcp/guides/publishing-esm-packages.mdx
- docs/frontmcp/servers/esm-packages.mdx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/frontmcp/servers/apps.mdx (1)
237-237: Consider avoiding implementation-specific wording in docs.“Fetched from a CDN” may age quickly if resolver/fetch internals evolve. Prefer capability-focused wording (runtime fetch/resolve + local cache + in-process execution).
Suggested wording tweak
-Unlike [Remote Apps](`#remote-apps`) which proxy requests to external MCP servers over HTTP, ESM packages are fetched from a CDN, cached locally, and executed **in-process**. They support two-tier caching, background version polling with hot-reload, and private registry authentication. +Unlike [Remote Apps](`#remote-apps`) which proxy requests to external MCP servers over HTTP, ESM packages are resolved and fetched at runtime, cached locally, and executed **in-process**. They support two-tier caching, background version polling with hot-reload, and private registry authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/servers/apps.mdx` at line 237, The sentence in the "ESM packages" description uses implementation-specific wording ("fetched from a CDN"); update the text in docs/frontmcp/servers/apps.mdx (the ESM packages paragraph near the Remote Apps link) to use capability-focused wording such as "resolved/fetched at runtime, cached locally, and executed in-process" and keep mentions of two-tier caching, background version polling with hot-reload, and private registry authentication intact so the doc describes behavior not the current fetch implementation.libs/testing/src/server/test-server.ts (1)
156-203: Consider extracting shared retry logic.The retry-with-cleanup pattern in
startNx()duplicatesstart(). If this file grows, consider extracting a common helper:private static async withRetry<T>( createAttempt: () => Promise<{ server: TestServer; result: T }>, options: { debug?: boolean; maxAttempts?: number } ): Promise<T>This would reduce duplication and centralize retry/backoff/cleanup concerns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/testing/src/server/test-server.ts` around lines 156 - 203, Extract the duplicated retry/cleanup logic into a private static helper named withRetry(createAttempt, options) on TestServer and have both startNx and start call it; withRetry should implement the same loop from the diff (default maxAttempts 3), call createAttempt() to obtain { server, result }, attempt server.startProcess(), on failure run server.stop() in a try/catch, detect EADDRINUSE by inspecting the thrown error message or server.getLogs(), perform the same backoff (delayMs = attempt * 500) and debug logging (respecting options.debug and DEBUG_SERVER), and finally return the successful result or rethrow after attempts exhausted. Ensure createAttempt provides a fresh TestServer instance (so callers like startNx use reservePort and construct TestServer inside createAttempt) and keep helper signatures matching the suggested private static async withRetry<T>(createAttempt: () => Promise<{ server: TestServer; result: T }>, options: { debug?: boolean; maxAttempts?: number }): Promise<T>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/frontmcp/guides/publishing-esm-packages.mdx`:
- Around line 208-214: The named-exports example uses a re-export "export {
echoTool, addTool } from './tools'" but then references echoTool and addTool
locally in "export const tools = [echoTool, addTool]" which fails because
re-exports do not create local bindings; fix by importing the symbols into the
module (e.g., using an import of echoTool and addTool from './tools' or change
the export to create local bindings) so that echoTool and addTool are available
to use in the export const tools declaration.
---
Nitpick comments:
In `@docs/frontmcp/servers/apps.mdx`:
- Line 237: The sentence in the "ESM packages" description uses
implementation-specific wording ("fetched from a CDN"); update the text in
docs/frontmcp/servers/apps.mdx (the ESM packages paragraph near the Remote Apps
link) to use capability-focused wording such as "resolved/fetched at runtime,
cached locally, and executed in-process" and keep mentions of two-tier caching,
background version polling with hot-reload, and private registry authentication
intact so the doc describes behavior not the current fetch implementation.
In `@libs/testing/src/server/test-server.ts`:
- Around line 156-203: Extract the duplicated retry/cleanup logic into a private
static helper named withRetry(createAttempt, options) on TestServer and have
both startNx and start call it; withRetry should implement the same loop from
the diff (default maxAttempts 3), call createAttempt() to obtain { server,
result }, attempt server.startProcess(), on failure run server.stop() in a
try/catch, detect EADDRINUSE by inspecting the thrown error message or
server.getLogs(), perform the same backoff (delayMs = attempt * 500) and debug
logging (respecting options.debug and DEBUG_SERVER), and finally return the
successful result or rethrow after attempts exhausted. Ensure createAttempt
provides a fresh TestServer instance (so callers like startNx use reservePort
and construct TestServer inside createAttempt) and keep helper signatures
matching the suggested private static async withRetry<T>(createAttempt: () =>
Promise<{ server: TestServer; result: T }>, options: { debug?: boolean;
maxAttempts?: number }): Promise<T>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6eec83da-dfcc-4e92-8566-56483bdd9376
📒 Files selected for processing (14)
apps/e2e/demo-e2e-esm/src/esm-package-server/main.tsdocs/docs.jsondocs/frontmcp/deployment/browser-compatibility.mdxdocs/frontmcp/features/esm-dynamic-loading.mdxdocs/frontmcp/features/multi-app-composition.mdxdocs/frontmcp/features/overview.mdxdocs/frontmcp/getting-started/cli-reference.mdxdocs/frontmcp/guides/publishing-esm-packages.mdxdocs/frontmcp/sdk-reference/errors/esm-errors.mdxdocs/frontmcp/sdk-reference/errors/overview.mdxdocs/frontmcp/servers/apps.mdxdocs/frontmcp/servers/esm-packages.mdxdocs/frontmcp/servers/server.mdxlibs/testing/src/server/test-server.ts
✅ Files skipped from review due to trivial changes (1)
- docs/frontmcp/sdk-reference/errors/esm-errors.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/frontmcp/sdk-reference/errors/overview.mdx
- docs/frontmcp/getting-started/cli-reference.mdx
- docs/frontmcp/features/overview.mdx
- docs/frontmcp/features/multi-app-composition.mdx
- docs/frontmcp/servers/server.mdx
- docs/frontmcp/servers/esm-packages.mdx
- apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts
- docs/docs.json
- docs/frontmcp/deployment/browser-compatibility.mdx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts (1)
127-147:⚠️ Potential issue | 🟠 MajorCache test still doesn’t prove cache writes happened.
This block only asserts directory existence, which can pass from pre-existing state and miss real cache regressions. Assert on actual artifacts (e.g., non-empty directory) after a guaranteed cache-triggering operation.
🔧 Proposed strengthening
-import { mkdtemp, rm, fileExists } from '@frontmcp/utils'; +import { mkdtemp, rm, fileExists, readdir } from '@frontmcp/utils'; @@ it('cache directory follows environment-aware logic', async () => { @@ if (hasNodeModules) { // Prior tests loaded ESM tools which should have populated the cache const cacheExists = await fileExists(projectCacheDir); + const cacheEntries = cacheExists ? await readdir(projectCacheDir) : []; log('[TEST] Project-local cache dir:', projectCacheDir, 'exists:', cacheExists); expect(cacheExists).toBe(true); + expect(cacheEntries.length).toBeGreaterThan(0); } else { @@ const homedirCacheExists = await fileExists(homedirCache); + const cacheEntries = homedirCacheExists ? await readdir(homedirCache) : []; log('[TEST] Homedir cache dir:', homedirCache, 'exists:', homedirCacheExists); expect(homedirCacheExists).toBe(true); + expect(cacheEntries.length).toBeGreaterThan(0); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts` around lines 127 - 147, The test "cache directory follows environment-aware logic" only checks directory existence (projectCacheDir/homedirCache) which can be pre-existing; update the test to perform a deterministic cache-triggering operation (e.g., run the ESM tool under test or call the function that writes cache) and then assert on actual cache artifacts rather than just existence: use fileExists plus checks that the directory contains files and/or non-zero file sizes (e.g., list directory contents and assert length > 0 or at least one file with size > 0) for projectCacheDir and homedirCache, referencing the existing variables projectCacheDir, homedirCache and helper fileExists to locate where to add the trigger and stronger assertions.
🧹 Nitpick comments (16)
docs/frontmcp/features/overview.mdx (1)
57-57: Align API notation with the rest of the page.Use
App.esm()in the diagram label to match the callable syntax already used in the comparison table.Proposed doc tweak
- FM --> A4["App.esm: `@acme/tools`"] + FM --> A4["App.esm(): `@acme/tools`"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/features/overview.mdx` at line 57, The diagram label currently uses "App.esm: `@acme/tools`" which is inconsistent with the callable syntax used elsewhere; update the diagram node A4 to use "App.esm()" (i.e., change the label text from "App.esm: `@acme/tools`" to "App.esm()") so it matches the API notation used in the comparison table and other documentation references to App.esm.docs/frontmcp/servers/server.mdx (2)
111-112: Add explicit secret-handling guidance fortokenvstokenEnvVar.At Lines 111-112, please document precedence and recommend
tokenEnvVarover inlinetokento reduce accidental secret leakage in source/config.✏️ Suggested docs tweak
- token?: string, // Bearer token for auth - tokenEnvVar?: string, // Env var name containing bearer token + token?: string, // Bearer token for auth (avoid hardcoding in source) + tokenEnvVar?: string, // Preferred: env var name containing bearer token + // If both are set, document precedence explicitly🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/servers/server.mdx` around lines 111 - 112, Document that when both token and tokenEnvVar are provided, tokenEnvVar takes precedence and the runtime will read the bearer token from the named environment variable; explicitly recommend using tokenEnvVar (and secret managers or process env) instead of embedding token inline to avoid accidental secret leakage, and add a short best-practice note under the token/tokenEnvVar fields referencing this precedence and recommending rotation and secure storage.
107-107: Clarify token vs. tokenEnvVar precedence and recommend secure usage pattern.Lines 111–112 document
tokenandtokenEnvVaroptions for the server-levelloaderconfig, but do not specify which takes precedence if both are present, nor do they recommend secure usage (e.g., preferring environment variables over hardcoded tokens). Add guidance to this section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/servers/server.mdx` at line 107, Clarify that for the server-level loader config (the loader options used by App.esm()/server loader), when both token and tokenEnvVar are provided the loader should prefer the environment variable value (tokenEnvVar) over the hardcoded token; update the docs to state this precedence explicitly, and add a short recommendation to avoid hardcoding tokens by using tokenEnvVar (or other secret managers) as the secure pattern with token only as a last-resort fallback for local/dev use.libs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.ts (1)
76-111: Add an explicit invalid-URI regression test forPrompt.remote().Current cases validate happy-path URL handling, but this suite should also pin the RFC 3986 validation behavior and required error message.
Suggested test addition
describe('Prompt.remote()', () => { @@ it('sets url and targetName', () => { const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; expect(record.url).toBe('https://api.example.com/mcp'); expect(record.targetName).toBe('greeting'); }); + + it('throws on invalid URI scheme', () => { + expect(() => (Prompt as any).remote('api.example.com/mcp', 'greeting')).toThrow( + 'URI must have a valid scheme (e.g., file://, https://, custom://)', + ); + });As per coding guidelines:
**/*.{ts,tsx}: "Validate URIs per RFC 3986 at metadata level using schema validation with message 'URI must have a valid scheme (e.g., file://, https://, custom://)'".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.ts` around lines 76 - 111, Add a new unit test for Prompt.remote that asserts it throws a validation error when given an invalid URI; call Prompt.remote(...) with a malformed URL (e.g., missing scheme) and expect the call to throw (or the returned Promise to reject) with the exact validation message "URI must have a valid scheme (e.g., file://, https://, custom://)"; place the test alongside other Prompt.remote() specs and reference Prompt.remote and the PromptRemoteRecord creation in the assertion to pin RFC 3986 behavior and required error text.libs/sdk/src/common/utils/primitive-filter.ts (1)
73-86: Clarify thatincludepatterns are ignored in'include'default mode.In the current implementation, when
defaultMode === 'include', thematchesIncludevariable is computed but never used in the filter decision. OnlymatchesExcludematters.This is consistent with the documented behavior ("everything is included unless in
exclude"), but could surprise users who specify bothincludeandexcludepatterns withdefault: 'include'.Consider adding a brief code comment to clarify this behavior.
return items.filter((item) => { const matchesInclude = includePatterns?.length ? matchesAny(item.name, includePatterns) : false; const matchesExclude = excludePatterns?.length ? matchesAny(item.name, excludePatterns) : false; if (defaultMode === 'include') { - // Include by default, exclude wins over include + // Include by default — only exclude patterns matter; include patterns are ignored if (matchesExclude) return false; return true; } else { - // Exclude by default, include wins over exclude + // Exclude by default — only include patterns matter; exclude patterns are ignored if (matchesInclude) return true; return false; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/utils/primitive-filter.ts` around lines 73 - 86, The filter computes matchesInclude (from includePatterns and matchesAny) but never uses it when defaultMode === 'include', which can be surprising; update the items.filter block (referencing defaultMode, includePatterns, excludePatterns, matchesInclude, matchesExclude, and matchesAny) by adding a concise comment inside the defaultMode === 'include' branch stating that include patterns are intentionally ignored in this mode (behavior: everything is included unless excluded) so callers understand why matchesInclude is not consulted.libs/sdk/src/resource/__tests__/resource-npm-remote.spec.ts (1)
8-10: Consider using a typed reference instead of casting toany.The
(Resource as any).esm()pattern works but bypasses type checking. IfResource.esmandResource.remoteare part of the public API, consider either:
- Exporting a typed interface that includes these static methods
- Using a helper like
const ResourceWithStatic = Resource as typeof Resource & { esm: ...; remote: ... }This would provide better type safety and IDE support in tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/resource/__tests__/resource-npm-remote.spec.ts` around lines 8 - 10, Replace the untyped cast "(Resource as any).esm" with a typed view of the static API: declare a local helper type or const (e.g. ResourceWithStatic) as "typeof Resource & { esm(spec: string, field?: string): ResourceEsmTargetRecord; remote(spec: string, field?: string): ResourceRemoteTargetRecord }" and then call ResourceWithStatic.esm(...) (or .remote(...)) in the test; this preserves type safety and IDE hints without changing the production API.libs/sdk/src/common/records/workflow.record.ts (1)
35-51: Two record types share the sameWorkflowKind.ESMdiscriminator.Both
WorkflowEsmRecordandWorkflowEsmTargetRecordusekind: WorkflowKind.ESM. TypeScript can still discriminate them via theprovidefield type (stringvssymbol), but this is less intuitive than using distinct kind values. Consider whether a separateWorkflowKind.ESM_TARGETwould improve clarity and type narrowing ergonomics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/records/workflow.record.ts` around lines 35 - 51, Two record types both use the same discriminator WorkflowKind.ESM which makes narrowing less obvious; add a distinct discriminator value (e.g. extend the WorkflowKind enum with ESM_TARGET) and change WorkflowEsmTargetRecord.kind to WorkflowKind.ESM_TARGET, then update all places that switch/compare on WorkflowKind or construct WorkflowEsmTargetRecord (references: WorkflowEsmRecord, WorkflowEsmTargetRecord, and WorkflowKind) so they use the new enum member to restore clear discriminated-union behavior and ensure switch/case or type-guards handle the new WorkflowKind.ESM_TARGET branch.libs/sdk/src/common/metadata/app.metadata.ts (1)
257-272: Consider documenting or enforcing token/tokenEnvVar precedence.The
PackageLoaderinterface allows bothtokenandtokenEnvVarto be set simultaneously. While this may be intentional (e.g.,tokentakes precedence), it could lead to confusion. Consider either:
- Adding a JSDoc note clarifying precedence when both are set, or
- Adding a Zod refinement to enforce mutual exclusivity if that's the desired behavior
💡 Option 1: Document precedence
export interface PackageLoader { /** Base URL for the loader server (registry API + bundle fetching). * Defaults: registry → https://registry.npmjs.org, bundles → https://esm.sh */ url?: string; /** Separate registry URL for version resolution (if different from bundle URL) */ registryUrl?: string; - /** Bearer token for authentication */ + /** Bearer token for authentication. Takes precedence over tokenEnvVar if both set. */ token?: string; /** Env var name containing the bearer token */ tokenEnvVar?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/metadata/app.metadata.ts` around lines 257 - 272, The PackageLoader interface allows both token and tokenEnvVar to be set which can be ambiguous; update the JSDoc on the PackageLoader interface to state the intended precedence (e.g., "token takes precedence over tokenEnvVar when both are provided") and expected behavior, or enforce mutual exclusivity by adding a Zod refinement to the PackageLoader validation schema (e.g., packageLoaderSchema / PackageLoaderSchema) that rejects objects where both token and tokenEnvVar are present with a clear error message; locate the interface named PackageLoader and the corresponding Zod schema (or create one if none exists) and implement either the documentation change or the Zod .refine check accordingly.libs/sdk/src/common/records/job.record.ts (1)
39-55: Two record types share the sameJobKind.ESMdiscriminator.Both
JobEsmRecordandJobEsmTargetRecordusekind: JobKind.ESM. While TypeScript can still discriminate via theprovidetype (stringvssymbol), this pattern is less ergonomic for type narrowing. This mirrors the same pattern inworkflow.record.ts— consider whether a distinctJobKind.ESM_TARGETwould improve clarity across both files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/records/job.record.ts` around lines 39 - 55, The two types JobEsmRecord and JobEsmTargetRecord both use the same discriminator JobKind.ESM which hampers ergonomic type narrowing; introduce a new enum member (e.g., JobKind.ESM_TARGET) and change JobEsmTargetRecord.kind to that new value, update any switches or type guards that discriminate on JobKind (including the corresponding pattern in workflow.record.ts) to handle JobKind.ESM_TARGET, and adjust any code that constructs or checks these records to emit/check the new discriminator so narrowing is consistent across JobEsmRecord, JobEsmTargetRecord, and related workflow types.libs/sdk/src/agent/agent.instance.ts (1)
527-529: Use a typed MCP error in the default branch instead of bareError.Line 529 should map to a structured MCP error class for consistent protocol-level error handling.
As per coding guidelines, "Create specific error classes with MCP error codes (RESOURCE_NOT_FOUND: -32002, INVALID_REQUEST: -32600, METHOD_NOT_FOUND: -32601, INVALID_PARAMS: -32602, INTERNAL_ERROR: -32603, PARSE_ERROR: -32700) and implement
toJsonRpcError()method."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/agent/agent.instance.ts` around lines 527 - 529, Replace the bare Error thrown in the default switch branch (the handling of this.record) with a structured MCP error: create/use a specific error class (e.g., MethodNotFoundError or a suitable MCP error type) that carries the correct MCP error code and message for "Unknown agent kind" and implements toJsonRpcError(), then throw that typed error instead of new Error; ensure the new error class maps to the protocol codes described (e.g., METHOD_NOT_FOUND: -32601) and is used in the default branch where the unreachable _exhaustive path is asserted.libs/sdk/src/common/decorators/skill.decorator.ts (2)
144-146: Move imports to the top of the file.Imports are placed mid-file (lines 144-146) after function definitions. While this works, it's unconventional and reduces readability. Standard practice is to group all imports at the file's beginning.
♻️ Suggested restructuring
Move these imports to join the other imports at the top of the file (after line 4):
import 'reflect-metadata'; import { extendedSkillMetadata, FrontMcpSkillTokens } from '../tokens'; import { SkillMetadata, skillMetadataSchema } from '../metadata'; import { SkillKind, SkillValueRecord } from '../records'; +import type { EsmOptions, RemoteOptions } from '../metadata'; +import type { SkillEsmTargetRecord, SkillRemoteRecord } from '../records/skill.record'; +import { parsePackageSpecifier } from '../../esm-loader/package-specifier';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/skill.decorator.ts` around lines 144 - 146, The imports for EsmOptions, RemoteOptions, SkillEsmTargetRecord, SkillRemoteRecord and parsePackageSpecifier are currently located mid-file; move these import statements to the top of skill.decorator.ts with the other imports (directly after the existing top imports) so all imports are grouped together, preserving the exact named specifiers (EsmOptions, RemoteOptions, SkillEsmTargetRecord, SkillRemoteRecord, parsePackageSpecifier) and updating any relative paths if needed.
180-183: Object.assign augmentation doesn't provide TypeScript types for the static methods.Using
Object.assign(FrontMcpSkill, { esm, remote })attaches the methods at runtime but TypeScript won't recognizeSkill.esm()orSkill.remote()without additional type declarations. This forces consumers to useanycasts (as seen in tests).Consider adding a module augmentation or updating the export to include proper typings:
// Option: Intersection type export export const Skill: typeof FrontMcpSkill & { esm: typeof skillEsm; remote: typeof skillRemote; } = Object.assign(FrontMcpSkill, { esm: skillEsm, remote: skillRemote });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/skill.decorator.ts` around lines 180 - 183, The runtime Object.assign(FrontMcpSkill, { esm: skillEsm, remote: skillRemote }) attaches static methods but lacks TypeScript static typings; update the export so the exported symbol (e.g., Skill) has an intersection/augmented type that combines typeof FrontMcpSkill with the statics' types (referencing FrontMcpSkill, skillEsm, and skillRemote) so consumers can call Skill.esm() and Skill.remote() without casting—replace the raw Object.assign export with a typed declaration that assigns the runtime value but declares its type as typeof FrontMcpSkill & { esm: typeof skillEsm; remote: typeof skillRemote } (or add a module augmentation) to provide proper static method typings.libs/sdk/src/skill/__tests__/skill-npm-remote.spec.ts (1)
8-9: Consider type-safe access to static methods instead ofanycasts.Throughout this test file,
(Skill as any).esm()and(Skill as any).remote()casts are used. While this works, it bypasses type checking. Consider declaring the static method types:// At the top of the test file or in a shared test types file: type SkillWithStatic = typeof Skill & { esm: (specifier: string, targetName: string, options?: EsmOptions<SkillMetadata>) => SkillEsmTargetRecord; remote: (url: string, targetName: string, options?: RemoteOptions<SkillMetadata>) => SkillRemoteRecord; }; const TypedSkill = Skill as SkillWithStatic;This would provide compile-time safety and better IDE support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/skill/__tests__/skill-npm-remote.spec.ts` around lines 8 - 9, Replace the untyped casts `(Skill as any).esm()` and `(Skill as any).remote()` with a type-safe alias: declare a `SkillWithStatic` type that extends `typeof Skill` adding typed static signatures for `esm` and `remote` (returning `SkillEsmTargetRecord` and `SkillRemoteRecord` and accepting `EsmOptions<SkillMetadata>` / `RemoteOptions<SkillMetadata>` respectively), then create `const TypedSkill = Skill as SkillWithStatic` and use `TypedSkill.esm(...)` and `TypedSkill.remote(...)` in the tests to restore compile-time checking and IDE support while preserving the same behavior.libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.ts (1)
12-37: Consider enhancing simulate helpers to match full decorator behavior.The
simulateToolhelper setstype,name, andextendedToolMetadata, but other helpers (simulateResource,simulatePrompt,simulateSkill,simulateJob) only set thetypemetadata. IfnormalizeEsmExportrelies on additional metadata fields for these primitives, tests may pass incorrectly.For more realistic test coverage, consider adding metadata that matches what the actual decorators set:
function simulateResource(cls: { new (...args: unknown[]): unknown }, name: string) { Reflect.defineMetadata(FrontMcpResourceTokens.type, true, cls); Reflect.defineMetadata(FrontMcpResourceTokens.name, name, cls); // Add other fields as needed per the decorator implementation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.ts` around lines 12 - 37, The simulate helpers for non-tool primitives are incomplete: update simulateResource, simulatePrompt, simulateSkill, and simulateJob to set the same metadata fields the real decorators do (e.g., add name metadata like FrontMcpResourceTokens.name, FrontMcpPromptTokens.name, FrontMcpSkillTokens.name, FrontMcpJobTokens.name and any other fields the decorators attach such as metadata objects similar to extendedToolMetadata) so tests that exercise normalizeEsmExport see realistic decorator metadata; keep simulateTool as-is and mirror its pattern (type, name, and any extended metadata) when enhancing the other helper functions.libs/sdk/src/common/decorators/app.decorator.ts (2)
9-10: Remove unused imports.
RemoteTransportOptionsandRemoteAuthConfigare imported but not used in this file. These types are referenced indirectly throughRemoteUrlAppOptionsbut don't need explicit imports here.🧹 Proposed fix
import { LocalAppMetadata, frontMcpLocalAppMetadataSchema, RemoteAppMetadata, EsmAppOptions, RemoteUrlAppOptions, - RemoteTransportOptions, - RemoteAuthConfig, } from '../metadata';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/app.decorator.ts` around lines 9 - 10, Remove the unused named imports RemoteTransportOptions and RemoteAuthConfig from the import list in app.decorator.ts; keep the import that provides RemoteUrlAppOptions (which already references those types indirectly) and ensure no other references to RemoteTransportOptions or RemoteAuthConfig exist in the file (e.g., check decorators or functions like the app decorator factory) before committing the change.
89-122: Code duplication withapp-helpers.ts.This
esmAppfunction is nearly identical toapp.esm()inlibs/sdk/src/esm-loader/app-helpers.ts(lines 58-91). Consider having one implementation delegate to the other to avoid maintaining duplicate logic.♻️ Proposed approach
Import and reuse the implementation from
app-helpers.ts:+import { app } from '../../esm-loader/app-helpers'; + // Attach static methods to the decorator function Object.assign(FrontMcpApp, { - esm: esmApp, - remote: remoteApp, + esm: app.esm, + remote: app.remote, });Then remove the local
esmAppandremoteAppfunction definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/app.decorator.ts` around lines 89 - 122, The esmApp function duplicates logic from app.esm in app-helpers.ts; refactor by importing the shared implementation (e.g., the app.esm factory or a helper function) from libs/sdk/src/esm-loader/app-helpers.ts and delegate to it instead of reimplementing the logic in esmApp (and do the same for remoteApp if it mirrors app-helpers.ts), then remove the local esmApp/remoteApp definitions so callers use the imported implementation; ensure you preserve the same return shape (RemoteAppMetadata) and any option mappings (name, namespace, description, standalone, filter, packageConfig fields like loader/autoUpdate/cacheTTL/importMap).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/app/instances/esm-normalize.utils.ts`:
- Around line 108-119: In normalizeResourceFromEsmExport, the current check only
casts obj['uri'] as string; replace that with RFC 3986 scheme validation: ensure
uri is a string, parse it (e.g., new URL(uri) or an RFC3986 validator) and
verify it contains a valid scheme (protocol) such as file:, http(s): or custom
schemes; if the scheme is invalid, raise a validation error using the exact
message "URI must have a valid scheme (e.g., file://, https://, custom://)" so
invalid URIs do not get normalized and callers receive the prescribed error;
update the uri check in normalizeResourceFromEsmExport and the returned object
to use the validated uri only.
- Around line 87-94: The optional ESM export fields are being cast without
runtime validation; update the normalization in esm-normalize.utils to guard and
only assign optional fields after type checks: for the tool path (where obj is
treated as EsmToolDefinition) set description only if typeof obj['description']
=== 'string', set inputSchema/outputSchema only if typeof obj['inputSchema'] ===
'object' (and not null) and similarly for outputSchema, and for file/media
normalization validate mimeType with typeof === 'string' and arguments with
Array.isArray(obj['arguments']) before assigning; keep the same property names
(name, description, inputSchema, outputSchema, execute, mimeType, arguments) but
return undefined when checks fail so malformed exports cannot be cast into the
registry.
In `@libs/sdk/src/common/decorators/agent.decorator.ts`:
- Around line 130-135: In agentRemote, validate the incoming url before
constructing and returning the AgentRemoteRecord: ensure the URL conforms to RFC
3986 and includes a scheme (e.g., file://, https://, custom://) and if it fails
validation throw or return an error with the exact message "URI must have a
valid scheme (e.g., file://, https://, custom://)"; implement this check at the
top of the function agentRemote (which builds the record with provide:
Symbol(`remote-agent:${url}:${targetName}`), url, targetName) using a robust URI
validation approach (URL constructor or RFC3986 regex / schema validator) so
malformed or scheme-less values are rejected before the record is created.
- Around line 148-153: FrontMcpAgent has runtime-attached static properties esm
and remote via Object.assign but the exported type (FrontMcpAgent / Agent)
doesn't declare them, forcing casts to any; update the type declarations so
these statics are part of the class type: add static properties or a static
interface for esm and remote on FrontMcpAgent (matching the shapes of agentEsm
and agentRemote) or declare a FrontMcpAgentStatic and have the class implement
it so consumers can call Agent.esm() and Agent.remote() without casting;
reference FrontMcpAgent, Agent, frontMcpAgent, esm, and remote when making this
change.
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 84-99: Add URI validation in jobRemote so invalid URLs are
rejected at creation: inside the jobRemote factory (function jobRemote and the
returned JobRemoteRecord metadata), validate the url string against RFC 3986 and
throw or reject with the exact message "URI must have a valid scheme (e.g.,
file://, https://, custom://)" if the scheme is missing/invalid; implement the
check before constructing/returning the JobRemoteRecord (use a shared URI
validator or a simple parse that ensures a scheme and authority when required)
so metadata always contains a validated URL per schema rules.
In `@libs/sdk/src/common/decorators/skill.decorator.ts`:
- Around line 156-161: The metadata built by skillEsm and skillRemote omits the
required instructions field so runtime validation (skillMetadataSchema) fails;
update the metadata construction in skill.decorator.ts (where metadata: { name:
targetName, description: ..., ...options?.metadata } is created) to ensure
instructions is always present by adding instructions:
options?.metadata?.instructions ?? '' (or a sensible default string) before
casting to SkillMetadata so validation passes; keep the rest of the merge with
...options?.metadata to allow overrides.
In `@libs/sdk/src/common/decorators/tool.decorator.ts`:
- Around line 89-103: The toolRemote() factory currently accepts any url string;
add RFC3986 URI validation into the returned ToolRemoteRecord by embedding a
schema validator into the record's metadata (e.g., augment metadata.inputSchema
rather than leaving it empty) that validates the url string against RFC3986 and
produces the exact message "URI must have a valid scheme (e.g., file://,
https://, custom://)"; ensure you merge this validation into the existing
...options?.metadata so you don't overwrite other metadata fields, and have
toolRemote (and the ToolRemoteRecord it returns) reject/flag invalid URIs early.
In `@libs/sdk/src/common/utils/primitive-filter.ts`:
- Around line 21-28: The current matchPattern function builds a regex from
pattern (variable pattern in function matchPattern) which can lead to ReDoS for
patterns with many wildcards; replace the regex-based matching with a
linear-time glob matcher or enforce a maximum wildcard count. Specifically,
update matchPattern to either (a) reject patterns with more than N wildcards
(e.g., throw or return false) before constructing any regex, or (b) implement a
non-regex matcher: split pattern on '*' into segments and sequentially search
for each segment in name (respecting starts-with/ends-with when pattern doesn't
start/end with '*'), ensuring you only use O(name.length + pattern.length)
operations; keep the existing early-return cases (exact match and '*' alone) and
use the same function signature matchPattern(name: string, pattern: string):
boolean.
In `@libs/sdk/src/esm-loader/esm-manifest.ts`:
- Around line 234-280: The module misses detection and collection for decorated
Agent and Workflow classes; import isDecoratedAgentClass and
isDecoratedWorkflowClass from esm-normalize.utils.ts, update
isDecoratedPrimitive to OR in isDecoratedAgentClass(value) and
isDecoratedWorkflowClass(value), and extend collectDecoratedExports by adding
agents and workflows arrays, pushing values when isDecoratedAgentClass(value) or
isDecoratedWorkflowClass(value) return true, and include ...(agents.length ? {
agents } : {}) and ...(workflows.length ? { workflows } : {}) in the returned
FrontMcpPackageManifest.
In `@libs/sdk/src/job/job.utils.ts`:
- Around line 24-31: normalizeJob currently returns any object whose kind equals
JobKind.ESM or JobKind.REMOTE without validating required fields, which can
cause runtime errors; update normalizeJob to perform a shape check before
casting to JobRecord: implement small type-guard helpers (e.g., isValidESMRecord
/ isValidRemoteRecord) that verify presence and types of the required properties
for ESM and REMOTE records on the incoming item (use the same unique names
JobKind.ESM, JobKind.REMOTE, JobRecord, and the normalizeJob function to locate
code), and only return item as JobRecord when the guard passes; otherwise handle
the invalid shape (throw a clear error or fall back to the existing
normalization path).
In `@libs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.ts`:
- Around line 8-137: Replace all repeated "(Prompt as any)" casts in the tests
with a typed test helper (e.g., create a promptFactory that exposes esm(...) and
remote(...) strongly typed) and update all calls to use promptFactory.esm(...)
and promptFactory.remote(...) so tests keep TypeScript typing instead of any;
update uses in tests that reference PromptEsmTargetRecord/PromptRemoteRecord and
ensure provide/metadata assertions remain unchanged. Also add a new test under
Prompt.remote() that calls remote with an invalid URI (e.g., 'not-a-uri') and
asserts it throws the exact message "URI must have a valid scheme (e.g.,
file://, https://, custom://)". Ensure references to Prompt (the original class)
remain for other assertions, but all construction uses the new promptFactory
helper.
---
Duplicate comments:
In `@apps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.ts`:
- Around line 127-147: The test "cache directory follows environment-aware
logic" only checks directory existence (projectCacheDir/homedirCache) which can
be pre-existing; update the test to perform a deterministic cache-triggering
operation (e.g., run the ESM tool under test or call the function that writes
cache) and then assert on actual cache artifacts rather than just existence: use
fileExists plus checks that the directory contains files and/or non-zero file
sizes (e.g., list directory contents and assert length > 0 or at least one file
with size > 0) for projectCacheDir and homedirCache, referencing the existing
variables projectCacheDir, homedirCache and helper fileExists to locate where to
add the trigger and stronger assertions.
---
Nitpick comments:
In `@docs/frontmcp/features/overview.mdx`:
- Line 57: The diagram label currently uses "App.esm: `@acme/tools`" which is
inconsistent with the callable syntax used elsewhere; update the diagram node A4
to use "App.esm()" (i.e., change the label text from "App.esm: `@acme/tools`" to
"App.esm()") so it matches the API notation used in the comparison table and
other documentation references to App.esm.
In `@docs/frontmcp/servers/server.mdx`:
- Around line 111-112: Document that when both token and tokenEnvVar are
provided, tokenEnvVar takes precedence and the runtime will read the bearer
token from the named environment variable; explicitly recommend using
tokenEnvVar (and secret managers or process env) instead of embedding token
inline to avoid accidental secret leakage, and add a short best-practice note
under the token/tokenEnvVar fields referencing this precedence and recommending
rotation and secure storage.
- Line 107: Clarify that for the server-level loader config (the loader options
used by App.esm()/server loader), when both token and tokenEnvVar are provided
the loader should prefer the environment variable value (tokenEnvVar) over the
hardcoded token; update the docs to state this precedence explicitly, and add a
short recommendation to avoid hardcoding tokens by using tokenEnvVar (or other
secret managers) as the secure pattern with token only as a last-resort fallback
for local/dev use.
In `@libs/sdk/src/agent/agent.instance.ts`:
- Around line 527-529: Replace the bare Error thrown in the default switch
branch (the handling of this.record) with a structured MCP error: create/use a
specific error class (e.g., MethodNotFoundError or a suitable MCP error type)
that carries the correct MCP error code and message for "Unknown agent kind" and
implements toJsonRpcError(), then throw that typed error instead of new Error;
ensure the new error class maps to the protocol codes described (e.g.,
METHOD_NOT_FOUND: -32601) and is used in the default branch where the
unreachable _exhaustive path is asserted.
In `@libs/sdk/src/common/decorators/app.decorator.ts`:
- Around line 9-10: Remove the unused named imports RemoteTransportOptions and
RemoteAuthConfig from the import list in app.decorator.ts; keep the import that
provides RemoteUrlAppOptions (which already references those types indirectly)
and ensure no other references to RemoteTransportOptions or RemoteAuthConfig
exist in the file (e.g., check decorators or functions like the app decorator
factory) before committing the change.
- Around line 89-122: The esmApp function duplicates logic from app.esm in
app-helpers.ts; refactor by importing the shared implementation (e.g., the
app.esm factory or a helper function) from
libs/sdk/src/esm-loader/app-helpers.ts and delegate to it instead of
reimplementing the logic in esmApp (and do the same for remoteApp if it mirrors
app-helpers.ts), then remove the local esmApp/remoteApp definitions so callers
use the imported implementation; ensure you preserve the same return shape
(RemoteAppMetadata) and any option mappings (name, namespace, description,
standalone, filter, packageConfig fields like
loader/autoUpdate/cacheTTL/importMap).
In `@libs/sdk/src/common/decorators/skill.decorator.ts`:
- Around line 144-146: The imports for EsmOptions, RemoteOptions,
SkillEsmTargetRecord, SkillRemoteRecord and parsePackageSpecifier are currently
located mid-file; move these import statements to the top of skill.decorator.ts
with the other imports (directly after the existing top imports) so all imports
are grouped together, preserving the exact named specifiers (EsmOptions,
RemoteOptions, SkillEsmTargetRecord, SkillRemoteRecord, parsePackageSpecifier)
and updating any relative paths if needed.
- Around line 180-183: The runtime Object.assign(FrontMcpSkill, { esm: skillEsm,
remote: skillRemote }) attaches static methods but lacks TypeScript static
typings; update the export so the exported symbol (e.g., Skill) has an
intersection/augmented type that combines typeof FrontMcpSkill with the statics'
types (referencing FrontMcpSkill, skillEsm, and skillRemote) so consumers can
call Skill.esm() and Skill.remote() without casting—replace the raw
Object.assign export with a typed declaration that assigns the runtime value but
declares its type as typeof FrontMcpSkill & { esm: typeof skillEsm; remote:
typeof skillRemote } (or add a module augmentation) to provide proper static
method typings.
In `@libs/sdk/src/common/metadata/app.metadata.ts`:
- Around line 257-272: The PackageLoader interface allows both token and
tokenEnvVar to be set which can be ambiguous; update the JSDoc on the
PackageLoader interface to state the intended precedence (e.g., "token takes
precedence over tokenEnvVar when both are provided") and expected behavior, or
enforce mutual exclusivity by adding a Zod refinement to the PackageLoader
validation schema (e.g., packageLoaderSchema / PackageLoaderSchema) that rejects
objects where both token and tokenEnvVar are present with a clear error message;
locate the interface named PackageLoader and the corresponding Zod schema (or
create one if none exists) and implement either the documentation change or the
Zod .refine check accordingly.
In `@libs/sdk/src/common/records/job.record.ts`:
- Around line 39-55: The two types JobEsmRecord and JobEsmTargetRecord both use
the same discriminator JobKind.ESM which hampers ergonomic type narrowing;
introduce a new enum member (e.g., JobKind.ESM_TARGET) and change
JobEsmTargetRecord.kind to that new value, update any switches or type guards
that discriminate on JobKind (including the corresponding pattern in
workflow.record.ts) to handle JobKind.ESM_TARGET, and adjust any code that
constructs or checks these records to emit/check the new discriminator so
narrowing is consistent across JobEsmRecord, JobEsmTargetRecord, and related
workflow types.
In `@libs/sdk/src/common/records/workflow.record.ts`:
- Around line 35-51: Two record types both use the same discriminator
WorkflowKind.ESM which makes narrowing less obvious; add a distinct
discriminator value (e.g. extend the WorkflowKind enum with ESM_TARGET) and
change WorkflowEsmTargetRecord.kind to WorkflowKind.ESM_TARGET, then update all
places that switch/compare on WorkflowKind or construct WorkflowEsmTargetRecord
(references: WorkflowEsmRecord, WorkflowEsmTargetRecord, and WorkflowKind) so
they use the new enum member to restore clear discriminated-union behavior and
ensure switch/case or type-guards handle the new WorkflowKind.ESM_TARGET branch.
In `@libs/sdk/src/common/utils/primitive-filter.ts`:
- Around line 73-86: The filter computes matchesInclude (from includePatterns
and matchesAny) but never uses it when defaultMode === 'include', which can be
surprising; update the items.filter block (referencing defaultMode,
includePatterns, excludePatterns, matchesInclude, matchesExclude, and
matchesAny) by adding a concise comment inside the defaultMode === 'include'
branch stating that include patterns are intentionally ignored in this mode
(behavior: everything is included unless excluded) so callers understand why
matchesInclude is not consulted.
In `@libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.ts`:
- Around line 12-37: The simulate helpers for non-tool primitives are
incomplete: update simulateResource, simulatePrompt, simulateSkill, and
simulateJob to set the same metadata fields the real decorators do (e.g., add
name metadata like FrontMcpResourceTokens.name, FrontMcpPromptTokens.name,
FrontMcpSkillTokens.name, FrontMcpJobTokens.name and any other fields the
decorators attach such as metadata objects similar to extendedToolMetadata) so
tests that exercise normalizeEsmExport see realistic decorator metadata; keep
simulateTool as-is and mirror its pattern (type, name, and any extended
metadata) when enhancing the other helper functions.
In `@libs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.ts`:
- Around line 76-111: Add a new unit test for Prompt.remote that asserts it
throws a validation error when given an invalid URI; call Prompt.remote(...)
with a malformed URL (e.g., missing scheme) and expect the call to throw (or the
returned Promise to reject) with the exact validation message "URI must have a
valid scheme (e.g., file://, https://, custom://)"; place the test alongside
other Prompt.remote() specs and reference Prompt.remote and the
PromptRemoteRecord creation in the assertion to pin RFC 3986 behavior and
required error text.
In `@libs/sdk/src/resource/__tests__/resource-npm-remote.spec.ts`:
- Around line 8-10: Replace the untyped cast "(Resource as any).esm" with a
typed view of the static API: declare a local helper type or const (e.g.
ResourceWithStatic) as "typeof Resource & { esm(spec: string, field?: string):
ResourceEsmTargetRecord; remote(spec: string, field?: string):
ResourceRemoteTargetRecord }" and then call ResourceWithStatic.esm(...) (or
.remote(...)) in the test; this preserves type safety and IDE hints without
changing the production API.
In `@libs/sdk/src/skill/__tests__/skill-npm-remote.spec.ts`:
- Around line 8-9: Replace the untyped casts `(Skill as any).esm()` and `(Skill
as any).remote()` with a type-safe alias: declare a `SkillWithStatic` type that
extends `typeof Skill` adding typed static signatures for `esm` and `remote`
(returning `SkillEsmTargetRecord` and `SkillRemoteRecord` and accepting
`EsmOptions<SkillMetadata>` / `RemoteOptions<SkillMetadata>` respectively), then
create `const TypedSkill = Skill as SkillWithStatic` and use
`TypedSkill.esm(...)` and `TypedSkill.remote(...)` in the tests to restore
compile-time checking and IDE support while preserving the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b040a98c-eb1a-4cef-bf79-3e197882f1aa
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (67)
apps/e2e/demo-e2e-esm/browser-app/main.tsapps/e2e/demo-e2e-esm/e2e/esm-bin.e2e.spec.tsapps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/decorated-package.tsapps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/prompts-only-package.tsapps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/resources-only-package.tsapps/e2e/demo-e2e-esm/src/esm-package-server/fixtures/tools-only-package.tsapps/e2e/demo-e2e-esm/src/esm-package-server/main.tsapps/e2e/demo-e2e-esm/src/main-hot-reload.tsapps/e2e/demo-e2e-esm/src/main.tsdocs/frontmcp/deployment/browser-compatibility.mdxdocs/frontmcp/features/esm-dynamic-loading.mdxdocs/frontmcp/features/multi-app-composition.mdxdocs/frontmcp/features/overview.mdxdocs/frontmcp/guides/publishing-esm-packages.mdxdocs/frontmcp/plugins/cache-plugin.mdxdocs/frontmcp/servers/agents.mdxdocs/frontmcp/servers/apps.mdxdocs/frontmcp/servers/esm-packages.mdxdocs/frontmcp/servers/jobs.mdxdocs/frontmcp/servers/prompts.mdxdocs/frontmcp/servers/resources.mdxdocs/frontmcp/servers/server.mdxdocs/frontmcp/servers/skills.mdxdocs/frontmcp/servers/tools.mdxlibs/react/tsconfig.lib.jsonlibs/sdk/eslint.config.mjslibs/sdk/src/agent/__tests__/agent-npm-remote.spec.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/app/instances/esm-normalize.utils.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/decorators/app.decorator.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/prompt.decorator.tslibs/sdk/src/common/decorators/resource.decorator.tslibs/sdk/src/common/decorators/skill.decorator.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/common/metadata/app-filter.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/index.tslibs/sdk/src/common/metadata/remote-primitive.metadata.tslibs/sdk/src/common/records/agent.record.tslibs/sdk/src/common/records/job.record.tslibs/sdk/src/common/records/prompt.record.tslibs/sdk/src/common/records/resource.record.tslibs/sdk/src/common/records/skill.record.tslibs/sdk/src/common/records/tool.record.tslibs/sdk/src/common/records/workflow.record.tslibs/sdk/src/common/tokens/app.tokens.tslibs/sdk/src/common/utils/__tests__/primitive-filter.spec.tslibs/sdk/src/common/utils/index.tslibs/sdk/src/common/utils/primitive-filter.tslibs/sdk/src/esm-loader/__tests__/esm-class-registration.spec.tslibs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tslibs/sdk/src/esm-loader/__tests__/load-from.spec.tslibs/sdk/src/esm-loader/app-helpers.tslibs/sdk/src/esm-loader/esm-manifest.tslibs/sdk/src/esm-loader/index.tslibs/sdk/src/job/__tests__/job-npm-remote.spec.tslibs/sdk/src/job/job.utils.tslibs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.tslibs/sdk/src/prompt/prompt.utils.tslibs/sdk/src/resource/__tests__/resource-npm-remote.spec.tslibs/sdk/src/resource/resource.utils.tslibs/sdk/src/skill/__tests__/skill-npm-remote.spec.tslibs/sdk/src/skill/skill.utils.tslibs/sdk/src/tool/__tests__/tool-npm-remote.spec.tslibs/sdk/src/tool/tool.utils.ts
✅ Files skipped from review due to trivial changes (3)
- docs/frontmcp/guides/publishing-esm-packages.mdx
- docs/frontmcp/features/esm-dynamic-loading.mdx
- docs/frontmcp/servers/jobs.mdx
🚧 Files skipped from review as they are similar to previous changes (12)
- libs/sdk/src/resource/resource.utils.ts
- libs/sdk/src/prompt/prompt.utils.ts
- libs/sdk/src/skill/skill.utils.ts
- libs/sdk/src/tool/tool.utils.ts
- apps/e2e/demo-e2e-esm/browser-app/main.ts
- docs/frontmcp/servers/esm-packages.mdx
- docs/frontmcp/deployment/browser-compatibility.mdx
- libs/sdk/src/esm-loader/tests/esm-class-registration.spec.ts
- libs/sdk/src/esm-loader/tests/load-from.spec.ts
- docs/frontmcp/features/multi-app-composition.mdx
- libs/sdk/src/esm-loader/index.ts
- apps/e2e/demo-e2e-esm/src/esm-package-server/main.ts
Comment on lines
+108
to
+119
| export function normalizeResourceFromEsmExport(raw: unknown): EsmResourceDefinition | undefined { | ||
| if (!raw || typeof raw !== 'object' || isClass(raw)) return undefined; | ||
|
|
||
| const obj = raw as Record<string, unknown>; | ||
| if (typeof obj['read'] === 'function' && typeof obj['name'] === 'string' && typeof obj['uri'] === 'string') { | ||
| return { | ||
| name: obj['name'] as string, | ||
| description: obj['description'] as string | undefined, | ||
| uri: obj['uri'] as string, | ||
| mimeType: obj['mimeType'] as string | undefined, | ||
| read: obj['read'] as EsmResourceDefinition['read'], | ||
| }; |
Contributor
There was a problem hiding this comment.
Enforce RFC 3986 URI scheme validation for resource metadata.
At Line 112 and Line 116, uri is only checked/cast as string, so invalid schemes pass normalization. Please validate the URI at metadata/schema level and use the exact message: URI must have a valid scheme (e.g., file://, https://, custom://).
As per coding guidelines: "Validate URIs per RFC 3986 at metadata level using schema validation with message 'URI must have a valid scheme (e.g., file://, https://, custom://)'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/app/instances/esm-normalize.utils.ts` around lines 108 - 119, In
normalizeResourceFromEsmExport, the current check only casts obj['uri'] as
string; replace that with RFC 3986 scheme validation: ensure uri is a string,
parse it (e.g., new URL(uri) or an RFC3986 validator) and verify it contains a
valid scheme (protocol) such as file:, http(s): or custom schemes; if the scheme
is invalid, raise a validation error using the exact message "URI must have a
valid scheme (e.g., file://, https://, custom://)" so invalid URIs do not get
normalized and callers receive the prescribed error; update the uri check in
normalizeResourceFromEsmExport and the returned object to use the validated uri
only.
Comment on lines
+148
to
153
| Object.assign(FrontMcpAgent, { | ||
| esm: agentEsm, | ||
| remote: agentRemote, | ||
| }); | ||
|
|
||
| export { FrontMcpAgent, FrontMcpAgent as Agent, frontMcpAgent, frontMcpAgent as agent }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm runtime static assignment exists
rg -n "Object.assign\\(FrontMcpAgent|esm:\\s*agentEsm|remote:\\s*agentRemote" libs/sdk/src/common/decorators/agent.decorator.ts
# 2) Confirm downstream any-cast usage
rg -n "\\(Agent as any\\)\\.(esm|remote)\\(" libs/sdk/src/agent/__tests__/agent-npm-remote.spec.ts
# 3) Check whether any explicit static type declaration exists
rg -n "declare namespace FrontMcpAgent|interface FrontMcpAgent|type .*FrontMcpAgent.*(esm|remote)" libs/sdk/src/common/decorators/agent.decorator.tsRepository: agentfront/frontmcp
Length of output: 1823
🏁 Script executed:
# Get full context of the export statement and surrounding code
sed -n '140,160p' libs/sdk/src/common/decorators/agent.decorator.ts
# Check for other usages of Agent.esm or Agent.remote outside tests
rg -n "Agent\.(esm|remote)" --type ts --type tsx -g '!**/__tests__/**' -g '!**/*.spec.ts' -g '!**/*.test.ts' libs/sdk/src/
# Check the FrontMcpAgent class/function definition to understand structure
rg -n "class FrontMcpAgent|function FrontMcpAgent|const FrontMcpAgent|export.*FrontMcpAgent" libs/sdk/src/common/decorators/agent.decorator.ts -B 2 -A 2Repository: agentfront/frontmcp
Length of output: 1536
🏁 Script executed:
# Better search for non-test usages with correct syntax
rg "Agent\.(esm|remote)" libs/sdk/src --type ts -g '!**/__tests__/**' -g '!**/*.spec.ts' -g '!**/*.test.ts'
# Get the full FrontMcpAgent function signature and any overloads
sed -n '1,100p' libs/sdk/src/common/decorators/agent.decorator.ts | grep -A 30 "function FrontMcpAgent"
# Check if there's an interface or namespace declaration
rg -n "interface.*FrontMcpAgent|namespace.*FrontMcpAgent" libs/sdk/src/common/decorators/agent.decorator.tsRepository: agentfront/frontmcp
Length of output: 1344
🏁 Script executed:
# Check the full file structure - imports, exports, overall
wc -l libs/sdk/src/common/decorators/agent.decorator.ts
# Look at the section before Object.assign to understand context
sed -n '100,155p' libs/sdk/src/common/decorators/agent.decorator.ts
# Check what agentEsm and agentRemote are
rg -n "function agentEsm|const agentEsm|function agentRemote|const agentRemote" libs/sdk/src/common/decorators/agent.decorator.ts -A 1Repository: agentfront/frontmcp
Length of output: 2396
🏁 Script executed:
# Check what FrontMcpAgent function looks like (get full definition)
sed -n '23,75p' libs/sdk/src/common/decorators/agent.decorator.ts
# Look for imports of Agent or FrontMcpAgent in the codebase (outside tests)
rg "import.*Agent.*from|import.*FrontMcpAgent" libs/sdk/src --type ts -g '!**/__tests__/**' -g '!**/*.spec.ts'
# Check index.ts or barrel exports
rg "export.*Agent|export.*FrontMcpAgent" libs/sdk/src/index.ts 2>/dev/null || echo "No match in index"Repository: agentfront/frontmcp
Length of output: 5688
Expose runtime-attached static properties in type system.
Agent.esm and Agent.remote are attached to FrontMcpAgent at runtime via Object.assign but are not exposed in the exported type. This forces downstream code to use (Agent as any).esm() and (Agent as any).remote() (17+ occurrences in tests), violating the strict typing guideline.
Proposed fix
+type FrontMcpAgentStatic = typeof FrontMcpAgent & {
+ esm: typeof agentEsm;
+ remote: typeof agentRemote;
+};
+
-Object.assign(FrontMcpAgent, {
+const FrontMcpAgentWithStatics = Object.assign(FrontMcpAgent, {
esm: agentEsm,
remote: agentRemote,
-});
+}) as FrontMcpAgentStatic;
-export { FrontMcpAgent, FrontMcpAgent as Agent, frontMcpAgent, frontMcpAgent as agent };
+export {
+ FrontMcpAgentWithStatics as FrontMcpAgent,
+ FrontMcpAgentWithStatics as Agent,
+ frontMcpAgent,
+ frontMcpAgent as agent,
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Object.assign(FrontMcpAgent, { | |
| esm: agentEsm, | |
| remote: agentRemote, | |
| }); | |
| export { FrontMcpAgent, FrontMcpAgent as Agent, frontMcpAgent, frontMcpAgent as agent }; | |
| type FrontMcpAgentStatic = typeof FrontMcpAgent & { | |
| esm: typeof agentEsm; | |
| remote: typeof agentRemote; | |
| }; | |
| const FrontMcpAgentWithStatics = Object.assign(FrontMcpAgent, { | |
| esm: agentEsm, | |
| remote: agentRemote, | |
| }) as FrontMcpAgentStatic; | |
| export { | |
| FrontMcpAgentWithStatics as FrontMcpAgent, | |
| FrontMcpAgentWithStatics as Agent, | |
| frontMcpAgent, | |
| frontMcpAgent as agent, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/common/decorators/agent.decorator.ts` around lines 148 - 153,
FrontMcpAgent has runtime-attached static properties esm and remote via
Object.assign but the exported type (FrontMcpAgent / Agent) doesn't declare
them, forcing casts to any; update the type declarations so these statics are
part of the class type: add static properties or a static interface for esm and
remote on FrontMcpAgent (matching the shapes of agentEsm and agentRemote) or
declare a FrontMcpAgentStatic and have the class implement it so consumers can
call Agent.esm() and Agent.remote() without casting; reference FrontMcpAgent,
Agent, frontMcpAgent, esm, and remote when making this change.
Comment on lines
+8
to
+137
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| expect(record.kind).toBe(PromptKind.ESM); | ||
| }); | ||
|
|
||
| it('parses scoped specifier correctly', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| expect(record.specifier.scope).toBe('@acme'); | ||
| expect(record.specifier.name).toBe('tools'); | ||
| expect(record.specifier.fullName).toBe('@acme/tools'); | ||
| expect(record.specifier.range).toBe('^1.0.0'); | ||
| }); | ||
|
|
||
| it('parses unscoped specifier', () => { | ||
| const record = (Prompt as any).esm('prompts-lib@2.0.0', 'welcome') as PromptEsmTargetRecord; | ||
| expect(record.specifier.scope).toBeUndefined(); | ||
| expect(record.specifier.fullName).toBe('prompts-lib'); | ||
| expect(record.specifier.range).toBe('2.0.0'); | ||
| }); | ||
|
|
||
| it('sets targetName', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| expect(record.targetName).toBe('greeting'); | ||
| }); | ||
|
|
||
| it('creates unique symbol provide token', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| expect(typeof record.provide).toBe('symbol'); | ||
| expect(record.provide.toString()).toContain('esm-prompt:@acme/tools:greeting'); | ||
| }); | ||
|
|
||
| it('creates different symbols for different targets', () => { | ||
| const r1 = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| const r2 = (Prompt as any).esm('@acme/tools@^1.0.0', 'farewell') as PromptEsmTargetRecord; | ||
| expect(r1.provide).not.toBe(r2.provide); | ||
| }); | ||
|
|
||
| it('passes options through', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting', { | ||
| cacheTTL: 120000, | ||
| }) as PromptEsmTargetRecord; | ||
| expect(record.options?.cacheTTL).toBe(120000); | ||
| }); | ||
|
|
||
| it('generates placeholder metadata with arguments array', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| expect(record.metadata.name).toBe('greeting'); | ||
| expect(record.metadata.description).toContain('greeting'); | ||
| expect(record.metadata.arguments).toEqual([]); | ||
| }); | ||
|
|
||
| it('allows overriding metadata via options', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting', { | ||
| metadata: { description: 'Custom greeting prompt' }, | ||
| }) as PromptEsmTargetRecord; | ||
| expect(record.metadata.description).toBe('Custom greeting prompt'); | ||
| expect(record.metadata.name).toBe('greeting'); | ||
| expect(record.metadata.arguments).toEqual([]); | ||
| }); | ||
|
|
||
| it('throws on empty specifier', () => { | ||
| expect(() => (Prompt as any).esm('', 'greeting')).toThrow('Package specifier cannot be empty'); | ||
| }); | ||
|
|
||
| it('throws on invalid specifier', () => { | ||
| expect(() => (Prompt as any).esm('!!!', 'greeting')).toThrow('Invalid package specifier'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Prompt.remote()', () => { | ||
| it('creates PromptRemoteRecord with kind REMOTE', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; | ||
| expect(record.kind).toBe(PromptKind.REMOTE); | ||
| }); | ||
|
|
||
| it('sets url and targetName', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; | ||
| expect(record.url).toBe('https://api.example.com/mcp'); | ||
| expect(record.targetName).toBe('greeting'); | ||
| }); | ||
|
|
||
| it('creates unique symbol provide token', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; | ||
| expect(typeof record.provide).toBe('symbol'); | ||
| expect(record.provide.toString()).toContain('remote-prompt:https://api.example.com/mcp:greeting'); | ||
| }); | ||
|
|
||
| it('passes transportOptions and remoteAuth', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting', { | ||
| transportOptions: { timeout: 15000 }, | ||
| remoteAuth: { mode: 'static', credentials: { type: 'bearer', value: 'abc' } }, | ||
| }) as PromptRemoteRecord; | ||
| expect(record.transportOptions).toEqual({ timeout: 15000 }); | ||
| expect(record.remoteAuth).toEqual({ | ||
| mode: 'static', | ||
| credentials: { type: 'bearer', value: 'abc' }, | ||
| }); | ||
| }); | ||
|
|
||
| it('generates placeholder metadata with arguments array', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; | ||
| expect(record.metadata.name).toBe('greeting'); | ||
| expect(record.metadata.arguments).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('normalizePrompt() with ESM/REMOTE records', () => { | ||
| it('passes through PromptEsmTargetRecord unchanged', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| const normalized = normalizePrompt(record); | ||
| expect(normalized).toBe(record); | ||
| expect(normalized.kind).toBe(PromptKind.ESM); | ||
| }); | ||
|
|
||
| it('passes through PromptRemoteRecord unchanged', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; | ||
| const normalized = normalizePrompt(record); | ||
| expect(normalized).toBe(record); | ||
| expect(normalized.kind).toBe(PromptKind.REMOTE); | ||
| }); | ||
| }); | ||
|
|
||
| describe('promptDiscoveryDeps() with ESM/REMOTE records', () => { | ||
| it('returns empty array for ESM record', () => { | ||
| const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord; | ||
| expect(promptDiscoveryDeps(record)).toEqual([]); | ||
| }); | ||
|
|
||
| it('returns empty array for REMOTE record', () => { | ||
| const record = (Prompt as any).remote('https://api.example.com/mcp', 'greeting') as PromptRemoteRecord; | ||
| expect(promptDiscoveryDeps(record)).toEqual([]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit any-casts are removed from this test file.
rg -nP '\bas\s+any\b' libs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.tsRepository: agentfront/frontmcp
Length of output: 2189
Replace repeated (Prompt as any) casts with a typed test helper and add missing invalid-URI test.
The 20+ instances of (Prompt as any) bypass TypeScript's type system and violate the strict typing guideline. The proposed refactor using a typed helper centralizes the cast and maintains type safety.
Additionally, add a test case to Prompt.remote() for invalid URIs to ensure the URI validation error message matches the guideline requirement.
Refactor for any casts
import { Prompt } from '../../common/decorators/prompt.decorator';
import { PromptKind } from '../../common/records/prompt.record';
import type { PromptEsmTargetRecord, PromptRemoteRecord } from '../../common/records/prompt.record';
import { normalizePrompt, promptDiscoveryDeps } from '../prompt.utils';
+const promptFactory = Prompt as unknown as {
+ esm: (specifier: string, targetName: string, options?: unknown) => PromptEsmTargetRecord;
+ remote: (url: string, targetName: string, options?: unknown) => PromptRemoteRecord;
+};
+
describe('Prompt.esm()', () => {
it('creates PromptEsmTargetRecord with kind ESM', () => {
- const record = (Prompt as any).esm('@acme/tools@^1.0.0', 'greeting') as PromptEsmTargetRecord;
+ const record = promptFactory.esm('@acme/tools@^1.0.0', 'greeting');
expect(record.kind).toBe(PromptKind.ESM);
});Replace all (Prompt as any).esm(...) and (Prompt as any).remote(...) calls with promptFactory.esm(...) and promptFactory.remote(...) respectively.
For the invalid URI test, add:
it('throws on invalid URI', () => {
expect(() => (Prompt as any).remote('not-a-uri', 'greeting')).toThrow('URI must have a valid scheme (e.g., file://, https://, custom://)');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.ts` around lines 8 -
137, Replace all repeated "(Prompt as any)" casts in the tests with a typed test
helper (e.g., create a promptFactory that exposes esm(...) and remote(...)
strongly typed) and update all calls to use promptFactory.esm(...) and
promptFactory.remote(...) so tests keep TypeScript typing instead of any; update
uses in tests that reference PromptEsmTargetRecord/PromptRemoteRecord and ensure
provide/metadata assertions remain unchanged. Also add a new test under
Prompt.remote() that calls remote with an invalid URI (e.g., 'not-a-uri') and
asserts it throws the exact message "URI must have a valid scheme (e.g.,
file://, https://, custom://)". Ensure references to Prompt (the original class)
remain for other assertions, but all construction uses the new promptFactory
helper.
…tors for Agent and Workflow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
libs/sdk/src/common/decorators/agent.decorator.ts (1)
150-155:⚠️ Potential issue | 🟠 MajorExpose
Agent.esm()andAgent.remote()in the exported type surface.Line 150 attaches statics at runtime, but Line 155 exports
Agentwithout a static-aware type. This keeps downstream usage dependent on casts like(Agent as any).esm(...).🔧 Proposed fix
+type AgentDecorator = { + <InSchema extends ToolInputType = ToolInputType, OutSchema extends ToolOutputType = ToolOutputType>( + providedMetadata: AgentMetadata<InSchema, OutSchema>, + ): ClassDecorator; + esm(specifier: string, targetName: string, options?: EsmOptions<AgentMetadata>): AgentEsmTargetRecord; + remote(url: string, targetName: string, options?: RemoteOptions<AgentMetadata>): AgentRemoteRecord; +}; + +const Agent = FrontMcpAgent as unknown as AgentDecorator; -export { FrontMcpAgent, FrontMcpAgent as Agent, frontMcpAgent, frontMcpAgent as agent }; +export { FrontMcpAgent, Agent, frontMcpAgent, frontMcpAgent as agent };#!/bin/bash set -euo pipefail # Confirm runtime static assignment rg -n "Object.assign\\(FrontMcpAgent|esm:\\s*agentEsm|remote:\\s*agentRemote" libs/sdk/src/common/decorators/agent.decorator.ts -C2 # Detect downstream any-casts that indicate missing static typing rg -n "\\(Agent as any\\)\\.(esm|remote)\\(" libs/sdk/src --type ts -g '!**/dist/**'As per coding guidelines, "Use strict TypeScript type checking with no
anytypes without justification; useunknowninstead ofanyfor generic type defaults".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/decorators/agent.decorator.ts` around lines 150 - 155, The exported Agent type lacks the static typings for the runtime-assigned statics (agentEsm, agentRemote) so consumers must cast to any; update the type surface so Agent/FrontMcpAgent includes those statics: add a static-aware export signature (or declaration-merge) that exposes esm: typeof agentEsm and remote: typeof agentRemote alongside the existing FrontMcpAgent instance type; adjust the final export (FrontMcpAgent, FrontMcpAgent as Agent, frontMcpAgent, frontMcpAgent as agent) to use the augmented type so callers can call Agent.esm(...) and Agent.remote(...) without any-casts, referencing the symbols FrontMcpAgent, Agent, agentEsm, agentRemote, and frontMcpAgent when making the change.
🧹 Nitpick comments (3)
docs/frontmcp/features/overview.mdx (1)
96-96: Align table terminology for non-decorator APIs.Line 96 uses
App.esm()under the Key Decorator column. Since this is an API method, a neutral column label (e.g., “Key API”) will avoid ambiguity.Suggested doc tweak
-| Feature | What It Does | Key Decorator | Docs | +| Feature | What It Does | Key API | Docs |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/features/overview.mdx` at line 96, The table row for ESM Packages incorrectly places the API method App.esm() under a "Key Decorator" column; update the column label to a neutral term like "Key API" (or "Key Method") and ensure App.esm() remains listed under that column so non-decorator APIs are correctly categorized; make the same label change wherever the table header uses "Key Decorator" for API methods to keep terminology consistent.libs/sdk/src/esm-loader/app-helpers.ts (1)
57-89: Consider adding error handling forparsePackageSpecifier.If
parsePackageSpecifierthrows on an invalid specifier, the error may surface with an unclear stack trace. For a public SDK library, wrapping this in a try-catch with a descriptive error message would improve the developer experience.💡 Proposed improvement
esm(specifier: string, options?: EsmAppOptions): RemoteAppMetadata { - const parsed = parsePackageSpecifier(specifier); + let parsed; + try { + parsed = parsePackageSpecifier(specifier); + } catch (error) { + throw new Error( + `Invalid npm package specifier "${specifier}": ${error instanceof Error ? error.message : String(error)}` + ); + } const packageConfig: RemoteAppMetadata['packageConfig'] = {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/app-helpers.ts` around lines 57 - 89, Wrap the call to parsePackageSpecifier inside the esm function in a try-catch to surface a clearer, contextual error for invalid specifiers: catch any thrown error from parsePackageSpecifier(specifier) and rethrow a new Error (or throw an error type consistent with the library) that includes the original specifier and the original error message/stack to aid debugging while preserving the original error as cause if supported; ensure the rest of esm (building packageConfig and returning the RemoteAppMetadata) remains unchanged and reference the esm function and parsePackageSpecifier to locate the change.libs/sdk/src/esm-loader/esm-manifest.ts (1)
107-108: Prefer typed SDK error classes over bareErrorin manifest normalization.Consider replacing generic
Errorthrows with a dedicated manifest-normalization error class carrying MCP-compatible error codes for consistent upstream handling.As per coding guidelines, "Create specific error classes with MCP error codes (RESOURCE_NOT_FOUND: -32002, INVALID_REQUEST: -32600, METHOD_NOT_FOUND: -32601, INVALID_PARAMS: -32602, INTERNAL_ERROR: -32603, PARSE_ERROR: -32700) and implement
toJsonRpcError()method".Also applies to: 157-161, 197-198, 298-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-manifest.ts` around lines 107 - 108, Replace the bare Error throws in the manifest normalization logic with a dedicated manifest error class (e.g., ManifestNormalizationError or SdkManifestError) that accepts an MCP/JSON-RPC compatible error code and message and implements toJsonRpcError(); update the throw sites (the current string throws in the ESM manifest normalization function and the other occurrences you indicated) to throw this class with the appropriate MCP code (use INVALID_PARAMS: -32602 for malformed exports/params, RESOURCE_NOT_FOUND: -32002 where a resource is missing, etc. per guideline), and ensure callers that catch these errors call toJsonRpcError() when converting to upstream JSON-RPC responses so the error is consistently encoded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/app.decorator.ts`:
- Around line 128-149: In remoteApp(url: string, options?: RemoteUrlAppOptions):
RemoteAppMetadata validate the provided url using RFC 3986 rules and reject
scheme-less or otherwise invalid URIs before returning metadata; specifically,
run a URI/schema validation (e.g., via URL constructor or a schema validator)
and throw an error with the exact message "URI must have a valid scheme (e.g.,
file://, https://, custom://)" when validation fails so callers get the
canonical validation message; keep the existing derivedName logic only for valid
URLs and ensure RemoteAppMetadata is not returned for invalid input.
- Around line 112-113: The metadata in esmApp() is using the wrong urlType:
update the object created in esmApp() (the one that sets url and urlType when
building the decorator metadata after calling parsePackageSpecifier()) to use
urlType: 'npm' instead of 'esm' so the package specifier is marked as an NPM
package loaded via esm.sh; ensure the field remains paired with url: specifier
and that parsePackageSpecifier() usage and JSDoc remain consistent.
In `@libs/sdk/src/common/decorators/prompt.decorator.ts`:
- Around line 101-106: Add a typed export for the decorator so TypeScript sees
the static helpers: declare a PromptDecorator type/ interface that extends
typeof FrontMcpPrompt and adds esm: typeof promptEsm and remote: typeof
promptRemote, then export FrontMcpPrompt under that type (e.g. export const
Prompt: PromptDecorator = FrontMcpPrompt as unknown as PromptDecorator) and
mirror the same typed alias for prompt/frontMcpPrompt so consumers can call
Prompt.esm() and Prompt.remote() with proper typings.
In `@libs/sdk/src/common/utils/validate-remote-url.ts`:
- Around line 16-17: In validate-remote-url.ts inside the isValidMcpUri() check,
remove the raw URL interpolation from the thrown error (do not log the full URL)
and replace it with the canonical schema-level message used in the project
(e.g., "Invalid remote URL: URI must have a valid scheme (e.g., file://,
https://, custom://)"); preferably reuse the same Zod schema/validator used by
resource.metadata.ts (the schema referenced at its line 62) or ensure the thrown
Error message exactly matches that canonical message so sensitive data is not
leaked.
In `@libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.ts`:
- Line 143: The test uses non-null assertions (e.g., result.tools![0]) which
bypass TypeScript null checks; change each such expectation to first assert the
container exists with toBeDefined(), then access the value via optional chaining
(e.g., expect(result.tools).toBeDefined();
expect(result.tools?.[0]).toBe(EchoTool)); update all similar assertions (the
ones referencing result.tools, result.runners, result.parsers, etc.) to follow
this pattern so the test explicitly checks presence before accessing elements.
In `@libs/sdk/src/esm-loader/esm-manifest.ts`:
- Around line 217-230: collectNamedExports (and collectDecoratedExports)
currently uses unchecked casts like (mod['name'] as string) and (mod['tools'] as
unknown[]), which lets non-string/non-array truthy values bypass type
validation; replace these casts with proper runtime checks and normalize values
(e.g., ensure typeof name === 'string' else use fallback, ensure
Array.isArray(tools) else undefined) and then pass the constructed object
through validateManifest so the Zod schema enforces correct types before
returning a FrontMcpPackageManifest.
---
Duplicate comments:
In `@libs/sdk/src/common/decorators/agent.decorator.ts`:
- Around line 150-155: The exported Agent type lacks the static typings for the
runtime-assigned statics (agentEsm, agentRemote) so consumers must cast to any;
update the type surface so Agent/FrontMcpAgent includes those statics: add a
static-aware export signature (or declaration-merge) that exposes esm: typeof
agentEsm and remote: typeof agentRemote alongside the existing FrontMcpAgent
instance type; adjust the final export (FrontMcpAgent, FrontMcpAgent as Agent,
frontMcpAgent, frontMcpAgent as agent) to use the augmented type so callers can
call Agent.esm(...) and Agent.remote(...) without any-casts, referencing the
symbols FrontMcpAgent, Agent, agentEsm, agentRemote, and frontMcpAgent when
making the change.
---
Nitpick comments:
In `@docs/frontmcp/features/overview.mdx`:
- Line 96: The table row for ESM Packages incorrectly places the API method
App.esm() under a "Key Decorator" column; update the column label to a neutral
term like "Key API" (or "Key Method") and ensure App.esm() remains listed under
that column so non-decorator APIs are correctly categorized; make the same label
change wherever the table header uses "Key Decorator" for API methods to keep
terminology consistent.
In `@libs/sdk/src/esm-loader/app-helpers.ts`:
- Around line 57-89: Wrap the call to parsePackageSpecifier inside the esm
function in a try-catch to surface a clearer, contextual error for invalid
specifiers: catch any thrown error from parsePackageSpecifier(specifier) and
rethrow a new Error (or throw an error type consistent with the library) that
includes the original specifier and the original error message/stack to aid
debugging while preserving the original error as cause if supported; ensure the
rest of esm (building packageConfig and returning the RemoteAppMetadata) remains
unchanged and reference the esm function and parsePackageSpecifier to locate the
change.
In `@libs/sdk/src/esm-loader/esm-manifest.ts`:
- Around line 107-108: Replace the bare Error throws in the manifest
normalization logic with a dedicated manifest error class (e.g.,
ManifestNormalizationError or SdkManifestError) that accepts an MCP/JSON-RPC
compatible error code and message and implements toJsonRpcError(); update the
throw sites (the current string throws in the ESM manifest normalization
function and the other occurrences you indicated) to throw this class with the
appropriate MCP code (use INVALID_PARAMS: -32602 for malformed exports/params,
RESOURCE_NOT_FOUND: -32002 where a resource is missing, etc. per guideline), and
ensure callers that catch these errors call toJsonRpcError() when converting to
upstream JSON-RPC responses so the error is consistently encoded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfc50082-505f-4e09-9077-307f22c81937
📒 Files selected for processing (16)
docs/frontmcp/features/overview.mdxdocs/frontmcp/servers/server.mdxlibs/sdk/src/app/instances/esm-normalize.utils.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/decorators/app.decorator.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/prompt.decorator.tslibs/sdk/src/common/decorators/resource.decorator.tslibs/sdk/src/common/decorators/skill.decorator.tslibs/sdk/src/common/decorators/tool.decorator.tslibs/sdk/src/common/utils/index.tslibs/sdk/src/common/utils/primitive-filter.tslibs/sdk/src/common/utils/validate-remote-url.tslibs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tslibs/sdk/src/esm-loader/app-helpers.tslibs/sdk/src/esm-loader/esm-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/sdk/src/common/utils/index.ts
- libs/sdk/src/common/utils/primitive-filter.ts
- libs/sdk/src/common/decorators/resource.decorator.ts
- libs/sdk/src/common/decorators/tool.decorator.ts
- libs/sdk/src/app/instances/esm-normalize.utils.ts
Comment on lines
+112
to
+113
| urlType: 'esm', | ||
| url: specifier, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect metadata contract and runtime branching for urlType values
rg -n "urlType\\s*:\\s*'worker'\\s*\\|\\s*'url'\\s*\\|\\s*'npm'\\s*\\|\\s*'esm'" libs/sdk/src/common/metadata/app.metadata.ts -C3
rg -n "case\\s*'npm'|case\\s*'esm'|urlType\\s*===\\s*'npm'|urlType\\s*===\\s*'esm'" libs/sdk/src -C3
# Inspect App.esm construction site
rg -n "function esmApp\\(|urlType:\\s*'esm'|parsePackageSpecifier\\(" libs/sdk/src/common/decorators/app.decorator.ts -C3Repository: agentfront/frontmcp
Length of output: 3406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for npmApp function or related functions
rg -n "function npmApp|export.*npmApp|npmApp\s*=" libs/sdk/src/common/decorators/ -C2
# Search for how npm and esm urlTypes are processed differently
rg -n "urlType.*npm|urlType.*esm" libs/sdk/src/app/instances/ -C2
# Check if there's processing logic that treats npm differently from esm
rg -n "esm.sh|esbuild|importmap|resolveImports|createEsmShResolver" libs/sdk/src -C2
# Look at the learnings applied - check resolver imports usage
rg -n "resolveImports|createEsmShResolver" libs/uipack/src -C2Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for npmApp or any app-related decorator functions
fd "decorator" libs/sdk/src/common/decorators/ --type f -exec basename {} \;
cat libs/sdk/src/common/decorators/app.decorator.ts | head -150
# Look for usage examples or tests
fd "esmApp\|npmApp" libs --type f -exec grep -l . {} \;Repository: agentfront/frontmcp
Length of output: 5227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for usage of esmApp in tests or examples
rg "esmApp\(" libs --type ts -B2 -A2
# Check if there's a distinction in how AppEsmInstance handles npm vs esm
rg -n "AppEsmInstance" libs/sdk/src/app/instances -C5
# Look at the full file to see both functions and understand intent
rg -n "function.*App\(|export.*App" libs/sdk/src/common/decorators/app.decorator.tsRepository: agentfront/frontmcp
Length of output: 6979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any code that explicitly sets urlType: 'npm'
rg "urlType.*npm\|urlType:\s*['\"]npm" libs/sdk/src --type ts
# Check if there's a distinction in how npm vs esm are loaded
rg -n "npm|esm" libs/sdk/src/app/instances/app.esm.instance.ts | head -40
# Look for the full RemoteAppMetadata type definition
cat libs/sdk/src/common/metadata/app.metadata.ts | grep -A30 "type RemoteAppMetadata"Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the metadata file more broadly
cat libs/sdk/src/common/metadata/app.metadata.ts | head -500 | tail -200
# Search for any literal 'npm' in the codebase
rg "'npm'" libs/sdk/src --type tsRepository: agentfront/frontmcp
Length of output: 8402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the test to see what urlType is expected for package specifiers
rg -n "urlType.*npm\|urlType.*esm" libs/sdk/src/app/__tests__/app.utils.spec.ts -B3 -A3
# Check the app.utils.ts to see how it distinguishes them
cat libs/sdk/src/app/app.utils.ts | head -100Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the test file
head -200 libs/sdk/src/app/__tests__/app.utils.spec.ts
# Read app.utils.ts
head -150 libs/sdk/src/app/app.utils.tsRepository: agentfront/frontmcp
Length of output: 6549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for AppEsmInstance or code that actually loads/processes based on npm vs esm distinction
rg -n "urlType === 'npm'|urlType === 'esm'" libs/sdk/src/app/instances/app.esm.instance.ts -B2 -A5
# Check if there's actual different processing logic
rg -n "npm\|esm" libs/sdk/src/app/instances/app.esm.instance.ts | head -30
# Check if esmApp has a counterpart npmApp function
grep -n "^export.*App\|^function.*App" libs/sdk/src/common/decorators/app.decorator.ts
# Look at the App export
tail -20 libs/sdk/src/common/decorators/app.decorator.tsRepository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the end of the decorator file
tail -50 libs/sdk/src/common/decorators/app.decorator.ts
# Check the export statement
rg "export.*App" libs/sdk/src/common/decorators/app.decorator.tsRepository: agentfront/frontmcp
Length of output: 1549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a distinction in how AppEsmInstance handles npm vs esm
head -300 libs/sdk/src/app/instances/app.esm.instance.ts | tail -150
# Look for any URL resolution or CDN logic that might differ between npm and esm
rg -n "esm.sh\|cdn\|CDN\|resolve.*npm\|resolve.*esm" libs/sdk/src/app/instances/app.esm.instance.ts -C2Repository: agentfront/frontmcp
Length of output: 4963
Change urlType: 'esm' to urlType: 'npm' in esmApp() function.
The esmApp() function accepts an npm package specifier (as documented in its JSDoc at line 84) and parses it with parsePackageSpecifier(). Per the metadata documentation, urlType: 'npm' is intended for "NPM package loaded via esm.sh CDN", while urlType: 'esm' is for "Direct ESM module URL". Since the input is a package specifier, not a direct URL, the correct urlType should be 'npm'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/common/decorators/app.decorator.ts` around lines 112 - 113, The
metadata in esmApp() is using the wrong urlType: update the object created in
esmApp() (the one that sets url and urlType when building the decorator metadata
after calling parsePackageSpecifier()) to use urlType: 'npm' instead of 'esm' so
the package specifier is marked as an NPM package loaded via esm.sh; ensure the
field remains paired with url: specifier and that parsePackageSpecifier() usage
and JSDoc remain consistent.
Comment on lines
+100
to
+108
| remote(url: string, options?: RemoteUrlAppOptions): RemoteAppMetadata { | ||
| // Derive name from URL hostname if not provided | ||
| let derivedName: string; | ||
| try { | ||
| const parsed = new URL(url); | ||
| derivedName = parsed.hostname.split('.')[0]; | ||
| } catch { | ||
| derivedName = url; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing URL validation per RFC 3986.
The url parameter is not validated before being stored in metadata. While the try-catch handles name derivation gracefully, an invalid URL will silently pass through to the returned metadata, potentially causing failures downstream. As per coding guidelines, URIs should be validated at the metadata level.
Additionally, hostname.split('.')[0] may produce unexpected results for IP addresses (e.g., 192.168.1.1 → "192") or numeric hostnames.
🛡️ Proposed fix with URL validation
remote(url: string, options?: RemoteUrlAppOptions): RemoteAppMetadata {
- // Derive name from URL hostname if not provided
- let derivedName: string;
+ // Validate URL and derive name from hostname if not provided
+ let parsedUrl: URL;
try {
- const parsed = new URL(url);
- derivedName = parsed.hostname.split('.')[0];
+ parsedUrl = new URL(url);
} catch {
- derivedName = url;
+ throw new Error(
+ `Invalid URL "${url}": URI must have a valid scheme (e.g., file://, https://, custom://)`
+ );
}
+
+ // Use full hostname for IPs, first segment otherwise
+ const hostname = parsedUrl.hostname;
+ const derivedName = /^\d+\.\d+\.\d+\.\d+$/.test(hostname) || /^\[.*\]$/.test(hostname)
+ ? hostname
+ : hostname.split('.')[0];
return {
Comment on lines
+217
to
+230
| function collectNamedExports(mod: Record<string, unknown>): FrontMcpPackageManifest { | ||
| return { | ||
| name: (mod['name'] as string) ?? 'unknown', | ||
| version: (mod['version'] as string) ?? '0.0.0', | ||
| description: mod['description'] as string | undefined, | ||
| tools: mod['tools'] as unknown[] | undefined, | ||
| prompts: mod['prompts'] as unknown[] | undefined, | ||
| resources: mod['resources'] as unknown[] | undefined, | ||
| skills: mod['skills'] as unknown[] | undefined, | ||
| agents: mod['agents'] as unknown[] | undefined, | ||
| jobs: mod['jobs'] as unknown[] | undefined, | ||
| workflows: mod['workflows'] as unknown[] | undefined, | ||
| providers: mod['providers'] as unknown[] | undefined, | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate unchecked cast + fallback patterns that can bypass runtime type safety
rg -nP "\(mod\['(name|version|description|tools|prompts|resources|skills|agents|jobs|workflows|providers)'\]\s+as\s+[^\)]+\)\s*\?\?" libs/sdk/src/esm-loader/esm-manifest.ts -C2Repository: agentfront/frontmcp
Length of output: 619
🏁 Script executed:
wc -l libs/sdk/src/esm-loader/esm-manifest.tsRepository: agentfront/frontmcp
Length of output: 106
🏁 Script executed:
cat -n libs/sdk/src/esm-loader/esm-manifest.ts | head -80Repository: agentfront/frontmcp
Length of output: 3133
🏁 Script executed:
cat -n libs/sdk/src/esm-loader/esm-manifest.ts | sed -n '210,290p'Repository: agentfront/frontmcp
Length of output: 3610
🏁 Script executed:
# Check the FrontMcpPackageManifest type definition
rg -n "type FrontMcpPackageManifest" libs/sdk/src/ -A 15Repository: agentfront/frontmcp
Length of output: 1360
🏁 Script executed:
# Find where collectNamedExports and collectDecoratedExports are called
rg -n "collectNamedExports|collectDecoratedExports" libs/sdk/src/esm-loader/Repository: agentfront/frontmcp
Length of output: 792
🏁 Script executed:
# Find where frontMcpPackageManifestSchema is used for validation
rg -n "frontMcpPackageManifestSchema" libs/sdk/src/Repository: agentfront/frontmcp
Length of output: 948
🏁 Script executed:
# Check the full context around the normalize function that likely calls these
sed -n '100,180p' libs/sdk/src/esm-loader/esm-manifest.tsRepository: agentfront/frontmcp
Length of output: 2808
🏁 Script executed:
# Check the validateManifest function
sed -n '290,310p' libs/sdk/src/esm-loader/esm-manifest.tsRepository: agentfront/frontmcp
Length of output: 434
🏁 Script executed:
# Check if there's any downstream validation of manifests returned from collectNamedExports/collectDecoratedExports
rg -n "normalizeEsmExport" libs/sdk/src/ -A 5 | head -60Repository: agentfront/frontmcp
Length of output: 4487
🏁 Script executed:
# Check if there are tests that verify the unsafe cast behavior
rg -n "collectNamedExports|collectDecoratedExports" libs/sdk/src/esm-loader/__tests__/ -B2 -A5Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Check the test file for manifest normalization to see if they test invalid types
sed -n '200,320p' libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tsRepository: agentfront/frontmcp
Length of output: 4067
🏁 Script executed:
# Check if there are any tests for invalid type handling
rg -n "name.*number|version.*number|123|test.*invalid" libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tsRepository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Look at the entire test file to see what cases are covered
wc -l libs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tsRepository: agentfront/frontmcp
Length of output: 121
🏁 Script executed:
# Check the complete flow: how is normalizeEsmExport result used in practice
rg -n "manifest\." libs/sdk/src/esm-loader/esm-module-loader.ts -A2 -B2 | head -40Repository: agentfront/frontmcp
Length of output: 199
🏁 Script executed:
# Verify the entire collectDecoratedExports function scope to see all affected lines
sed -n '259,290p' libs/sdk/src/esm-loader/esm-manifest.tsRepository: agentfront/frontmcp
Length of output: 1351
Remove unchecked type casts that allow invalid values to bypass manifest validation.
Lines 219–220 and 280–281 use (mod['fieldName'] as string) ?? 'fallback' which allows non-string truthy values (e.g., 123) to be returned as string. The ?? operator doesn't check type—it only checks for null/undefined. Similarly, array fields cast without validation via as unknown[] | undefined.
Both collectNamedExports and collectDecoratedExports return FrontMcpPackageManifest without passing through validateManifest, bypassing the Zod schema validation that exists for manifest objects. This leaks type violations downstream.
Suggested fix
+function asNonEmptyString(value: unknown, fallback: string): string {
+ return typeof value === 'string' && value.trim().length > 0 ? value : fallback;
+}
+
+function asOptionalString(value: unknown): string | undefined {
+ return typeof value === 'string' ? value : undefined;
+}
+
+function asOptionalArray(value: unknown): unknown[] | undefined {
+ return Array.isArray(value) ? value : undefined;
+}
+
function collectNamedExports(mod: Record<string, unknown>): FrontMcpPackageManifest {
return {
- name: (mod['name'] as string) ?? 'unknown',
- version: (mod['version'] as string) ?? '0.0.0',
- description: mod['description'] as string | undefined,
- tools: mod['tools'] as unknown[] | undefined,
- prompts: mod['prompts'] as unknown[] | undefined,
- resources: mod['resources'] as unknown[] | undefined,
- skills: mod['skills'] as unknown[] | undefined,
- agents: mod['agents'] as unknown[] | undefined,
- jobs: mod['jobs'] as unknown[] | undefined,
- workflows: mod['workflows'] as unknown[] | undefined,
- providers: mod['providers'] as unknown[] | undefined,
+ name: asNonEmptyString(mod['name'], 'unknown'),
+ version: asNonEmptyString(mod['version'], '0.0.0'),
+ description: asOptionalString(mod['description']),
+ tools: asOptionalArray(mod['tools']),
+ prompts: asOptionalArray(mod['prompts']),
+ resources: asOptionalArray(mod['resources']),
+ skills: asOptionalArray(mod['skills']),
+ agents: asOptionalArray(mod['agents']),
+ jobs: asOptionalArray(mod['jobs']),
+ workflows: asOptionalArray(mod['workflows']),
+ providers: asOptionalArray(mod['providers']),
};
}
function collectDecoratedExports(mod: Record<string, unknown>): FrontMcpPackageManifest {
// ... array collection code ...
return {
- name: (mod['name'] as string) ?? 'unknown',
- version: (mod['version'] as string) ?? '0.0.0',
+ name: asNonEmptyString(mod['name'], 'unknown'),
+ version: asNonEmptyString(mod['version'], '0.0.0'),
...(tools.length ? { tools } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/esm-loader/esm-manifest.ts` around lines 217 - 230,
collectNamedExports (and collectDecoratedExports) currently uses unchecked casts
like (mod['name'] as string) and (mod['tools'] as unknown[]), which lets
non-string/non-array truthy values bypass type validation; replace these casts
with proper runtime checks and normalize values (e.g., ensure typeof name ===
'string' else use fallback, ensure Array.isArray(tools) else undefined) and then
pass the constructed object through validateManifest so the Zod schema enforces
correct types before returning a FrontMcpPackageManifest.
… Job, Prompt, Resource, and Skill
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
libs/sdk/src/esm-loader/esm-manifest.ts (1)
222-229:⚠️ Potential issue | 🟡 MinorArray fields still use unchecked casts.
While
name,version, anddescriptionnow have proper type guards (lines 219-221), the array fields on lines 222-229 still use direct casts withoutArray.isArray()validation. Non-array truthy values would leak through.🛡️ Proposed fix
+function asOptionalArray(value: unknown): unknown[] | undefined { + return Array.isArray(value) ? value : undefined; +} + function collectNamedExports(mod: Record<string, unknown>): FrontMcpPackageManifest { return { name: typeof mod['name'] === 'string' ? mod['name'] : 'unknown', version: typeof mod['version'] === 'string' ? mod['version'] : '0.0.0', description: typeof mod['description'] === 'string' ? mod['description'] : undefined, - tools: mod['tools'] as unknown[] | undefined, - prompts: mod['prompts'] as unknown[] | undefined, - resources: mod['resources'] as unknown[] | undefined, - skills: mod['skills'] as unknown[] | undefined, - agents: mod['agents'] as unknown[] | undefined, - jobs: mod['jobs'] as unknown[] | undefined, - workflows: mod['workflows'] as unknown[] | undefined, - providers: mod['providers'] as unknown[] | undefined, + tools: asOptionalArray(mod['tools']), + prompts: asOptionalArray(mod['prompts']), + resources: asOptionalArray(mod['resources']), + skills: asOptionalArray(mod['skills']), + agents: asOptionalArray(mod['agents']), + jobs: asOptionalArray(mod['jobs']), + workflows: asOptionalArray(mod['workflows']), + providers: asOptionalArray(mod['providers']), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/esm-loader/esm-manifest.ts` around lines 222 - 229, The array-typed manifest fields (tools, prompts, resources, skills, agents, jobs, workflows, providers) are currently assigned using unchecked casts (e.g., mod['tools'] as unknown[]), which lets non-array truthy values through; update the assignment in esm-manifest.ts to validate with Array.isArray before casting (e.g., set each field to Array.isArray(mod['tools']) ? mod['tools'] as unknown[] : undefined) so only real arrays are accepted, using the same pattern for prompts, resources, skills, agents, jobs, workflows and providers.libs/sdk/src/job/job.utils.ts (1)
24-34:⚠️ Potential issue | 🟠 MajorStrengthen ESM/REMOTE shape checks before casting to
JobRecord.Line 24-34 currently accepts partial objects (only
kind/provide/metadata). Invalid records missingtargetName,specifier, orurlcan still pass and fail later.🔧 Proposed fix
+function isEsmOrRemoteJobRecord(item: unknown): item is JobRecord { + if (!item || typeof item !== 'object') return false; + const rec = item as Record<string, unknown>; + + if (rec.kind === JobKind.ESM) { + return ( + typeof rec.provide === 'symbol' && + typeof rec.targetName === 'string' && + !!rec.targetName && + typeof rec.specifier === 'object' && + rec.specifier !== null && + typeof rec.metadata === 'object' && + rec.metadata !== null + ); + } + + if (rec.kind === JobKind.REMOTE) { + return ( + typeof rec.provide === 'symbol' && + typeof rec.url === 'string' && + !!rec.url && + typeof rec.targetName === 'string' && + !!rec.targetName && + typeof rec.metadata === 'object' && + rec.metadata !== null + ); + } + + return false; +} + export function normalizeJob(item: unknown): JobRecord { - if ( - item && - typeof item === 'object' && - 'kind' in item && - (item.kind === JobKind.ESM || item.kind === JobKind.REMOTE) && - 'provide' in item && - 'metadata' in item - ) { - return item as JobRecord; - } + if (isEsmOrRemoteJobRecord(item)) { + return item; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/job/job.utils.ts` around lines 24 - 34, The current runtime check in job.utils.ts casts partial objects to JobRecord too early; update the guard around the `item` object (used when handling Job.esm() / Job.remote()) to verify required fields per kind before returning: when `item.kind === JobKind.ESM` ensure `typeof item.specifier === 'string'` and `typeof item.targetName === 'string'`; when `item.kind === JobKind.REMOTE` ensure `typeof item.url === 'string'` and `typeof item.targetName === 'string'`; only then return `item as JobRecord`. Keep the existing checks for `kind`, `provide`, and `metadata`, and otherwise fall through (do not cast).libs/sdk/src/common/utils/validate-remote-url.ts (1)
16-17:⚠️ Potential issue | 🟠 MajorUse the exact canonical URI validation message.
Line 17 still prefixes the canonical text with
Invalid remote URL:. Keep the thrown message exactly canonical to avoid contract drift across validators.🔧 Proposed fix
- throw new Error('Invalid remote URL: URI must have a valid scheme (e.g., file://, https://, custom://)'); + throw new Error('URI must have a valid scheme (e.g., file://, https://, custom://)');As per coding guidelines, "Validate URIs per RFC 3986 at metadata level using schema validation with message 'URI must have a valid scheme (e.g., file://, https://, custom://)'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/utils/validate-remote-url.ts` around lines 16 - 17, The throw in validate-remote-url currently prefixes the canonical message; update the error thrown in the isValidMcpUri failure branch so it throws exactly the canonical text "URI must have a valid scheme (e.g., file://, https://, custom://)" (no extra prefix or punctuation), locating the change in the isValidMcpUri check inside validate-remote-url.ts and replacing the existing throw new Error(...) message accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/common/decorators/app.decorator.ts`:
- Around line 98-101: The truthy check for options?.autoUpdate drops explicit
false values; change the condition to test for undefined (e.g.,
options?.autoUpdate !== undefined) so that packageConfig.autoUpdate is assigned
even when false, and still set hasPackageConfig = true; update the block around
packageConfig.autoUpdate and hasPackageConfig accordingly to preserve explicit
false.
In `@libs/sdk/src/common/decorators/job.decorator.ts`:
- Around line 89-97: The code currently embeds the full remote URL into the
Symbol key and default metadata.description (e.g., provide:
Symbol(`remote-job:${url}:${targetName}`) and description: `Remote job
"${targetName}" from ${url}`), which can leak credentials; change these to use a
sanitized form of the URL (e.g., strip credentials and query params or only
include host/origin) or omit the URL entirely from internal keys and metadata.
Implement a small helper (e.g., sanitizeRemoteUrl(url)) and replace the direct
interpolations in the job decorator (the provide Symbol creation and
metadata.description) to use the sanitized value or a generic placeholder like
"<redacted-url>"; apply the same pattern to the analogous occurrences in
prompt.decorator (provide and description), skill.decorator (provide and
description), and resource.decorator (provide and description).
In `@libs/sdk/src/esm-loader/esm-manifest.ts`:
- Around line 194-211: The extractFromDecoratedClass function currently casts
multiple config fields (tools, prompts, resources, skills, agents, jobs,
workflows, providers) directly to arrays without runtime checks; update
extractFromDecoratedClass to validate each of those fields with Array.isArray
before assigning them (e.g., only set tools = config['tools'] as unknown[] if
Array.isArray(config['tools'])), otherwise set them to undefined (or an empty
array per project convention), so downstream code never receives non-array
values; keep name/version logic unchanged and use narrowed types after the
Array.isArray checks to satisfy the type system.
---
Duplicate comments:
In `@libs/sdk/src/common/utils/validate-remote-url.ts`:
- Around line 16-17: The throw in validate-remote-url currently prefixes the
canonical message; update the error thrown in the isValidMcpUri failure branch
so it throws exactly the canonical text "URI must have a valid scheme (e.g.,
file://, https://, custom://)" (no extra prefix or punctuation), locating the
change in the isValidMcpUri check inside validate-remote-url.ts and replacing
the existing throw new Error(...) message accordingly.
In `@libs/sdk/src/esm-loader/esm-manifest.ts`:
- Around line 222-229: The array-typed manifest fields (tools, prompts,
resources, skills, agents, jobs, workflows, providers) are currently assigned
using unchecked casts (e.g., mod['tools'] as unknown[]), which lets non-array
truthy values through; update the assignment in esm-manifest.ts to validate with
Array.isArray before casting (e.g., set each field to
Array.isArray(mod['tools']) ? mod['tools'] as unknown[] : undefined) so only
real arrays are accepted, using the same pattern for prompts, resources, skills,
agents, jobs, workflows and providers.
In `@libs/sdk/src/job/job.utils.ts`:
- Around line 24-34: The current runtime check in job.utils.ts casts partial
objects to JobRecord too early; update the guard around the `item` object (used
when handling Job.esm() / Job.remote()) to verify required fields per kind
before returning: when `item.kind === JobKind.ESM` ensure `typeof item.specifier
=== 'string'` and `typeof item.targetName === 'string'`; when `item.kind ===
JobKind.REMOTE` ensure `typeof item.url === 'string'` and `typeof
item.targetName === 'string'`; only then return `item as JobRecord`. Keep the
existing checks for `kind`, `provide`, and `metadata`, and otherwise fall
through (do not cast).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13c0e11a-f968-40c6-96a1-2e8cf9e3556d
📒 Files selected for processing (14)
libs/sdk/src/common/decorators/app.decorator.tslibs/sdk/src/common/decorators/job.decorator.tslibs/sdk/src/common/decorators/prompt.decorator.tslibs/sdk/src/common/decorators/resource.decorator.tslibs/sdk/src/common/decorators/skill.decorator.tslibs/sdk/src/common/utils/validate-remote-url.tslibs/sdk/src/esm-loader/__tests__/esm-manifest.spec.tslibs/sdk/src/esm-loader/__tests__/load-from.spec.tslibs/sdk/src/esm-loader/esm-manifest.tslibs/sdk/src/job/__tests__/job-npm-remote.spec.tslibs/sdk/src/job/job.utils.tslibs/sdk/src/prompt/__tests__/prompt-npm-remote.spec.tslibs/sdk/src/resource/__tests__/resource-npm-remote.spec.tslibs/sdk/src/skill/__tests__/skill-npm-remote.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sdk/src/job/tests/job-npm-remote.spec.ts
- libs/sdk/src/prompt/tests/prompt-npm-remote.spec.ts
- libs/sdk/src/resource/tests/resource-npm-remote.spec.ts
Comment on lines
+98
to
+101
| if (options?.autoUpdate) { | ||
| packageConfig.autoUpdate = options.autoUpdate; | ||
| hasPackageConfig = true; | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve explicit autoUpdate: false in package config.
Line 98 uses a truthy check, so false is silently ignored.
🔧 Proposed fix
- if (options?.autoUpdate) {
+ if (options?.autoUpdate !== undefined) {
packageConfig.autoUpdate = options.autoUpdate;
hasPackageConfig = true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/common/decorators/app.decorator.ts` around lines 98 - 101, The
truthy check for options?.autoUpdate drops explicit false values; change the
condition to test for undefined (e.g., options?.autoUpdate !== undefined) so
that packageConfig.autoUpdate is assigned even when false, and still set
hasPackageConfig = true; update the block around packageConfig.autoUpdate and
hasPackageConfig accordingly to preserve explicit false.
Comment on lines
+89
to
+97
| provide: Symbol(`remote-job:${url}:${targetName}`), | ||
| url, | ||
| targetName, | ||
| transportOptions: options?.transportOptions, | ||
| remoteAuth: options?.remoteAuth, | ||
| metadata: { | ||
| name: targetName, | ||
| description: `Remote job "${targetName}" from ${url}`, | ||
| inputSchema: {}, |
Contributor
There was a problem hiding this comment.
Avoid exposing raw remote URLs in record internals and default metadata.
Line 89 (provide) and Line 96 (description) include full url. If callers pass credential-bearing URLs, this can leak sensitive data via logging/debug output. Please redact/sanitize defaults and avoid raw URL embedding.
Downstream impact: same pattern appears in libs/sdk/src/common/decorators/prompt.decorator.ts (Line 87, Line 94), libs/sdk/src/common/decorators/skill.decorator.ts (Line 170, Line 177), and libs/sdk/src/common/decorators/resource.decorator.ts (Line 160, Line 167).
🔧 Proposed fix
function jobRemote(url: string, targetName: string, options?: RemoteOptions<JobMetadata>): JobRemoteRecord {
validateRemoteUrl(url);
return {
kind: JobKind.REMOTE,
- provide: Symbol(`remote-job:${url}:${targetName}`),
+ provide: Symbol(`remote-job:${targetName}`),
url,
targetName,
@@
metadata: {
name: targetName,
- description: `Remote job "${targetName}" from ${url}`,
+ description: `Remote job "${targetName}"`,
inputSchema: {},
outputSchema: {},
...options?.metadata,
},
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/common/decorators/job.decorator.ts` around lines 89 - 97, The
code currently embeds the full remote URL into the Symbol key and default
metadata.description (e.g., provide: Symbol(`remote-job:${url}:${targetName}`)
and description: `Remote job "${targetName}" from ${url}`), which can leak
credentials; change these to use a sanitized form of the URL (e.g., strip
credentials and query params or only include host/origin) or omit the URL
entirely from internal keys and metadata. Implement a small helper (e.g.,
sanitizeRemoteUrl(url)) and replace the direct interpolations in the job
decorator (the provide Symbol creation and metadata.description) to use the
sanitized value or a generic placeholder like "<redacted-url>"; apply the same
pattern to the analogous occurrences in prompt.decorator (provide and
description), skill.decorator (provide and description), and resource.decorator
(provide and description).
Comment on lines
+194
to
+211
| function extractFromDecoratedClass(cls: unknown): FrontMcpPackageManifest { | ||
| const config = Reflect.getMetadata?.('__frontmcp:config', cls as object) as Record<string, unknown> | undefined; | ||
| if (!config) { | ||
| throw new Error('Decorated class does not have FrontMcp configuration metadata'); | ||
| } | ||
|
|
||
| return { | ||
| name: ((config['info'] as Record<string, unknown>)?.['name'] as string) ?? 'unknown', | ||
| version: '0.0.0', | ||
| tools: config['tools'] as unknown[] | undefined, | ||
| prompts: config['prompts'] as unknown[] | undefined, | ||
| resources: config['resources'] as unknown[] | undefined, | ||
| skills: config['skills'] as unknown[] | undefined, | ||
| agents: config['agents'] as unknown[] | undefined, | ||
| jobs: config['jobs'] as unknown[] | undefined, | ||
| workflows: config['workflows'] as unknown[] | undefined, | ||
| providers: config['providers'] as unknown[] | undefined, | ||
| }; |
Contributor
There was a problem hiding this comment.
Unsafe type casts in extractFromDecoratedClass for array fields.
The function casts array fields directly without runtime validation (lines 203-210). If the decorated class config contains non-array values for these fields, they will bypass validation and cause downstream errors.
🛡️ Proposed fix to add array validation
+function asOptionalArray(value: unknown): unknown[] | undefined {
+ return Array.isArray(value) ? value : undefined;
+}
+
function extractFromDecoratedClass(cls: unknown): FrontMcpPackageManifest {
const config = Reflect.getMetadata?.('__frontmcp:config', cls as object) as Record<string, unknown> | undefined;
if (!config) {
throw new Error('Decorated class does not have FrontMcp configuration metadata');
}
return {
name: ((config['info'] as Record<string, unknown>)?.['name'] as string) ?? 'unknown',
version: '0.0.0',
- tools: config['tools'] as unknown[] | undefined,
- prompts: config['prompts'] as unknown[] | undefined,
- resources: config['resources'] as unknown[] | undefined,
- skills: config['skills'] as unknown[] | undefined,
- agents: config['agents'] as unknown[] | undefined,
- jobs: config['jobs'] as unknown[] | undefined,
- workflows: config['workflows'] as unknown[] | undefined,
- providers: config['providers'] as unknown[] | undefined,
+ tools: asOptionalArray(config['tools']),
+ prompts: asOptionalArray(config['prompts']),
+ resources: asOptionalArray(config['resources']),
+ skills: asOptionalArray(config['skills']),
+ agents: asOptionalArray(config['agents']),
+ jobs: asOptionalArray(config['jobs']),
+ workflows: asOptionalArray(config['workflows']),
+ providers: asOptionalArray(config['providers']),
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/esm-loader/esm-manifest.ts` around lines 194 - 211, The
extractFromDecoratedClass function currently casts multiple config fields
(tools, prompts, resources, skills, agents, jobs, workflows, providers) directly
to arrays without runtime checks; update extractFromDecoratedClass to validate
each of those fields with Array.isArray before assigning them (e.g., only set
tools = config['tools'] as unknown[] if Array.isArray(config['tools'])),
otherwise set them to undefined (or an empty array per project convention), so
downstream code never receives non-array values; keep name/version logic
unchanged and use narrowed types after the Array.isArray checks to satisfy the
type system.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores