fix: Fix latest 1.1.x reported bugs#383
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughPreserves original errors in FlowControl, adds non-finite-number output validation (opt-out via metadata), returns JSON‑RPC error envelopes for uninitialized sessions, enhances TypeScript config loading with require→import→esbuild fallback plus tryLoad API, switches decorator metadata access to a helper, tweaks CLI spawn/port handling, and adds tests and a divide tool fixture. ChangesError Handling, Output Validation, Session Routing, Config & CLI Wiring
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPRouter
participant AuthVerifier
participant JSONRPCResponder
Client->>HTTPRouter: POST /tools/list (JSON-RPC, no initialize)
HTTPRouter->>AuthVerifier: verify / decide intent
AuthVerifier-->>HTTPRouter: decision.intent == "unknown"
HTTPRouter->>JSONRPCResponder: respondNoSession(id)
JSONRPCResponder-->>Client: 200 OK\nContent-Type: application/json\nJSON-RPC error (code -32600)
sequenceDiagram
participant CallToolFlow
participant OutputParser
participant NonFiniteChecker
participant ConfigMetadata
CallToolFlow->>OutputParser: parse tool raw output
OutputParser-->>CallToolFlow: parsed data
CallToolFlow->>ConfigMetadata: read scope.metadata.output.allowNonFinite
ConfigMetadata-->>CallToolFlow: allowNonFinite (true|false)
CallToolFlow->>NonFiniteChecker: findNonFiniteNumber(parsed)
alt non-finite found and not allowed
NonFiniteChecker-->>CallToolFlow: {path, value}
CallToolFlow->>CallToolFlow: throw InvalidOutputError(details)
else allowed or none found
NonFiniteChecker-->>CallToolFlow: undefined
CallToolFlow-->>Client: success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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: 6
🧹 Nitpick comments (1)
libs/sdk/src/tool/flows/call-tool.flow.ts (1)
1228-1243: 💤 Low valueConsider including the path in the error message for easier debugging.
The logging captures
pathandvalue, butInvalidOutputError()is thrown without context. Users seeing this error won't know which field caused the failure without checking logs.💡 Optional: include path in error
throw new InvalidOutputError(); + throw new InvalidOutputError(`Non-finite number at '${found.path}'`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/tool/flows/call-tool.flow.ts` around lines 1228 - 1243, The InvalidOutputError thrown in the allowNonFinite check lacks context; update the throw to include the offending field path (and optionally value) so callers can surface it without checking logs. In the block using allowNonFinite, when findNonFiniteNumber(parseResult.data) returns found, construct/throw an InvalidOutputError that includes found.path (and/or String(found.value)) in its message or properties (same area where this.logger.error is called), and ensure the change references the existing symbols allowNonFinite, findNonFiniteNumber, and InvalidOutputError so the error preserves the same semantics while carrying path info.
🤖 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/cli/src/commands/package/install.ts`:
- Around line 125-127: The port fallback currently uses the logical OR which
treats 0 as falsy; change the fallback in the port assignment to use the nullish
coalescing operator so an explicit --port 0 is preserved: update the statement
that defines port (currently using opts.port ||
manifestData.network?.defaultPort) to use opts.port ??
manifestData.network?.defaultPort so only null/undefined fall back to
manifestData.network?.defaultPort.
In `@libs/cli/src/config/__tests__/frontmcp-config.loader.spec.ts`:
- Around line 1-3: The tests use synchronous fs calls; change them to the async
helpers from `@frontmcp/utils` by importing mkdtemp, writeFile, and rm and
replacing fs.mkdtempSync, fs.writeFileSync, and fs.rmSync accordingly; make the
helper function makeProject async (and any helper callers in this spec) and
update all test invocations to await makeProject, and ensure cleanup uses await
rm(...) with proper options to mirror the previous sync behavior.
In `@libs/cli/src/config/frontmcp-config.loader.ts`:
- Around line 103-104: Replace direct fs usage for reading the config by
importing and using the filesystem wrapper from `@frontmcp/utils`: remove or stop
using fs.readFileSync(configPath, 'utf-8') and instead import readFileSync from
'@frontmcp/utils' and call readFileSync(configPath, 'utf-8') where the variable
source is assigned; keep the existing esbuild variable and configPath unchanged
so only the import and the call to readFileSync change.
- Around line 102-110: Replace the direct fs.readFileSync usage in
loadTsConfigViaEsbuild with the readFileSync export from `@frontmcp/utils` and
change the esbuild.transformSync call to an async esbuild.build call that
bundles dependencies; specifically, import/read via readFileSync from
`@frontmcp/utils` instead of fs, convert loadTsConfigViaEsbuild to await
esbuild.build({ entryPoints: [configPath], bundle: true, write: false, platform:
'node', packages: 'external', target: 'es2022' }) (or equivalent options) so
sibling .ts helpers are bundled into the output, and return/parse the built
output instead of the transformSync result.
In `@libs/sdk/src/errors/mcp.error.ts`:
- Around line 584-613: The function extractPublicMessage can recurse
indefinitely on cyclic error chains; add a visited-object guard (e.g., a Set
passed as an optional parameter) to track objects already seen and short-circuit
when revisiting one. Update extractPublicMessage to accept an internal visited
Set, add the current error to it before exploring originalError or cause, and on
any recursive call pass the same Set; if a candidate inner/cause is already in
visited, skip it and fall back to the next option (or return 'Unknown error') to
avoid stack overflows. Ensure checks reference the existing symbols:
extractPublicMessage, PublicMcpError, McpError, originalError, and cause so all
recursive paths use the guard.
In `@libs/sdk/src/scope/flows/http.request.flow.ts`:
- Around line 285-302: Change the helper respondNoSession to return void (remove
the unconditional throw new Error('unreachable')) and keep the JSON response
behavior; then update each call site that invokes respondNoSession() in this
flow (the three session-check locations where it was previously used) to add an
explicit return; immediately after the respondNoSession() call so control does
not fall through to this.next(). Ensure the helper signature and all call sites
compile after making respondNoSession return void.
---
Nitpick comments:
In `@libs/sdk/src/tool/flows/call-tool.flow.ts`:
- Around line 1228-1243: The InvalidOutputError thrown in the allowNonFinite
check lacks context; update the throw to include the offending field path (and
optionally value) so callers can surface it without checking logs. In the block
using allowNonFinite, when findNonFiniteNumber(parseResult.data) returns found,
construct/throw an InvalidOutputError that includes found.path (and/or
String(found.value)) in its message or properties (same area where
this.logger.error is called), and ensure the change references the existing
symbols allowNonFinite, findNonFiniteNumber, and InvalidOutputError so the error
preserves the same semantics while carrying path info.
🪄 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: 06cc9435-4ca5-45bb-9996-d9f4eb4b6249
⛔ Files ignored due to path filters (18)
libs/cli/src/commands/build/__tests__/adapters.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/adapters/cloudflare.tsis excluded by!**/build/**libs/cli/src/commands/build/adapters/distributed.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/__tests__/build-exec-cli.integration.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/__tests__/generate-cli-entry.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/__tests__/installer-script.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/__tests__/manifest.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/__tests__/runner-script.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/generate-cli-entry.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/schema-extractor.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/index.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/installer-script.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/manifest.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/runner-script.tsis excluded by!**/build/**libs/cli/src/commands/build/index.tsis excluded by!**/build/**libs/cli/src/commands/build/mcpb/__tests__/manifest.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/mcpb/manifest.tsis excluded by!**/build/**libs/cli/src/commands/build/types.tsis excluded by!**/build/**
📒 Files selected for processing (24)
apps/e2e/demo-e2e-cli-exec/e2e/cli-errors.e2e.spec.tsapps/e2e/demo-e2e-cli-exec/e2e/cli-prompts.e2e.spec.tsapps/e2e/demo-e2e-elicitation/e2e/streamable-http-transport.e2e.spec.tslibs/cli/package.jsonlibs/cli/src/commands/dev/__tests__/doctor.spec.tslibs/cli/src/commands/dev/doctor.tslibs/cli/src/commands/package/install.tslibs/cli/src/commands/package/types.tslibs/cli/src/config/__tests__/frontmcp-config.loader.spec.tslibs/cli/src/config/frontmcp-config.loader.tslibs/sdk/src/common/interfaces/flow.interface.tslibs/sdk/src/common/metadata/__tests__/parse-lite.spec.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/direct/direct-server.tslibs/sdk/src/errors/index.tslibs/sdk/src/errors/mcp.error.tslibs/sdk/src/scope/flows/__tests__/http.request.unknown-intent.spec.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/utils/src/content/content.spec.tslibs/utils/src/content/content.tslibs/utils/src/content/index.tslibs/utils/src/index.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/cli/src/config/frontmcp-config.loader.ts`:
- Around line 71-76: When frontmcpConfigSchema.safeParse(raw) fails, don't
always return undefined; instead detect the legacy exec-only shape (e.g., raw
has top-level "cli" or "sea") and only then return undefined so loadExecConfig
can handle it, otherwise throw the validation error. Update the loader around
frontmcpConfigSchema.safeParse(raw) to inspect raw for legacy keys ("cli" or
"sea") and if absent throw result.error (or rethrow) rather than returning
undefined.
🪄 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: d2be9fc4-57f6-4ef7-b05c-b3062994c6e3
⛔ Files ignored due to path filters (10)
libs/cli/src/commands/build/exec/__tests__/build-exec-cli.integration.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/__tests__/extract-public-message-parity.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/extract-public-message.snippet.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/generate-cli-entry.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/schema-extractor.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/index.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/installer-script.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/runner-script.tsis excluded by!**/build/**libs/cli/src/commands/build/index.tsis excluded by!**/build/**libs/cli/src/commands/build/load-entry-config.tsis excluded by!**/build/**
📒 Files selected for processing (13)
CLAUDE.mdapps/e2e/demo-e2e-cli-exec/fixture/src/apps/cli-exec/index.tsapps/e2e/demo-e2e-cli-exec/fixture/src/apps/cli-exec/tools/divide.tool.tslibs/cli/src/commands/package/install.tslibs/cli/src/config/__tests__/frontmcp-config.loader.spec.tslibs/cli/src/config/frontmcp-config.loader.tslibs/cli/src/config/index.tslibs/sdk/src/common/decorators/front-mcp.decorator.tslibs/sdk/src/direct/connect.tslibs/sdk/src/errors/mcp.error.tslibs/sdk/src/esm-loader/esm-manifest.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/tool/flows/call-tool.flow.ts
✅ Files skipped from review due to trivial changes (4)
- libs/cli/src/config/index.ts
- apps/e2e/demo-e2e-cli-exec/fixture/src/apps/cli-exec/tools/divide.tool.ts
- CLAUDE.md
- libs/sdk/src/scope/flows/http.request.flow.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/cli/src/commands/package/install.ts
- libs/sdk/src/tool/flows/call-tool.flow.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-03T00:54:30.646Z |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/e2e/demo-e2e-standalone/e2e/standalone.e2e.spec.ts (1)
282-285: Extract the JSON-RPC error envelope into a shared interface instead of duplicating the inline type cast.The same response shape appears at lines 282-285 and 300-303. Extracting a single
JsonRpcErrorEnvelopeinterface improves consistency and aligns with the typing guidelines preferring interfaces for object shapes.Suggested refactor
import { expect, McpTestClient, test } from '@frontmcp/testing'; + +interface JsonRpcErrorEnvelope { + jsonrpc?: string; + error?: { code?: number; message?: string }; +} @@ - const rootBody = (await rootResponse.json()) as { - jsonrpc?: string; - error?: { code?: number; message?: string }; - }; + const rootBody = (await rootResponse.json()) as JsonRpcErrorEnvelope; @@ - const isolatedBody = (await isolatedResponse.json()) as { - jsonrpc?: string; - error?: { code?: number; message?: string }; - }; + const isolatedBody = (await isolatedResponse.json()) as JsonRpcErrorEnvelope;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-standalone/e2e/standalone.e2e.spec.ts` around lines 282 - 285, Extract the duplicated inline type used for rootResponse.json() into a shared interface named e.g. JsonRpcErrorEnvelope and use it for both rootBody and the later response variable; locate the two inline casts around rootBody and the similar cast at the later response (the places currently typing as { jsonrpc?: string; error?: { code?: number; message?: string } }) and replace them with the interface, updating imports or file-scope declarations as needed so both usages reference JsonRpcErrorEnvelope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/e2e/demo-e2e-standalone/e2e/standalone.e2e.spec.ts`:
- Around line 282-285: Extract the duplicated inline type used for
rootResponse.json() into a shared interface named e.g. JsonRpcErrorEnvelope and
use it for both rootBody and the later response variable; locate the two inline
casts around rootBody and the similar cast at the later response (the places
currently typing as { jsonrpc?: string; error?: { code?: number; message?:
string } }) and replace them with the interface, updating imports or file-scope
declarations as needed so both usages reference JsonRpcErrorEnvelope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88b6be0a-5e0a-4b96-b177-af95478714f2
📒 Files selected for processing (3)
apps/e2e/demo-e2e-standalone/e2e/standalone.e2e.spec.tslibs/cli/src/config/__tests__/frontmcp-config.loader.spec.tslibs/cli/src/config/frontmcp-config.loader.ts
✅ Files skipped from review due to trivial changes (1)
- libs/cli/src/config/frontmcp-config.loader.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/cli/src/config/tests/frontmcp-config.loader.spec.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 @.nycrc.json:
- Around line 20-24: The patch lowers the global coverage thresholds in the
.nycrc.json (keys: "check-coverage", "statements", "branches", "functions",
"lines") to 60/49/58/60 which weakens the CI quality gate; restore those numeric
thresholds back to the repo standard (e.g., 95 for statements/lines/functions
and an appropriate branches value like 95) and set "check-coverage": true, or if
you truly need a temporary relaxation, add a clear comment in the PR and create
a documented exception (e.g., a separate config or README note) explaining why
and when it will be reverted.
🪄 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: f4ab1d44-1119-457d-aafb-3ec448b956e4
📒 Files selected for processing (2)
.github/workflows/push.yml.nycrc.json
Comment on lines
20
to
+24
| "check-coverage": true, | ||
| "statements": 95, | ||
| "branches": 95, | ||
| "functions": 95, | ||
| "lines": 95 | ||
| "statements": 60, | ||
| "branches": 49, | ||
| "functions": 58, | ||
| "lines": 60 |
Contributor
There was a problem hiding this comment.
Don't lower the global coverage floor.
Dropping the enforced thresholds to 60/49/58/60 materially weakens the repo's quality gate and can let coverage regressions pass CI. If the current suite is below target, fix the coverage gap or scope a temporary, documented exception instead.
🔧 Suggested fix
- "statements": 60,
- "branches": 49,
- "functions": 58,
- "lines": 60
+ "statements": 95,
+ "branches": 95,
+ "functions": 95,
+ "lines": 95Based on learnings, Never commit code with test failures, build warnings, or coverage below 95%.
📝 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
| "check-coverage": true, | |
| "statements": 95, | |
| "branches": 95, | |
| "functions": 95, | |
| "lines": 95 | |
| "statements": 60, | |
| "branches": 49, | |
| "functions": 58, | |
| "lines": 60 | |
| "check-coverage": true, | |
| "statements": 95, | |
| "branches": 95, | |
| "functions": 95, | |
| "lines": 95 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.nycrc.json around lines 20 - 24, The patch lowers the global coverage
thresholds in the .nycrc.json (keys: "check-coverage", "statements", "branches",
"functions", "lines") to 60/49/58/60 which weakens the CI quality gate; restore
those numeric thresholds back to the repo standard (e.g., 95 for
statements/lines/functions and an appropriate branches value like 95) and set
"check-coverage": true, or if you truly need a temporary relaxation, add a clear
comment in the PR and create a documented exception (e.g., a separate config or
README note) explaining why and when it will be reverted.
Contributor
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
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
Bug Fixes
Tests
Documentation