fix: enhance configuration resolution and client snippet generation for frontmcp#426
Merged
Conversation
7 tasks
Contributor
|
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 (8)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds a watch-aware stdio JSON-RPC bridge for frontmcp dev: newline-delimited framing, buffering/reload-aware state machine, upstream transports (HTTP/pipe), child supervision, file watcher, structured logging, CLI flags, tests, and docs. ChangesStdio Bridge Implementation
Sequence DiagramsequenceDiagram
participant Client as Stdio Client
participant Framer as StdioFramer
participant FSM as BridgeStateMachine
participant Upstream as UpstreamClient
participant Child as ChildProcess
Client->>Framer: stdin newline-delimited JSON-RPC
Framer->>FSM: enqueue(frame)
FSM->>Upstream: forward(frame) (when Ready)
Upstream->>Child: HTTP POST or IPC send
Child->>Upstream: JSON-RPC response / SSE event
Upstream->>FSM: onFrame(response)
FSM->>Framer: respond(response)
Framer->>Client: stdout newline JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 11
🧹 Nitpick comments (3)
libs/cli/src/commands/dev/bridge/__tests__/state-machine.spec.ts (1)
106-122: ⚡ Quick winAdd a direct
onReloadDeadline()contract test.Current suite validates the timer callback path, but not the public
onReloadDeadline()method directly. Adding that assertion would lock in-32097behavior and prevent regressions.Suggested test case
+ it('onReloadDeadline() flushes buffered requests with -32097', async () => { + const { fsm, rec } = makeFsm(); + fsm.onBootStart(); + await fsm.enqueue({ jsonrpc: '2.0', id: 77, method: 'wait' }); + fsm.onReloadDeadline(); + await Promise.resolve(); + expect(rec.responses.find((r) => r.id === 77 && r.error?.code === -32097)).toBeDefined(); + });🤖 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/bridge/__tests__/state-machine.spec.ts` around lines 106 - 122, Add a direct unit test that calls the public onReloadDeadline() method instead of advancing timers: create the FSM via makeFsm({ reloadDeadlineMs: 1000 }), drive it to the Reloading state (call fsm.onBootStart(), fsm.onChildReady(), fsm.onWatcherEvent('src/foo.ts')), enqueue a buffered request (await fsm.enqueue({ jsonrpc:'2.0', id:5, method:'wait' })), then call fsm.onReloadDeadline() and assert fsm.state === 'Degraded' and that rec.responses contains an entry with id === 5 and error.code === -32097 to lock in the contract.libs/cli/src/commands/dev/bridge/index.ts (1)
160-165: ⚡ Quick winDrop the non-null assertion on
supervisor.
supervisoris a localconstinitialized immediately above, so!only weakens strict typing here.Suggested fix
- await supervisor!.restart(); + await supervisor.restart();As per coding guidelines, "Avoid non-null assertions (
!); instead use proper error handling with specific error classes when values are not found".🤖 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/bridge/index.ts` around lines 160 - 165, The callback passed to onChange uses a non-null assertion on supervisor (supervisor!.restart()); remove the `!` and add a runtime check before calling restart: inside onChange (and the surrounding async IIFE) verify supervisor is defined (e.g., if (!supervisor) { /* log or throw a specific Error class */ return; }) and then call await supervisor.restart(); also keep the existing try/catch around restart and surface a clear error (or throw a specific error class) instead of relying on the non-null assertion; this change touches the onChange handler and the supervisor variable usage so locate supervisor, onChange, fsm.onWatcherEvent, and the restart call to implement the check.libs/cli/src/commands/dev/bridge/stdio-framer.ts (1)
67-67: ⚖️ Poor tradeoffType assertions bypass structural validation.
Lines 67 and 74 use
as JsonRpcFramewithout runtime validation of required fields. While this prioritizes performance, consider documenting that the framer trusts upstream to send well-formed JSON-RPC frames, or add optional validation for strictness.Also applies to: 74-74
🤖 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/bridge/stdio-framer.ts` at line 67, The code currently casts error payloads using "as JsonRpcFrame" when calling write(makeDevError(...)) (see makeDevError and write invocations), which bypasses runtime structural validation; update the framer to either (A) add a lightweight runtime guard that verifies required JsonRpcFrame fields before calling write (and fall back to a safe default or log+drop when invalid), or (B) if you intentionally trust upstream, add an explicit code comment and JSDoc on makeDevError/write that documents the trust boundary so the assertion is intentional; implement one of these options and reference the makeDevError and write call sites to locate 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/bridge/__tests__/stdio-framer.spec.ts`:
- Around line 21-111: Add three tests to cover backpressure/drain, CRLF
normalization, and non-object-JSON branches: use createStdioFramer and
framer.start() as in existing tests; for backpressure create a PassThrough
output with a small highWaterMark, call framer.write(...) with a large payload
and assert the returned promise does not resolve until you call output.read()
and the 'drain' cycle runs; for CRLF write a JSON line terminated with '\r\n'
and assert onFrame receives the parsed object; for non-object JSON write valid
non-object JSON lines (e.g., 42 and "string") and assert no frames are emitted
and that the framer/log records parse-error/not_object entries (use makeLog to
capture logger lines).
In `@libs/cli/src/commands/dev/bridge/index.ts`:
- Around line 156-158: The watcher is being rooted at
path.dirname(path.resolve(runtime.entry)) which misses changes above that
directory; update the root passed to createDevWatcher so it watches the project
root (e.g., use process.cwd() or an existing projectRoot variable) instead of
watchRoot derived from runtime.entry; locate the code around watchRoot,
runtime.entry, createDevWatcher and replace the rootDir value so the watcher
covers the full project root to capture shared files and top-level config
changes.
- Around line 138-140: The onExit handler currently fires-and-forgets
upstream?.close(), which can cause unhandled rejections; change the handler to
properly await or handle the promise from upstream.close() (e.g., make the
onExit callback async and await upstream.close(), or call
upstream.close().catch(err => /* log/error handle */)) and only set upstream =
undefined after the close completes or the error is handled; update the onExit
implementation that references upstream to ensure the close promise is not
dropped.
- Around line 24-26: The import is using node:crypto.randomUUID instead of the
project's crypto abstraction; replace the direct import of randomUUID with the
exported function from `@frontmcp/utils` and update any usages to call that
function (locate the import line importing randomUUID and references in this
file—e.g., the import statement and any call sites that use randomUUID in the
bridge entrypoint). Ensure you import the named export (or default as provided
by `@frontmcp/utils`) and remove the node:crypto import so all UUID generation
uses the repo's crypto layer.
In `@libs/cli/src/commands/dev/bridge/log.ts`:
- Around line 52-75: Update imports to use pathResolve and dirname from
`@frontmcp/utils` and remove the node:path import, then replace any usage of
path.resolve/dirname with pathResolve(...) and dirname(...). In the stream
creation block (references: stream, filePath, resolvedPath) compute resolvedPath
via dirname(pathResolve(filePath)); after creating stream
(fs.createWriteStream(...)) attach an 'error' listener on stream to handle
asynchronous write errors and set stream = undefined and resolvedPath =
undefined inside that handler so subsequent calls to write(...) fall back to
stderr; ensure the existing try/catch remains for synchronous failures and keep
write(level, message, data) using formatLine and options.alsoStderr as before.
- Around line 10-11: The file currently imports node:fs and node:path directly
and creates a WriteStream without an 'error' handler; update the imports to use
the `@frontmcp/utils` equivalents for filesystem and path operations, and when
creating the stream (the variable named stream in the bridge log logic) wrap the
createWriteStream/ensureDir sequence in a try/catch and register
stream.on('error', ...) to set stream = undefined (fall back to stderr-only) and
clear resolvedPath, similarly handle any errors from path.resolve/dirname by
falling back to undefined; ensure the code paths that call stream.write() still
check stream existence before writing.
In `@libs/cli/src/commands/dev/bridge/state-machine.ts`:
- Around line 83-94: flushBufferAsResponses currently hardcodes
DEV_SERVER_UNREACHABLE so onReloadDeadline emits the wrong public error; change
flushBufferAsResponses to accept an error-type parameter (e.g., errorCode:
string) or optional override and use that instead of DEV_SERVER_UNREACHABLE in
the respond(makeDevError(...)) call, then call flushBufferAsResponses with
DEV_RELOAD_DEADLINE from onReloadDeadline; apply the same change to the other
call site referenced (around the 160-164 block) so both places pass the correct
error constant.
- Around line 121-131: During the drain loop when forward(f) throws, the code
only logs and leaves frames unresolved/inflight; change the catch block in the
async drain IIFE to mirror the live-enqueue failure path: synthesize an error
response for the request frame `f` (using the same shape/message used for live
failures), clear the `inflight` entry for `f.id`, and deliver that synthesized
error upstream via the same mechanism used for live enqueue failures (e.g. the
function that handles enqueue rejections or `relayUpstream`/request-resolve
routine) so pending promises/requests are resolved and inflight state is
removed; reference `drain`, `forward`, `inflight`, and the on-child-ready/drain
handling code when implementing.
In `@libs/cli/src/commands/dev/bridge/stdio-framer.ts`:
- Around line 70-73: When `JSON.parse` yields a non-object the code currently
only logs via log.warn and continues; change this to emit a JSON-RPC parse error
response (code -32700) the same way you do for actual parse failures. In the
block that checks `if (!parsed || typeof parsed !== 'object')` replace or
augment the log.warn with sending a JSON-RPC error frame (e.g. { jsonrpc: "2.0",
id: null, error: { code: -32700, message: "Parse error: expected JSON object" }
}) using the same outgoing framing/sending function used elsewhere, then
continue; keep the raw logging if desired but ensure the error response is
emitted for `parsed` non-objects.
- Line 53: The `paused` flag in stdio-framer.ts is toggled on write backpressure
but never read; either remove it or use it to control the input stream. Fix by
implementing the backpressure flow in the same module: when the write call
returns false and you set paused = true, call input.pause() (or equivalent
readable.pause()) to stop parsing/memory growth, and when the 'drain' handler
sets paused = false, call input.resume() to continue parsing; alternatively, if
you intend to keep parsing regardless, remove the paused variable and its
toggles (remove assignments around the write/backpressure and drain handlers) to
avoid dead state. Ensure you reference the existing paused variable and the
input readable used in the parser code.
In `@libs/cli/src/commands/dev/bridge/upstream-client.ts`:
- Around line 45-59: The abortController variable is declared but never
instantiated or wired into fetch, so close()'s abort is a no-op; inside
send(frame) create a new AbortController (assign it to the outer
abortController), pass its signal into fetch (e.g., signal:
abortController.signal), and ensure you clear abortController (set to undefined)
in a finally block after the request/stream completes or errors so subsequent
calls create a fresh controller; reference the abortController variable, the
send(frame) function, the fetch call, and the close() path that calls
abortController?.abort().
---
Nitpick comments:
In `@libs/cli/src/commands/dev/bridge/__tests__/state-machine.spec.ts`:
- Around line 106-122: Add a direct unit test that calls the public
onReloadDeadline() method instead of advancing timers: create the FSM via
makeFsm({ reloadDeadlineMs: 1000 }), drive it to the Reloading state (call
fsm.onBootStart(), fsm.onChildReady(), fsm.onWatcherEvent('src/foo.ts')),
enqueue a buffered request (await fsm.enqueue({ jsonrpc:'2.0', id:5,
method:'wait' })), then call fsm.onReloadDeadline() and assert fsm.state ===
'Degraded' and that rec.responses contains an entry with id === 5 and error.code
=== -32097 to lock in the contract.
In `@libs/cli/src/commands/dev/bridge/index.ts`:
- Around line 160-165: The callback passed to onChange uses a non-null assertion
on supervisor (supervisor!.restart()); remove the `!` and add a runtime check
before calling restart: inside onChange (and the surrounding async IIFE) verify
supervisor is defined (e.g., if (!supervisor) { /* log or throw a specific Error
class */ return; }) and then call await supervisor.restart(); also keep the
existing try/catch around restart and surface a clear error (or throw a specific
error class) instead of relying on the non-null assertion; this change touches
the onChange handler and the supervisor variable usage so locate supervisor,
onChange, fsm.onWatcherEvent, and the restart call to implement the check.
In `@libs/cli/src/commands/dev/bridge/stdio-framer.ts`:
- Line 67: The code currently casts error payloads using "as JsonRpcFrame" when
calling write(makeDevError(...)) (see makeDevError and write invocations), which
bypasses runtime structural validation; update the framer to either (A) add a
lightweight runtime guard that verifies required JsonRpcFrame fields before
calling write (and fall back to a safe default or log+drop when invalid), or (B)
if you intentionally trust upstream, add an explicit code comment and JSDoc on
makeDevError/write that documents the trust boundary so the assertion is
intentional; implement one of these options and reference the makeDevError and
write call sites to locate 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: 6d679ef1-2fed-4b2b-ac32-587853bd7fd9
📒 Files selected for processing (18)
docs/frontmcp/deployment/mcp-clients.mdxlibs/cli/src/commands/dev/bridge/__tests__/errors.spec.tslibs/cli/src/commands/dev/bridge/__tests__/state-machine.spec.tslibs/cli/src/commands/dev/bridge/__tests__/stdio-framer.spec.tslibs/cli/src/commands/dev/bridge/child-supervisor.tslibs/cli/src/commands/dev/bridge/errors.tslibs/cli/src/commands/dev/bridge/index.tslibs/cli/src/commands/dev/bridge/log.tslibs/cli/src/commands/dev/bridge/state-machine.tslibs/cli/src/commands/dev/bridge/stdio-framer.tslibs/cli/src/commands/dev/bridge/upstream-client.tslibs/cli/src/commands/dev/bridge/watcher.tslibs/cli/src/commands/dev/dev.tslibs/cli/src/commands/dev/register.tslibs/cli/src/core/args.tslibs/cli/src/core/bridge.tslibs/sdk/src/front-mcp/front-mcp.tslibs/skills/catalog/frontmcp-deployment/references/mcp-client-integration.md
Contributor
There was a problem hiding this comment.
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/dev.ts (1)
84-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad
.envbefore the--stdioearly returnLine 84 returns before Line 93 runs
loadDevEnv(cwd), so stdio mode skips.env/.env.localloading. That changes runtime behavior versus normaldevmode and can break config-dependent startup.💡 Proposed fix
export async function runDev(opts: ParsedArgs): Promise<void> { + const cwd = process.cwd(); + // Keep env loading consistent across both legacy and --stdio dev paths. + loadDevEnv(cwd); + // Issue `#399` — `--stdio` runs the first-party watch-aware stdio bridge // instead of the legacy `tsx --watch + tsc --noEmit --watch` pair. The // bridge owns process stdin/stdout (JSON-RPC frames only), holds the // upstream MCP session across child restarts, and replaces the // third-party `mcp-remote` recipe for the dev loop. if (opts.stdio) { const { runDevBridge } = await import('./bridge/index.js'); return runDevBridge(opts); } - const cwd = process.cwd(); const entry = await resolveEntry(cwd, opts.entry); - // Load .env and .env.local files before starting the server - loadDevEnv(cwd); + // .env already loaded above for both execution paths🤖 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/dev.ts` around lines 84 - 87, The stdio early return in dev.ts causes loadDevEnv(cwd) to be skipped when opts.stdio is true; move the call to loadDevEnv(cwd) so it runs before the opts.stdio branch. Specifically, ensure loadDevEnv is invoked (using the same cwd used elsewhere in dev.ts) prior to checking opts.stdio and before importing/returning runDevBridge, so both stdio and normal dev paths load .env/.env.local consistently.
🤖 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.
Outside diff comments:
In `@libs/cli/src/commands/dev/dev.ts`:
- Around line 84-87: The stdio early return in dev.ts causes loadDevEnv(cwd) to
be skipped when opts.stdio is true; move the call to loadDevEnv(cwd) so it runs
before the opts.stdio branch. Specifically, ensure loadDevEnv is invoked (using
the same cwd used elsewhere in dev.ts) prior to checking opts.stdio and before
importing/returning runDevBridge, so both stdio and normal dev paths load
.env/.env.local consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d56f42d3-69fb-46a0-86c9-061f3fb94082
📒 Files selected for processing (3)
libs/cli/src/commands/dev/dev.tslibs/sdk/src/front-mcp/front-mcp.tslibs/skills/catalog/frontmcp-deployment/references/mcp-client-integration.md
✅ Files skipped from review due to trivial changes (1)
- libs/skills/catalog/frontmcp-deployment/references/mcp-client-integration.md
# Conflicts: # libs/cli/src/commands/dev/register.ts # libs/cli/src/core/args.ts # libs/cli/src/core/bridge.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-24T20:45:45.138Z |
Contributor
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
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 `@docs/frontmcp/deployment/mcp-clients.mdx`:
- Around line 250-252: Update the npx invocation used for the FrontMCP client
bridge so it runs non-interactively: add the "-y" flag to the npx args array
used in the command object (where "command": "npx" and "args": ["frontmcp",
"dev", "--stdio"]) and likewise for the other similar snippet around the second
occurrence; this ensures npx won't prompt for confirmation during unattended
runs.
In `@libs/cli/src/commands/dev/bridge/__tests__/state-machine.spec.ts`:
- Around line 212-239: The test never triggers the drain-failure path because
the cast to call __setForwardError is a no-op; fix by changing the test or
makeFsm builder so the FSM exposes a real hook to inject a forward() failure
(e.g. have makeFsm return a setter like __setForwardError or attach
setForwardError on the fsm/supervisor object), then use that setter before
calling fsm.onChildReady() to throw an Error during the drain; finally assert
that rec.responses contains the synthesized dev_server_unreachable error for
id=99 and that fsm.bufferDepth() goes to 0.
In `@libs/cli/src/commands/dev/bridge/child-supervisor.ts`:
- Around line 63-70: The stderr bootstrap sentinel is being enabled for all
modes causing probeReady to treat the child as ready in pipe mode before IPC
arrives; update buildEnv (the function that sets FRONTMCP_DEV_BOOTSTRAP_SENTINEL
along with FRONTMCP_DEV_FORCE_SESSION_ID, FRONTMCP_DEV_PORT, and
FRONTMCP_DEV_STDIO_FD) to only set FRONTMCP_DEV_BOOTSTRAP_SENTINEL when mode !==
'pipe' (leave FRONTMCP_DEV_STDIO_FD for pipe), and/or adjust probeReady so it
ignores the bootstrap sentinel when mode === 'pipe' so readiness in pipe mode is
only satisfied by the IPC readiness message.
- Around line 200-218: Wrap the post-spawn startup sequence in start() and
restart() (the code that calls probeReady(child) and onReady(child) after
spawnChild() and wireExitHandler(child)) so that any rejection/throw will clean
up the spawned process: in a try/catch around the awaits, on error ensure the
child is terminated (call killCurrent() or otherwise kill and await the spawned
child), remove/unwire any exit handler if necessary, set current = undefined,
and then rethrow the original error; this guarantees spawnChild(), current,
probeReady(), and onReady() won't leave a stranded subprocess after failure.
In `@libs/cli/src/commands/dev/bridge/index.ts`:
- Around line 133-138: The onReady handler currently awaits upstream?.close()
which, if it rejects, prevents re-binding and calling fsm.onChildReady(); wrap
the upstream?.close() call in a try/catch so any error is caught and logged (or
ignored) and execution continues to call buildUpstreamForChild(child) and
fsm.onChildReady(); update the onReady function around the existing upstream
close/assign logic to safely handle failures without aborting the reload path
(refer to upstream, buildUpstreamForChild, and fsm.onChildReady).
In `@libs/cli/src/commands/dev/bridge/state-machine.ts`:
- Around line 115-142: The drain started in onChildReady() must be cancellable
when the FSM leaves 'Ready': add a generation/token (e.g. readyGen) or
AbortController that onChildReady captures and that
onWatcherEvent()/onChildExit()/stop() increments/aborts; before forwarding each
buffered frame (in the async drain loop) check the token/generation or aborted
flag and bail out if the FSM is no longer the same Ready instance so you don't
forward into a replaced child; ensure inflight bookkeeping and
respond(makeDevError(...)) only run if the drain still owns the captured
token/generation to avoid duplicate or stranded responses.
- Around line 197-214: The enqueue logic currently treats 'Stopping' like
Idle/Booting and buffers incoming frames, which can never be drained after
stop() starts; update the state check in the enqueue function to explicitly
handle state === 'Stopping' the same way 'Degraded' does: if isRequest call
respond(makeDevError(frame.id ?? null, <appropriate stopping error constant>, {
reason: 'stopping' })) and return (do not push into buffer), otherwise log a
drop-notification for non-requests; use the existing symbols (enqueue, stop,
state, frame, buffer, bufferSize, respond, makeDevError, DEV_BUFFER_FULL) and
pick/introduce a stopping-specific error code constant (e.g.,
DEV_SERVER_STOPPING) if one exists or add it before reusing
DEV_SERVER_UNREACHABLE.
In `@libs/cli/src/commands/dev/bridge/stdio-framer.ts`:
- Around line 95-108: The stop() path currently removes the 'drain' listener and
can leave promises in drainResolvers unresolved causing awaiting callers of
write() to hang; modify stop() to settle queued backpressure writes by iterating
over the drainResolvers array and calling each resolver (resolving pending
write() promises) before clearing the array and removing listeners, ensure
paused is reset and input.resume()/input.pause handling is symmetric, and avoid
double-resolving by clearing drainResolvers immediately after invoking
resolvers; reference functions/variables: write, stop, drainResolvers,
output.write, input.pause, paused.
- Around line 86-87: The call to onFrame(parsed as JsonRpcFrame) currently
discards its returned Promise (void onFrame(...)), so any rejection becomes an
unhandled rejection; change the invocation in stdio-framer to handle rejections
explicitly by either awaiting the Promise (if inside an async wrapper) or
attaching a .catch handler that logs or forwards the error; specifically locate
the onFrame(...) call and replace the void call with onFrame(parsed as
JsonRpcFrame).catch(err => /* handle/log error, e.g. processLogger.error or emit
error event */) or await onFrame(parsed as JsonRpcFrame) inside a try/catch
block to ensure rejected Promises are handled.
In `@libs/cli/src/commands/dev/bridge/upstream-client.ts`:
- Around line 45-54: abortController is a single shared AbortController that
gets overwritten by concurrent send() calls, causing close() to only abort the
last-assigned request; change the implementation to create a per-request
controller and track all active controllers (e.g., a Set or Map) instead of a
single abortController, have each send(frame) add its controller to the
collection and remove it in finally, and have close() iterate the collection to
abort and clear all controllers; apply the same pattern to the similar
controller usage referenced around lines 113-123 so no request's controller can
be clobbered.
- Around line 69-72: The send() flow currently swallows transport failures by
logging and returning when res.ok is false, when fetch throws, when pipe.send
fails, or when child.send fails; change these branches to reject the send
promise (throw an Error or return a rejected Promise) with a clear message
including context (e.g., HTTP status, frame.method, underlying error) so callers
can handle inflight bookkeeping and produce JSON-RPC errors. Update the non-OK
branch that checks res.ok (using res and frame), the fetch catch path, the
pipe.send failure path (pipe.send), and the child.send callback path
(child.send) to propagate errors instead of just log-and-return; include the
original error or status in the thrown/rejected Error to aid debugging. Ensure
all call sites of send() still await/reject properly so upstream logic can
synthesize JSON-RPC error responses.
In
`@libs/skills/catalog/frontmcp-deployment/references/mcp-client-integration.md`:
- Around line 86-88: Update the bridge wiring snippet so the spawned command
uses npx with the -y flag to avoid interactive first-run prompts: change the
JSON "command" and "args" invocation where "command": "npx" and "args":
["frontmcp", "dev", "--stdio"] to include the -y option in the args (i.e.,
ensure the args for the npx invocation include "-y" before "frontmcp") so client
startup cannot be blocked by prompts.
🪄 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: 7917398d-be4a-4977-9611-2410da6896df
📒 Files selected for processing (18)
docs/frontmcp/deployment/mcp-clients.mdxlibs/cli/src/commands/dev/bridge/__tests__/errors.spec.tslibs/cli/src/commands/dev/bridge/__tests__/state-machine.spec.tslibs/cli/src/commands/dev/bridge/__tests__/stdio-framer.spec.tslibs/cli/src/commands/dev/bridge/child-supervisor.tslibs/cli/src/commands/dev/bridge/errors.tslibs/cli/src/commands/dev/bridge/index.tslibs/cli/src/commands/dev/bridge/log.tslibs/cli/src/commands/dev/bridge/state-machine.tslibs/cli/src/commands/dev/bridge/stdio-framer.tslibs/cli/src/commands/dev/bridge/upstream-client.tslibs/cli/src/commands/dev/bridge/watcher.tslibs/cli/src/commands/dev/dev.tslibs/cli/src/commands/dev/register.tslibs/cli/src/core/args.tslibs/cli/src/core/bridge.tslibs/sdk/src/front-mcp/front-mcp.tslibs/skills/catalog/frontmcp-deployment/references/mcp-client-integration.md
…ror handling in state machine
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
CLI
Documentation
Tests