fix: enhance configuration resolution and client snippet generation for frontmcp#430
Open
frontegg-david wants to merge 1 commit into
Open
fix: enhance configuration resolution and client snippet generation for frontmcp#430frontegg-david wants to merge 1 commit into
frontegg-david wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete config resolution system for FrontMCP CLI (issue ChangesConfig Resolution & Command Integration
Tests, Documentation, and Schema Emission
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as frontmcp dev/test
participant resolveConfig as resolveConfig
participant loadConfig as loadFrontMcpConfigFromFile
participant findConfig as findConfigDir
participant DevCmd as Command Handler
participant Child as Child Process
User->>CLI: frontmcp dev --config path/frontmcp.config.ts
CLI->>resolveConfig: resolve(configPath, mode: 'dev', cwd)
alt explicit config path
resolveConfig->>loadConfig: loadFrontMcpConfigFromFile(configPath)
else directory search
resolveConfig->>findConfig: findConfigDir(cwd, maxLevels: 10)
findConfig-->>resolveConfig: configDir or undefined
end
loadConfig-->>resolveConfig: FrontMcpConfigParsed
resolveConfig->>resolveConfig: compose effectiveEnv (process.env + config.env.shared + config.env.dev)
resolveConfig-->>CLI: ResolvedFrontMcpConfig
CLI->>DevCmd: entry from config or CLI
CLI->>Child: spawn with effectiveEnv + PORT
Child-->>User: dev server running
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cli/src/commands/dev/test.ts`:
- Around line 189-194: The status logging uses the original opts while runTest
runs with mergedOpts, causing mismatched logs; update the logging calls that
reference opts (the ones around where runTest is invoked) to use mergedOpts
instead so reported values (runInBand, coverage, timeout) reflect the actual
parameters passed to runTest (keep mergedOpts variable name and behavior
intact).
In `@libs/cli/src/commands/eject/mcp-client.ts`:
- Around line 30-77: Add unit tests that exercise buildServerEntry and
emitClientSnippet covering all branches: (1) missing client -> expect throw from
buildServerEntry, (2) stdio path -> verify default command 'npx', default args
['-y', config.name], and env copying when present, (3) http/sse path -> verify
URL is derived from transport.http.port or deployment server.http.port and
transport is set on the ServerEntry, and (4) missing URL -> expect throw when
neither connection.url nor fallback port exists; also assert emitClientSnippet
returns the correct JSON payload with the server key chosen from connection.name
or config.name. Use the exported functions buildServerEntry and
emitClientSnippet and create minimal FrontMcpConfigParsed fixtures to drive each
branch.
- Around line 51-55: The fallback URL selection is ambiguous because httpPort is
chosen from the first deployment, which can point to the wrong server when
multiple deployments exist; update the logic in the block that computes
httpPort/httpHost/httpPath/fallbackUrl/url so that if connection.url is absent
you first try config.transport?.http, then search config.deployments for the
deployment that matches the connection (e.g., compare deployment.name or
deployment.id to connection.deployment or connection.target) and use that
deployment's .server?.http?.port/host/path; if no matching deployment exists,
fall back to undefined (or throw) instead of using the first deployment; update
variables httpPort, fallbackUrl and url accordingly so the selected deployment
is explicitly matched to the connection.
In `@libs/cli/src/commands/scaffold/create.ts`:
- Line 178: The stdio client branch uses sanitizeForFolder(projectName) to build
the safeName for command args, but scaffoldProject uses
sanitizeForNpm(projectName) for package.json, causing scoped names to diverge;
change the stdio usage to use sanitizeForNpm so both client invocation and
published package name match. Locate the safeName assignment (currently using
sanitizeForFolder) in the stdio/stdio args code and replace it with
sanitizeForNpm(projectName) (and update the other occurrence around line 198
similarly) so the stdio command uses the exact package name that scaffoldProject
will publish; ensure variable name and downstream uses remain unchanged.
- Around line 179-183: The stdio client/transport branch is dead because
isHttpTarget (used in create.ts) always evaluates true for deploymentTarget
('node'|'vercel'|'lambda'|'cloudflare'); either remove the stdio block (delete
the stdio client/transport code) or make stdio reachable by changing
isHttpTarget to exclude 'node' (or update the DeploymentTarget type to add a
distinct 'desktop' target) so the stdio branch can execute; if you choose to
reintroduce stdio, also fix the package-name mismatch by using the same
sanitization function as scaffoldProject (replace sanitizeForFolder usage with
sanitizeForNpm or unify both to one function) so scoped packages like
`@myorg/my-app` run and declare the correct name.
In `@libs/cli/src/config/__tests__/frontmcp-config.resolve.spec.ts`:
- Around line 13-15: The tests import fs directly; replace that with async
helpers from `@frontmcp/utils` (import mkdtemp, mkdir, writeFile) while keeping
os.tmpdir() for the prefix, update the helper function makeTempProject to be
async and use await mkdtemp(...) instead of fs.mkdtempSync and await mkdir(...)
/ await writeFile(...) instead of fs.mkdirSync/fs.writeFileSync, then await
every call site that invokes makeTempProject (the calls at the test locations)
and replace any other direct fs.mkdtempSync or fs.mkdirSync usages with the
async mkdtemp/mkdir equivalents.
In `@libs/cli/src/config/frontmcp-config.loader.ts`:
- Around line 48-55: The loader is directly using fs.existsSync and other fs
APIs instead of the shared FS abstraction; update loadFrontMcpConfigFromFile to
call the fileExists (and readFile/readFileBuffer or readJSON as appropriate)
helpers from `@frontmcp/utils` rather than fs.existsSync or fs/promises, and
ensure imports are added from `@frontmcp/utils`; also audit and replace similar
direct fs usage in the same module (including the logic around loadRawFileAtPath
and the other blocks flagged) so all filesystem access goes through the shared
utilities (e.g., use fileExists, readFile/readFileBuffer, etc.) and remove any
direct node:fs or fs/promises usage.
- Around line 195-217: Dynamic imports use a plain filesystem path which fails
on Windows; convert configPath to a file:// URL before calling import in both
the early TS branch (where you do const mod = await import(configPath)) and the
.mjs branch (where you do const mod = await import(configPath)); import and use
pathToFileURL from 'url' and pass pathToFileURL(configPath).href (or the URL
object) to import() so Windows absolute paths are valid ESM specifiers, keeping
the existing mod.default ?? mod behavior and error handling around
loadTsConfigViaEsbuild and requireErr unchanged.
In `@libs/cli/src/config/frontmcp-config.resolve.ts`:
- Around line 112-129: The current explicitPath and configDir branches call
hard-validating loaders (loadFrontMcpConfigFromFile and loadFrontMcpConfig) and
throw on errors, bypassing the legacy-config fallback; change these branches to
use the non-throwing path in tryLoadFrontMcpConfig (or catch the loader error
and return { config: undefined } instead of throwing) so that exec-only/legacy
configs produce a graceful undefined config rather than an exception; ensure you
still set configPath when explicitPath succeeds and preserve the existing error
surfacing for real schema/load failures only when tryLoadFrontMcpConfig
indicates it’s not a legacy shape.
- Around line 101-104: The current unchecked cast of options.cliOptions?.['env']
to Record<string,string> can allow non-string values; replace it by building
cliEnv from Object.entries(options.cliOptions?.['env'] ?? {}) and only include
keys whose values satisfy typeof value === 'string' (e.g., filter or reduce to a
Record<string,string>), so cliEnv becomes a safe Record<string,string> of only
string values before merging into effectiveEnv; apply the same validation logic
to the other occurrence referenced around the symbol on line ~144.
- Around line 112-118: The explicitPath branch currently returns the
caller-supplied string and leaves configDir unset; update the branch that calls
loadFrontMcpConfigFromFile(explicitPath) to normalize/resolve configPath to an
absolute path and populate configDir with its parent directory before returning
a ResolvedFrontMcpConfig. Concretely, after successful load in the explicitPath
branch (the block using loadFrontMcpConfigFromFile and setting configPath =
explicitPath), replace the verbatim assignment with logic that resolves
explicitPath to an absolute path (e.g., via path.resolve or equivalent), set
configPath to that normalized absolute path, and set configDir to
path.dirname(configPath) so the returned ResolvedFrontMcpConfig always contains
a canonical absolute configPath and its containing directory.
In `@libs/cli/src/config/frontmcp-config.schema.ts`:
- Around line 382-390: The clientConnectionSchema currently lets any string
through for the url field; update the schema so provided URLs are validated by
Zod. Replace the url: z.string().optional() entry in clientConnectionSchema with
url: z.string().url().optional() (or z.string().url({ message: "invalid url"
}).optional() if you want a custom message) so clients[*].url is syntactically
validated at parse time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ed7c0a9-f530-4140-96e2-aaa925ecaf4a
📒 Files selected for processing (20)
docs/frontmcp/deployment/frontmcp-config.mdxlibs/cli/scripts/emit-schema.tslibs/cli/src/commands/dev/dev.tslibs/cli/src/commands/dev/inspector.tslibs/cli/src/commands/dev/register.tslibs/cli/src/commands/dev/test.tslibs/cli/src/commands/eject/mcp-client.tslibs/cli/src/commands/eject/register.tslibs/cli/src/commands/scaffold/create.tslibs/cli/src/config/__tests__/frontmcp-config.resolve.spec.tslibs/cli/src/config/frontmcp-config.loader.tslibs/cli/src/config/frontmcp-config.resolve.tslibs/cli/src/config/frontmcp-config.schema.tslibs/cli/src/config/frontmcp-config.types.tslibs/cli/src/config/index.tslibs/cli/src/core/__tests__/program.spec.tslibs/cli/src/core/args.tslibs/cli/src/core/bridge.tslibs/cli/src/core/program.tslibs/skills/catalog/frontmcp-config/references/configure-deployment-targets.md
Comment on lines
+189
to
+194
| const mergedOpts = { | ||
| ...opts, | ||
| runInBand: opts.runInBand ?? testDefaults?.runInBand, | ||
| coverage: opts.coverage ?? testDefaults?.coverage, | ||
| timeout: opts.timeout ?? testDefaults?.timeoutMs, | ||
| } as ParsedArgs; |
Contributor
There was a problem hiding this comment.
Use merged test options for status logging.
runTest executes with mergedOpts, but Lines 265, 269, and 273 still log from opts. When values come from frontmcp.config, execution and logs diverge.
Suggested fix
- if (opts.runInBand) {
+ if (mergedOpts.runInBand) {
console.log(`${c('gray', '[test]')} running tests sequentially (--runInBand)`);
}
- if (opts.watch) {
+ if (mergedOpts.watch) {
console.log(`${c('gray', '[test]')} watch mode enabled`);
}
- if (opts.coverage) {
+ if (mergedOpts.coverage) {
console.log(`${c('gray', '[test]')} coverage collection enabled`);
}Also applies to: 265-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/commands/dev/test.ts` around lines 189 - 194, The status logging
uses the original opts while runTest runs with mergedOpts, causing mismatched
logs; update the logging calls that reference opts (the ones around where
runTest is invoked) to use mergedOpts instead so reported values (runInBand,
coverage, timeout) reflect the actual parameters passed to runTest (keep
mergedOpts variable name and behavior intact).
Comment on lines
+30
to
+77
| function buildServerEntry(client: McpClientName, config: FrontMcpConfigParsed): ServerEntry { | ||
| const connection = config.clients?.[client]; | ||
| if (!connection) { | ||
| throw new Error( | ||
| `frontmcp.config has no \`clients.${client}\` entry. ` + | ||
| `Add it: \`clients: { '${client}': { transport: '...' , ... } }\``, | ||
| ); | ||
| } | ||
|
|
||
| // Stdio: spawn `command` with `args` + `env`. Most MCP clients omit the | ||
| // `transport` field when stdio (it's the default), so we follow suit. | ||
| if (connection.transport === 'stdio') { | ||
| const command = connection.command ?? 'npx'; | ||
| const args = connection.args ?? ['-y', config.name]; | ||
| const entry: ServerEntry = { command, args }; | ||
| if (connection.env && Object.keys(connection.env).length > 0) entry.env = { ...connection.env }; | ||
| return entry; | ||
| } | ||
|
|
||
| // HTTP / SSE: emit `url` + `transport`. URL falls back to the configured | ||
| // HTTP port when none is provided. | ||
| const httpPort = config.transport?.http?.port ?? config.deployments.find((d) => 'server' in d)?.server?.http?.port; | ||
| const httpHost = config.transport?.http?.host ?? '127.0.0.1'; | ||
| const httpPath = config.transport?.http?.path ?? '/mcp'; | ||
| const fallbackUrl = httpPort ? `http://${httpHost}:${httpPort}${httpPath}` : undefined; | ||
| const url = connection.url ?? fallbackUrl; | ||
| if (!url) { | ||
| throw new Error( | ||
| `frontmcp.config \`clients.${client}\` needs a \`url\`, or a \`transport.http.port\` / deployment HTTP port to derive one.`, | ||
| ); | ||
| } | ||
| const entry: ServerEntry = { url, transport: connection.transport }; | ||
| if (connection.env && Object.keys(connection.env).length > 0) entry.env = { ...connection.env }; | ||
| return entry; | ||
| } | ||
|
|
||
| /** | ||
| * Build the user-pasteable snippet for the given client. All five clients | ||
| * use the `{ mcpServers: { <name>: { ... } } }` shape — they differ only in | ||
| * the file the user pastes it into. | ||
| */ | ||
| export function emitClientSnippet(client: McpClientName, config: FrontMcpConfigParsed): string { | ||
| const connection = config.clients?.[client]; | ||
| const serverKey = connection?.name ?? config.name; | ||
| const entry = buildServerEntry(client, config); | ||
| const payload = { mcpServers: { [serverKey]: entry } }; | ||
| return JSON.stringify(payload, null, 2); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Please add targeted tests for snippet generation branches.
This adds multiple branch/error paths (missing client, stdio defaults, http/sse URL derivation, missing URL), but no accompanying tests are included in this PR slice. Add unit tests for these paths before merge.
As per coding guidelines, "libs/**/*.ts?(x): Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines) in all libraries".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/commands/eject/mcp-client.ts` around lines 30 - 77, Add unit
tests that exercise buildServerEntry and emitClientSnippet covering all
branches: (1) missing client -> expect throw from buildServerEntry, (2) stdio
path -> verify default command 'npx', default args ['-y', config.name], and env
copying when present, (3) http/sse path -> verify URL is derived from
transport.http.port or deployment server.http.port and transport is set on the
ServerEntry, and (4) missing URL -> expect throw when neither connection.url nor
fallback port exists; also assert emitClientSnippet returns the correct JSON
payload with the server key chosen from connection.name or config.name. Use the
exported functions buildServerEntry and emitClientSnippet and create minimal
FrontMcpConfigParsed fixtures to drive each branch.
Comment on lines
+51
to
+55
| const httpPort = config.transport?.http?.port ?? config.deployments.find((d) => 'server' in d)?.server?.http?.port; | ||
| const httpHost = config.transport?.http?.host ?? '127.0.0.1'; | ||
| const httpPath = config.transport?.http?.path ?? '/mcp'; | ||
| const fallbackUrl = httpPort ? `http://${httpHost}:${httpPort}${httpPath}` : undefined; | ||
| const url = connection.url ?? fallbackUrl; |
Contributor
There was a problem hiding this comment.
Ambiguous deployment fallback can generate the wrong client URL.
When connection.url and transport.http.port are absent, this picks the first deployment HTTP port. In configs with multiple deployments, the snippet can target the wrong server.
Suggested fix
- const httpPort = config.transport?.http?.port ?? config.deployments.find((d) => 'server' in d)?.server?.http?.port;
+ const deploymentPorts = config.deployments
+ .map((d) => ('server' in d ? d.server?.http?.port : undefined))
+ .filter((p): p is number => typeof p === 'number');
+
+ const derivedDeploymentPort = deploymentPorts.length === 1 ? deploymentPorts[0] : undefined;
+ const httpPort = config.transport?.http?.port ?? derivedDeploymentPort;
+
+ if (!connection.url && !httpPort && deploymentPorts.length > 1) {
+ throw new Error(
+ `frontmcp.config \`clients.${client}.url\` is required when multiple deployment HTTP ports are configured.`,
+ );
+ }📝 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
| const httpPort = config.transport?.http?.port ?? config.deployments.find((d) => 'server' in d)?.server?.http?.port; | |
| const httpHost = config.transport?.http?.host ?? '127.0.0.1'; | |
| const httpPath = config.transport?.http?.path ?? '/mcp'; | |
| const fallbackUrl = httpPort ? `http://${httpHost}:${httpPort}${httpPath}` : undefined; | |
| const url = connection.url ?? fallbackUrl; | |
| const deploymentPorts = config.deployments | |
| .map((d) => ('server' in d ? d.server?.http?.port : undefined)) | |
| .filter((p): p is number => typeof p === 'number'); | |
| const derivedDeploymentPort = deploymentPorts.length === 1 ? deploymentPorts[0] : undefined; | |
| const httpPort = config.transport?.http?.port ?? derivedDeploymentPort; | |
| if (!connection.url && !httpPort && deploymentPorts.length > 1) { | |
| throw new Error( | |
| `frontmcp.config \`clients.${client}.url\` is required when multiple deployment HTTP ports are configured.`, | |
| ); | |
| } | |
| const httpHost = config.transport?.http?.host ?? '127.0.0.1'; | |
| const httpPath = config.transport?.http?.path ?? '/mcp'; | |
| const fallbackUrl = httpPort ? `http://${httpHost}:${httpPort}${httpPath}` : undefined; | |
| const url = connection.url ?? fallbackUrl; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/commands/eject/mcp-client.ts` around lines 51 - 55, The fallback
URL selection is ambiguous because httpPort is chosen from the first deployment,
which can point to the wrong server when multiple deployments exist; update the
logic in the block that computes httpPort/httpHost/httpPath/fallbackUrl/url so
that if connection.url is absent you first try config.transport?.http, then
search config.deployments for the deployment that matches the connection (e.g.,
compare deployment.name or deployment.id to connection.deployment or
connection.target) and use that deployment's .server?.http?.port/host/path; if
no matching deployment exists, fall back to undefined (or throw) instead of
using the first deployment; update variables httpPort, fallbackUrl and url
accordingly so the selected deployment is explicitly matched to the connection.
| * `deployment/frontmcp-config` docs page for the full surface. | ||
| */ | ||
| function renderFrontmcpConfigTemplate(projectName: string, deploymentTarget: DeploymentTarget): string { | ||
| const safeName = sanitizeForFolder(projectName); |
Contributor
There was a problem hiding this comment.
Fix package name sanitization mismatch for stdio clients.
Line 178 uses sanitizeForFolder(projectName) for the stdio command args, but scaffoldProject uses sanitizeForNpm(projectName) for the package.json name (lines 1669, 1866). These sanitizers differ in scoped package handling:
sanitizeForFolder('@myorg/my-app')→'my-app'(strips scope)sanitizeForNpm('@myorg/my-app')→'@myorg/my-app'(preserves scope)
If the stdio branch becomes reachable, scoped packages would generate npx -y my-app but be published as @myorg/my-app, breaking the client connection.
🔧 Proposed fix
function renderFrontmcpConfigTemplate(projectName: string, deploymentTarget: DeploymentTarget): string {
const safeName = sanitizeForFolder(projectName);
+ const npmName = sanitizeForNpm(projectName);
const isHttpTarget =
deploymentTarget === 'node' ||
deploymentTarget === 'vercel' ||
deploymentTarget === 'lambda' ||
deploymentTarget === 'cloudflare';
const port = 3000;
const clientBlock = isHttpTarget
? ` clients: {
'claude-code': {
name: '${safeName}',
transport: 'http',
url: 'http://127.0.0.1:${port}/mcp',
},
},`
: ` clients: {
'claude-code': {
name: '${safeName}',
transport: 'stdio',
command: 'npx',
- args: ['-y', '${safeName}'],
+ args: ['-y', '${npmName}'],
},
},`;Also applies to: 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/commands/scaffold/create.ts` at line 178, The stdio client
branch uses sanitizeForFolder(projectName) to build the safeName for command
args, but scaffoldProject uses sanitizeForNpm(projectName) for package.json,
causing scoped names to diverge; change the stdio usage to use sanitizeForNpm so
both client invocation and published package name match. Locate the safeName
assignment (currently using sanitizeForFolder) in the stdio/stdio args code and
replace it with sanitizeForNpm(projectName) (and update the other occurrence
around line 198 similarly) so the stdio command uses the exact package name that
scaffoldProject will publish; ensure variable name and downstream uses remain
unchanged.
Comment on lines
+179
to
+183
| const isHttpTarget = | ||
| deploymentTarget === 'node' || | ||
| deploymentTarget === 'vercel' || | ||
| deploymentTarget === 'lambda' || | ||
| deploymentTarget === 'cloudflare'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for stdio transport references in config-related files
rg -nC3 "transport.*stdio|stdio.*transport" \
--type=ts \
--glob '!**/node_modules/**' \
libs/cli/src/config/ libs/cli/src/commands/
# Check if DeploymentTarget type is extended elsewhere
ast-grep --pattern 'type DeploymentTarget = $$$'Repository: agentfront/frontmcp
Length of output: 7037
🏁 Script executed:
#!/bin/bash
# Check what sanitizeForFolder and sanitizeForNpm do
rg -nA5 "export.*function.*sanitizeFor(Folder|Npm)" \
--type=ts \
libs/cli/
# Look at how the config template is used when rendered
sed -n '1720,1730p' libs/cli/src/commands/scaffold/create.ts
# Check if there's a package name handling mismatch
rg -nB2 -A2 "sanitizeForNpm|sanitizeForFolder" \
libs/cli/src/commands/scaffold/create.tsRepository: agentfront/frontmcp
Length of output: 2051
🏁 Script executed:
#!/bin/bash
# Verify the actual type at line 45 to ensure it's as stated
sed -n '40,50p' libs/cli/src/commands/scaffold/create.tsRepository: agentfront/frontmcp
Length of output: 529
Remove or make reachable the stdio transport branch — it's currently dead code.
The isHttpTarget condition at lines 179–183 checks all four possible DeploymentTarget values ('node' | 'vercel' | 'lambda' | 'cloudflare'), making it always true. The entire stdio client/transport block (lines 193–200, 203) is unreachable.
Either:
- Delete the unreachable stdio code, or
- If 'node' should support stdio for local desktop integration, change the condition and update the
DeploymentTargettype.
Note: If stdio is reintroduced, there's also a package name mismatch: the stdio command uses sanitizeForFolder() (which strips scope from scoped packages), while scaffoldProject() uses sanitizeForNpm() (which preserves it). For a scoped package like @myorg/my-app, the generated config would run npx -y my-app but declare name: "@myorg/my-app" in package.json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/commands/scaffold/create.ts` around lines 179 - 183, The stdio
client/transport branch is dead because isHttpTarget (used in create.ts) always
evaluates true for deploymentTarget ('node'|'vercel'|'lambda'|'cloudflare');
either remove the stdio block (delete the stdio client/transport code) or make
stdio reachable by changing isHttpTarget to exclude 'node' (or update the
DeploymentTarget type to add a distinct 'desktop' target) so the stdio branch
can execute; if you choose to reintroduce stdio, also fix the package-name
mismatch by using the same sanitization function as scaffoldProject (replace
sanitizeForFolder usage with sanitizeForNpm or unify both to one function) so
scoped packages like `@myorg/my-app` run and declare the correct name.
Comment on lines
+195
to
217
| try { | ||
| const mod = await import(configPath); | ||
| return mod.default ?? mod; | ||
| } catch { | ||
| // Fall through to esbuild. | ||
| } | ||
| try { | ||
| return await loadTsConfigViaEsbuild(configPath); | ||
| } catch (esbuildErr) { | ||
| throw new Error( | ||
| `Failed to load ${filename}.\n` + | ||
| ` require() error: ${requireErr?.message ?? '(skipped)'}\n` + | ||
| ` esbuild error: ${(esbuildErr as Error).message}\n` + | ||
| `Hint: ensure the file exports a default config (e.g., ` + | ||
| `\`export default defineConfig({...})\`) and that all imports resolve.`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const mod = require(configPath); | ||
| // JS/MJS/CJS | ||
| if (filename.endsWith('.mjs')) { | ||
| const mod = await import(configPath); | ||
| return mod.default ?? mod; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does dynamic import()accept an absolute Windows filesystem path likeC:\project\frontmcp.config.mjsdirectly, or should it be converted withpathToFileURL() first?
💡 Result:
In Node.js, dynamic import does not accept absolute Windows filesystem paths (e.g., C:\project\frontmcp.config.mjs) directly [1][2]. Node.js ESM import specifiers are required to be valid URLs [1][3]. To dynamically import a file using an absolute path, you must convert the path into a file:// URL using the pathToFileURL function from the node:url module [1][4][2]. Recommended approach: import { pathToFileURL } from 'node:url'; const absolutePath = 'C:\project\frontmcp.config.mjs'; const fileUrl = pathToFileURL(absolutePath).href; const module = await import(fileUrl); Using pathToFileURL ensures that the path is correctly encoded and formatted as a valid URL, which is necessary for cross-platform compatibility and to satisfy the requirements of the ESM loader [1][5][2].
Citations:
- 1: Absolute Windows paths are handled as invalid URL path in ESM import() nodejs/node#34765
- 2: ES Module loading with abolute path fails on windows nodejs/node#31710
- 3: Absolute Windows paths are handled as invalid URL path in ESM import() nodejs/node#34765
- 4: https://nodejs.org/download/release/v22.4.1/docs/api/module.html
- 5: https://wohugb.github.io/nodejs/esm/import-specifiers
Use file URLs for dynamic imports.
import(configPath) works on POSIX systems but fails on Windows—absolute Windows paths like C:\project\frontmcp.config.mjs are not valid ESM specifiers. Node.js requires a file:// URL. Both .ts and .mjs branches need fixing.
Fix
+import { pathToFileURL } from 'node:url';
+
...
- const mod = await import(configPath);
+ const mod = await import(pathToFileURL(configPath).href);
return mod.default ?? mod;
...
- const mod = await import(configPath);
+ const mod = await import(pathToFileURL(configPath).href);
return mod.default ?? mod;📝 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
| try { | |
| const mod = await import(configPath); | |
| return mod.default ?? mod; | |
| } catch { | |
| // Fall through to esbuild. | |
| } | |
| try { | |
| return await loadTsConfigViaEsbuild(configPath); | |
| } catch (esbuildErr) { | |
| throw new Error( | |
| `Failed to load ${filename}.\n` + | |
| ` require() error: ${requireErr?.message ?? '(skipped)'}\n` + | |
| ` esbuild error: ${(esbuildErr as Error).message}\n` + | |
| `Hint: ensure the file exports a default config (e.g., ` + | |
| `\`export default defineConfig({...})\`) and that all imports resolve.`, | |
| ); | |
| } | |
| } | |
| const mod = require(configPath); | |
| // JS/MJS/CJS | |
| if (filename.endsWith('.mjs')) { | |
| const mod = await import(configPath); | |
| return mod.default ?? mod; | |
| try { | |
| const mod = await import(pathToFileURL(configPath).href); | |
| return mod.default ?? mod; | |
| } catch { | |
| // Fall through to esbuild. | |
| } | |
| try { | |
| return await loadTsConfigViaEsbuild(configPath); | |
| } catch (esbuildErr) { | |
| throw new Error( | |
| `Failed to load ${filename}.\n` + | |
| ` require() error: ${requireErr?.message ?? '(skipped)'}\n` + | |
| ` esbuild error: ${(esbuildErr as Error).message}\n` + | |
| `Hint: ensure the file exports a default config (e.g., ` + | |
| `\`export default defineConfig({...})\`) and that all imports resolve.`, | |
| ); | |
| } | |
| } | |
| // JS/MJS/CJS | |
| if (filename.endsWith('.mjs')) { | |
| const mod = await import(pathToFileURL(configPath).href); | |
| return mod.default ?? mod; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/config/frontmcp-config.loader.ts` around lines 195 - 217,
Dynamic imports use a plain filesystem path which fails on Windows; convert
configPath to a file:// URL before calling import in both the early TS branch
(where you do const mod = await import(configPath)) and the .mjs branch (where
you do const mod = await import(configPath)); import and use pathToFileURL from
'url' and pass pathToFileURL(configPath).href (or the URL object) to import() so
Windows absolute paths are valid ESM specifiers, keeping the existing
mod.default ?? mod behavior and error handling around loadTsConfigViaEsbuild and
requireErr unchanged.
Comment on lines
+101
to
+104
| const cliEnv = | ||
| typeof options.cliOptions?.['env'] === 'object' && options.cliOptions['env'] !== null | ||
| ? (options.cliOptions['env'] as Record<string, string>) | ||
| : {}; |
Contributor
There was a problem hiding this comment.
Validate cliOptions.env before merging it.
The cast to Record<string, string> is unchecked. A numeric or boolean value can flow straight into effectiveEnv even though this API promises string env values.
🛠️ Proposed fix
const cliEnv =
typeof options.cliOptions?.['env'] === 'object' && options.cliOptions['env'] !== null
- ? (options.cliOptions['env'] as Record<string, string>)
+ ? Object.fromEntries(
+ Object.entries(options.cliOptions['env'] as Record<string, unknown>).filter(
+ (entry): entry is [string, string] => typeof entry[1] === 'string',
+ ),
+ )
: {};Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/config/frontmcp-config.resolve.ts` around lines 101 - 104, The
current unchecked cast of options.cliOptions?.['env'] to Record<string,string>
can allow non-string values; replace it by building cliEnv from
Object.entries(options.cliOptions?.['env'] ?? {}) and only include keys whose
values satisfy typeof value === 'string' (e.g., filter or reduce to a
Record<string,string>), so cliEnv becomes a safe Record<string,string> of only
string values before merging into effectiveEnv; apply the same validation logic
to the other occurrence referenced around the symbol on line ~144.
Comment on lines
+112
to
+129
| if (explicitPath) { | ||
| try { | ||
| config = await loadFrontMcpConfigFromFile(explicitPath); | ||
| configPath = explicitPath; | ||
| } catch (err) { | ||
| throw new Error(`Failed to load config from "${explicitPath}": ${(err as Error).message}`); | ||
| } | ||
| } else { | ||
| configDir = findConfigDir(options.cwd); | ||
| if (configDir) { | ||
| try { | ||
| config = await loadFrontMcpConfig(configDir); | ||
| } catch (err) { | ||
| // Surface schema/load errors — silent fallback was the corruption | ||
| // mode #365 worked to eliminate. The legacy-shape branch in | ||
| // `tryLoadFrontMcpConfig` handles old configs separately. | ||
| throw new Error(`Failed to load frontmcp.config in ${configDir}: ${(err as Error).message}`); | ||
| } |
Contributor
There was a problem hiding this comment.
The legacy-config fallback is bypassed here.
Both branches call hard-validating loaders, so an old exec-only config now throws instead of returning { config: undefined } as the comments in this file and tryLoadFrontMcpConfig() describe. If backward compatibility is still intended, this needs the non-throwing path too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/config/frontmcp-config.resolve.ts` around lines 112 - 129, The
current explicitPath and configDir branches call hard-validating loaders
(loadFrontMcpConfigFromFile and loadFrontMcpConfig) and throw on errors,
bypassing the legacy-config fallback; change these branches to use the
non-throwing path in tryLoadFrontMcpConfig (or catch the loader error and return
{ config: undefined } instead of throwing) so that exec-only/legacy configs
produce a graceful undefined config rather than an exception; ensure you still
set configPath when explicitPath succeeds and preserve the existing error
surfacing for real schema/load failures only when tryLoadFrontMcpConfig
indicates it’s not a legacy shape.
Comment on lines
+112
to
+118
| if (explicitPath) { | ||
| try { | ||
| config = await loadFrontMcpConfigFromFile(explicitPath); | ||
| configPath = explicitPath; | ||
| } catch (err) { | ||
| throw new Error(`Failed to load config from "${explicitPath}": ${(err as Error).message}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Populate configDir and normalize configPath for explicit configs.
This branch returns the caller-supplied string verbatim and leaves configDir unset, even though ResolvedFrontMcpConfig documents an absolute path plus containing directory. Callers will get different metadata depending on how the config was found.
🛠️ Proposed fix
+import * as path from 'path';
+
...
if (explicitPath) {
try {
- config = await loadFrontMcpConfigFromFile(explicitPath);
- configPath = explicitPath;
+ configPath = path.isAbsolute(explicitPath) ? explicitPath : path.resolve(options.cwd, explicitPath);
+ configDir = path.dirname(configPath);
+ config = await loadFrontMcpConfigFromFile(configPath);
} catch (err) {
throw new Error(`Failed to load config from "${explicitPath}": ${(err as Error).message}`);
}
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/config/frontmcp-config.resolve.ts` around lines 112 - 118, The
explicitPath branch currently returns the caller-supplied string and leaves
configDir unset; update the branch that calls
loadFrontMcpConfigFromFile(explicitPath) to normalize/resolve configPath to an
absolute path and populate configDir with its parent directory before returning
a ResolvedFrontMcpConfig. Concretely, after successful load in the explicitPath
branch (the block using loadFrontMcpConfigFromFile and setting configPath =
explicitPath), replace the verbatim assignment with logic that resolves
explicitPath to an absolute path (e.g., via path.resolve or equivalent), set
configPath to that normalized absolute path, and set configDir to
path.dirname(configPath) so the returned ResolvedFrontMcpConfig always contains
a canonical absolute configPath and its containing directory.
Comment on lines
+382
to
+390
| export const clientConnectionSchema = z | ||
| .object({ | ||
| name: z.string().optional(), | ||
| transport: z.enum(['http', 'sse', 'stdio']), | ||
| command: z.string().optional(), | ||
| args: z.array(z.string()).optional(), | ||
| env: z.record(z.string(), z.string()).optional(), | ||
| url: z.string().optional(), | ||
| }) |
Contributor
There was a problem hiding this comment.
Validate clients[*].url when it is provided.
Right now any string passes here, so a malformed endpoint survives config parsing and only breaks when the emitted snippet is used.
🛠️ Proposed fix
export const clientConnectionSchema = z
.object({
name: z.string().optional(),
transport: z.enum(['http', 'sse', 'stdio']),
command: z.string().optional(),
args: z.array(z.string()).optional(),
env: z.record(z.string(), z.string()).optional(),
- url: z.string().optional(),
+ url: z.string().url().optional(),
})
.strict();📝 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
| export const clientConnectionSchema = z | |
| .object({ | |
| name: z.string().optional(), | |
| transport: z.enum(['http', 'sse', 'stdio']), | |
| command: z.string().optional(), | |
| args: z.array(z.string()).optional(), | |
| env: z.record(z.string(), z.string()).optional(), | |
| url: z.string().optional(), | |
| }) | |
| export const clientConnectionSchema = z | |
| .object({ | |
| name: z.string().optional(), | |
| transport: z.enum(['http', 'sse', 'stdio']), | |
| command: z.string().optional(), | |
| args: z.array(z.string()).optional(), | |
| env: z.record(z.string(), z.string()).optional(), | |
| url: z.string().url().optional(), | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cli/src/config/frontmcp-config.schema.ts` around lines 382 - 390, The
clientConnectionSchema currently lets any string through for the url field;
update the schema so provided URLs are validated by Zod. Replace the url:
z.string().optional() entry in clientConnectionSchema with url:
z.string().url().optional() (or z.string().url({ message: "invalid url"
}).optional() if you want a custom message) so clients[*].url is syntactically
validated at parse time.
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
Release Notes
New Features
frontmcp eject-mcp-configcommand to generate MCP client configuration snippets for supported clients.Improvements
dev,test, andinspectorcommands to consume project configuration.