fix: enhance configuration resolution and client snippet generation for frontmcp#430
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR ( ChangesConfig Resolution & Command Integration
Tests, Documentation, and Build Utilities
Sequence DiagramsequenceDiagram
participant UserCLI as User CLI
participant Program as createProgram
participant resolveConfig as resolveConfig
participant Loader as frontmcp-config.loader
participant FS as Filesystem
UserCLI->>Program: run command with --config?/env/opts
Program->>resolveConfig: resolve({ configPath?, cwd, mode, cliOptions })
resolveConfig->>Loader: tryLoadFrontMcpConfigFromFile or findConfigDir
Loader->>FS: read/load candidate file (json/ts/mjs/js)
Loader-->>resolveConfig: parsed config or undefined
resolveConfig->>resolveConfig: compose effectiveEnv (process.env ⊕ config.env.shared ⊕ config.env[mode] ⊕ cliOptions.env)
resolveConfig-->>Program: ResolvedFrontMcpConfig (config?, configDir, configPath, effectiveEnv)
Program-->>UserCLI: command runs using resolved config and environment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/eject/__tests__/mcp-client.spec.ts`:
- Around line 30-33: Update the throw-path tests to also assert the error class
(use instanceof checks) in addition to the message pattern: instead of only
using expect(() => emitClientSnippet(...)).toThrow(/.../), execute the call
inside a try/catch (or capture the thrown value) and assert both
expect(err).toBeInstanceOf(Error) (or the specific error constructor used in the
module) and expect(err.message).toMatch(/no `clients\.claude-code` entry/);
apply the same pattern to the other throw-path tests referenced (around the
blocks using emitClientSnippet and the other failing cases at the regions noted)
so every test validates both the error class and the message.
🪄 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: 214bcf22-bc6e-419d-8765-f277aece3007
📒 Files selected for processing (14)
.gitignorelibs/cli/src/commands/dev/test.tslibs/cli/src/commands/eject/__tests__/mcp-client.spec.tslibs/cli/src/commands/eject/mcp-client.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/core/program.tslibs/utils/src/index.tslibs/utils/src/path/browser-path.tslibs/utils/src/path/index.tslibs/utils/src/path/node-path.ts
✅ Files skipped from review due to trivial changes (3)
- libs/utils/src/path/index.ts
- .gitignore
- libs/utils/src/index.ts
…sport configurations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/cli/src/commands/dev/inspector.ts (1)
52-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
resolved.effectiveEnvwhen spawning Inspector.
resolveConfig({ mode: 'inspector' })computes env overlays, but they are never applied to the spawnednpxprocess. In stdio mode, this can break server startup when config-driven env vars are required.Proposed fix
- await runCmd('npx', args); + await runCmd('npx', args, { env: resolved.effectiveEnv });If
runCmdtyping currently only allows{ cwd?: string }, please widen its options type to includeenv?: NodeJS.ProcessEnvso this call remains type-safe.🤖 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/inspector.ts` around lines 52 - 64, resolveConfig(...) computes environment overlays into resolved.effectiveEnv but the spawned Inspector process isn't receiving them; update the run of the inspector so that runCmd('npx', args) is called with the resolved.effectiveEnv passed as the env option (e.g., runCmd('npx', args, { env: resolved.effectiveEnv, cwd: process.cwd() })), and if runCmd's options type only includes cwd extend its signature/type to accept env?: NodeJS.ProcessEnv so the call remains type-safe; locate usages around buildInspectorArgs, resolveConfig, and the runCmd invocation to apply the change.
🤖 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/inspector.ts`:
- Around line 31-33: Normalize transport.http.path to always start with a single
leading slash before composing the server URL: when computing mountPath (used in
the args.push call that builds
`http://${host}:${transport.http.port}${mountPath}`) ensure you prepend a '/' if
transport.http.path is present without one and strip any duplicate slashes so
you don't produce `//`; apply the same normalization logic wherever
transport.http.path is used (including the SSE branch guarded by `else if
(transport?.default === 'sse' && transport.http?.port)`), then use the
normalized mountPath when pushing `--server-url`.
---
Outside diff comments:
In `@libs/cli/src/commands/dev/inspector.ts`:
- Around line 52-64: resolveConfig(...) computes environment overlays into
resolved.effectiveEnv but the spawned Inspector process isn't receiving them;
update the run of the inspector so that runCmd('npx', args) is called with the
resolved.effectiveEnv passed as the env option (e.g., runCmd('npx', args, { env:
resolved.effectiveEnv, cwd: process.cwd() })), and if runCmd's options type only
includes cwd extend its signature/type to accept env?: NodeJS.ProcessEnv so the
call remains type-safe; locate usages around buildInspectorArgs, resolveConfig,
and the runCmd invocation to apply the change.
🪄 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: 516b55f0-de63-4e1d-8749-1a4ca07a677e
📒 Files selected for processing (2)
libs/cli/src/commands/dev/__tests__/inspector.spec.tslibs/cli/src/commands/dev/inspector.ts
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-05-24T17:57:29.187Z |
# Conflicts: # libs/cli/src/commands/dev/dev.ts # libs/cli/src/core/program.ts
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