Implement STDIO transport support and enhance connection management #2098
Conversation
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
Contributor
Release OptionsShould a new version be published when this PR is merged? React with an emoji to vote on the release type:
Current version: Deployment
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/mesh/src/web/routes/orgs/connections.tsx">
<violation number="1" location="apps/mesh/src/web/routes/orgs/connections.tsx:86">
P1: Schema validation doesn't enforce conditional required fields. `npx_package` should be required when `ui_type === "NPX"`, and `connection_url` should be required when `ui_type !== "NPX"`. Consider using Zod's `.refine()` or `.superRefine()` to add conditional validation.</violation>
<violation number="2" location="apps/mesh/src/web/routes/orgs/connections.tsx:226">
P1: Unsafe type assertion: If a STDIO connection's URL fails to parse as NPX format, `connection_type` ("STDIO") is incorrectly cast to `"HTTP" | "SSE" | "Websocket"`. Consider handling this edge case explicitly, perhaps defaulting to a sensible `ui_type` or showing an error.</violation>
</file>
<file name="apps/mesh/src/api/routes/proxy.ts">
<violation number="1" location="apps/mesh/src/api/routes/proxy.ts:299">
P1: STDIO clients from `stdioManager.spawn()` are pooled and reused, but `executeToolCall` closes the client in its `finally` block (line 419). This will close the managed STDIO connection after the first tool call, leaving the manager in an inconsistent state where subsequent requests return a broken client. Consider conditionally skipping the close for STDIO connections, or having the manager return a wrapper that ignores close calls.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| form.reset({ | ||
| title: editingConnection.title, | ||
| description: editingConnection.description, | ||
| ui_type: editingConnection.connection_type as "HTTP" | "SSE" | "Websocket", |
There was a problem hiding this comment.
P1: Unsafe type assertion: If a STDIO connection's URL fails to parse as NPX format, connection_type ("STDIO") is incorrectly cast to "HTTP" | "SSE" | "Websocket". Consider handling this edge case explicitly, perhaps defaulting to a sensible ui_type or showing an error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/web/routes/orgs/connections.tsx, line 226:
<comment>Unsafe type assertion: If a STDIO connection's URL fails to parse as NPX format, `connection_type` ("STDIO") is incorrectly cast to `"HTTP" | "SSE" | "Websocket"`. Consider handling this edge case explicitly, perhaps defaulting to a sensible `ui_type` or showing an error.</comment>
<file context>
@@ -148,33 +180,67 @@ function OrgMcpsContent() {
+ form.reset({
+ title: editingConnection.title,
+ description: editingConnection.description,
+ ui_type: editingConnection.connection_type as "HTTP" | "SSE" | "Websocket",
+ connection_url: editingConnection.connection_url,
+ connection_token: null, // Don't pre-fill token for security
</file context>
Suggested change
| ui_type: editingConnection.connection_type as "HTTP" | "SSE" | "Websocket", | |
| ui_type: ["HTTP", "SSE", "Websocket"].includes(editingConnection.connection_type) | |
| ? (editingConnection.connection_type as "HTTP" | "SSE" | "Websocket") | |
| : "HTTP", |
| connection_url: z.string().optional(), | ||
| connection_token: z.string().nullable().optional(), | ||
| // For NPX | ||
| npx_package: z.string().optional(), |
There was a problem hiding this comment.
P1: Schema validation doesn't enforce conditional required fields. npx_package should be required when ui_type === "NPX", and connection_url should be required when ui_type !== "NPX". Consider using Zod's .refine() or .superRefine() to add conditional validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/web/routes/orgs/connections.tsx, line 86:
<comment>Schema validation doesn't enforce conditional required fields. `npx_package` should be required when `ui_type === "NPX"`, and `connection_url` should be required when `ui_type !== "NPX"`. Consider using Zod's `.refine()` or `.superRefine()` to add conditional validation.</comment>
<file context>
@@ -75,20 +74,53 @@ import { generatePrefixedId } from "@/shared/utils/generate-id";
+ connection_url: z.string().optional(),
+ connection_token: z.string().nullable().optional(),
+ // For NPX
+ npx_package: z.string().optional(),
+ npx_env_var_name: z.string().optional(),
+ npx_api_key: z.string().optional(),
</file context>
| } | ||
|
|
||
| // Spawn or get existing client from the manager | ||
| return stdioManager.spawn({ |
There was a problem hiding this comment.
P1: STDIO clients from stdioManager.spawn() are pooled and reused, but executeToolCall closes the client in its finally block (line 419). This will close the managed STDIO connection after the first tool call, leaving the manager in an inconsistent state where subsequent requests return a broken client. Consider conditionally skipping the close for STDIO connections, or having the manager return a wrapper that ignores close calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/proxy.ts, line 299:
<comment>STDIO clients from `stdioManager.spawn()` are pooled and reused, but `executeToolCall` closes the client in its `finally` block (line 419). This will close the managed STDIO connection after the first tool call, leaving the manager in an inconsistent state where subsequent requests return a broken client. Consider conditionally skipping the close for STDIO connections, or having the manager return a wrapper that ignores close calls.</comment>
<file context>
@@ -271,8 +275,35 @@ async function createMCPProxyDoNotUseDirectly(
+ }
+
+ // Spawn or get existing client from the manager
+ return stdioManager.spawn({
+ ...stdioConfig,
+ id: connectionId,
</file context>
✅ Addressed in d7acad1
There was a problem hiding this comment.
10 issues found across 52 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/studio/src/components/widgets/dynamic-options.tsx">
<violation number="1" location="apps/studio/src/components/widgets/dynamic-options.tsx:120">
P1: Missing `fetchOptions` in the useEffect dependency array violates the React hooks exhaustive-deps rule. Since `fetchOptions` is a `useCallback` with its own dependencies, it should be included in this effect's deps. Without it, the effect may use a stale version of the callback.</violation>
</file>
<file name="apps/mesh/src/web/components/details/connection/settings-tab/stdio-status-panel.tsx">
<violation number="1" location="apps/mesh/src/web/components/details/connection/settings-tab/stdio-status-panel.tsx:86">
P1: The response from the `/restart` endpoint is not checked for errors. If the restart fails, the error is silently swallowed and the mutation proceeds to call `/start`, potentially masking the real failure from users.</violation>
</file>
<file name="apps/studio/src/components/json-preview.tsx">
<violation number="1" location="apps/studio/src/components/json-preview.tsx:59">
P2: The Copy button always copies `jsonString` regardless of which tab is active. When viewing the Schema tab, users would expect to copy the schema content. Consider tracking the active tab and copying the appropriate content:
```tsx
const [activeTab, setActiveTab] = useState('json');
// ...
<Tabs defaultValue="json" onValueChange={setActiveTab}>
// ...
onClick={() => handleCopy(activeTab === 'json' ? jsonString : schemaString)}
```</violation>
</file>
<file name="apps/studio/src/components/widgets/color-widget.tsx">
<violation number="1" location="apps/studio/src/components/widgets/color-widget.tsx:22">
P2: Inconsistent default values between color picker and text input. When `value` is undefined, the color picker shows `#000000` but the text input shows an empty string, creating a confusing user experience where the controls display different values.</violation>
</file>
<file name="apps/studio/src/server/index.ts">
<violation number="1" location="apps/studio/src/server/index.ts:50">
P2: Missing input validation for `name` and `schema`. Unlike the `/api/extract-schema` endpoint which validates required fields, this endpoint will create entries with potentially undefined values.</violation>
</file>
<file name="apps/studio/src/components/widgets/text-widget.tsx">
<violation number="1" location="apps/studio/src/components/widgets/text-widget.tsx:16">
P2: The `onBlur` handler uses `value` from props (captured at render time) instead of the current DOM value. This can cause stale values to be reported during race conditions. Use `e.target.value` to get the actual input value.</violation>
</file>
<file name="apps/mesh/src/tools/stdio/index.ts">
<violation number="1" location="apps/mesh/src/tools/stdio/index.ts:109">
P2: Handler always returns `success: true` even when the connection doesn't exist. Consider checking `stdioManager.isRunning()` first, similar to `stopStdioConnection`, to return an appropriate error message when the connection is not found.</violation>
</file>
<file name="apps/studio/src/lib/schema-extractor.ts">
<violation number="1" location="apps/studio/src/lib/schema-extractor.ts:20">
P1: Regex injection vulnerability: `typeName` is directly interpolated into RegExp without escaping special characters. If `typeName` contains regex metacharacters (e.g., `.`, `*`, `[`), it could cause unexpected matches or ReDoS attacks. Escape the input before using it in the regex.</violation>
</file>
<file name="apps/mesh/src/stdio/stdio-manager.ts">
<violation number="1" location="apps/mesh/src/stdio/stdio-manager.ts:115">
P1: The try-catch block is ineffective - `return existing.client` cannot throw. This means the connection health check doesn't work as intended. Consider actually performing a lightweight operation like `ping()` to verify the connection is alive, or remove the try-catch if the `onclose` handler is sufficient for detecting dead connections.</violation>
</file>
<file name="apps/mesh/src/api/app.ts">
<violation number="1" location="apps/mesh/src/api/app.ts:409">
P1: Missing authentication middleware for STDIO management routes. These endpoints expose sensitive operations (listing connections, viewing logs, stopping/starting processes) without requiring authentication. Apply `mcpAuth` middleware similar to `/mcp/*` routes.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| if (isLoader || (isMustache && !isLoader)) { | ||
| fetchOptions(value as string | undefined); | ||
| } | ||
| }, [optionsUrl, JSON.stringify(parentData)]); |
There was a problem hiding this comment.
P1: Missing fetchOptions in the useEffect dependency array violates the React hooks exhaustive-deps rule. Since fetchOptions is a useCallback with its own dependencies, it should be included in this effect's deps. Without it, the effect may use a stale version of the callback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/studio/src/components/widgets/dynamic-options.tsx, line 120:
<comment>Missing `fetchOptions` in the useEffect dependency array violates the React hooks exhaustive-deps rule. Since `fetchOptions` is a `useCallback` with its own dependencies, it should be included in this effect's deps. Without it, the effect may use a stale version of the callback.</comment>
<file context>
@@ -0,0 +1,170 @@
+ if (isLoader || (isMustache && !isLoader)) {
+ fetchOptions(value as string | undefined);
+ }
+ }, [optionsUrl, JSON.stringify(parentData)]);
+
+ // Handle autocomplete search
</file context>
| const restartMutation = useMutation({ | ||
| mutationFn: async () => { | ||
| // First stop, then start | ||
| await fetch(`/api/stdio/${connectionId}/restart`, { method: "POST" }); |
There was a problem hiding this comment.
P1: The response from the /restart endpoint is not checked for errors. If the restart fails, the error is silently swallowed and the mutation proceeds to call /start, potentially masking the real failure from users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/web/components/details/connection/settings-tab/stdio-status-panel.tsx, line 86:
<comment>The response from the `/restart` endpoint is not checked for errors. If the restart fails, the error is silently swallowed and the mutation proceeds to call `/start`, potentially masking the real failure from users.</comment>
<file context>
@@ -0,0 +1,306 @@
+ const restartMutation = useMutation({
+ mutationFn: async () => {
+ // First stop, then start
+ await fetch(`/api/stdio/${connectionId}/restart`, { method: "POST" });
+ // Then start it again
+ const response = await fetch(`/api/stdio/${connectionId}/start`, {
</file context>
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => handleCopy(jsonString)} |
There was a problem hiding this comment.
P2: The Copy button always copies jsonString regardless of which tab is active. When viewing the Schema tab, users would expect to copy the schema content. Consider tracking the active tab and copying the appropriate content:
const [activeTab, setActiveTab] = useState('json');
// ...
<Tabs defaultValue="json" onValueChange={setActiveTab}>
// ...
onClick={() => handleCopy(activeTab === 'json' ? jsonString : schemaString)}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/studio/src/components/json-preview.tsx, line 59:
<comment>The Copy button always copies `jsonString` regardless of which tab is active. When viewing the Schema tab, users would expect to copy the schema content. Consider tracking the active tab and copying the appropriate content:
```tsx
const [activeTab, setActiveTab] = useState('json');
// ...
<Tabs defaultValue="json" onValueChange={setActiveTab}>
// ...
onClick={() => handleCopy(activeTab === 'json' ? jsonString : schemaString)}
```</comment>
<file context>
@@ -0,0 +1,130 @@
+ <Button
+ variant="outline"
+ size="sm"
+ onClick={() => handleCopy(jsonString)}
+ className="gap-2"
+ >
</file context>
| <Input | ||
| id={id} | ||
| type="text" | ||
| value={value ?? ""} |
There was a problem hiding this comment.
P2: Inconsistent default values between color picker and text input. When value is undefined, the color picker shows #000000 but the text input shows an empty string, creating a confusing user experience where the controls display different values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/studio/src/components/widgets/color-widget.tsx, line 22:
<comment>Inconsistent default values between color picker and text input. When `value` is undefined, the color picker shows `#000000` but the text input shows an empty string, creating a confusing user experience where the controls display different values.</comment>
<file context>
@@ -0,0 +1,32 @@
+ <Input
+ id={id}
+ type="text"
+ value={value ?? ""}
+ required={required}
+ disabled={disabled || readonly}
</file context>
apps/studio/src/server/index.ts
Outdated
|
|
||
| app.post("/api/schemas", async (c) => { | ||
| try { | ||
| const { name, schema } = await c.req.json(); |
There was a problem hiding this comment.
P2: Missing input validation for name and schema. Unlike the /api/extract-schema endpoint which validates required fields, this endpoint will create entries with potentially undefined values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/studio/src/server/index.ts, line 50:
<comment>Missing input validation for `name` and `schema`. Unlike the `/api/extract-schema` endpoint which validates required fields, this endpoint will create entries with potentially undefined values.</comment>
<file context>
@@ -0,0 +1,76 @@
+
+app.post("/api/schemas", async (c) => {
+ try {
+ const { name, schema } = await c.req.json();
+ const id = crypto.randomUUID();
+ const entry = { id, name, schema, createdAt: new Date().toISOString() };
</file context>
| disabled={disabled || readonly} | ||
| placeholder={placeholder} | ||
| onChange={(e) => onChange(e.target.value === "" ? undefined : e.target.value)} | ||
| onBlur={() => onBlur(id, value)} |
There was a problem hiding this comment.
P2: The onBlur handler uses value from props (captured at render time) instead of the current DOM value. This can cause stale values to be reported during race conditions. Use e.target.value to get the actual input value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/studio/src/components/widgets/text-widget.tsx, line 16:
<comment>The `onBlur` handler uses `value` from props (captured at render time) instead of the current DOM value. This can cause stale values to be reported during race conditions. Use `e.target.value` to get the actual input value.</comment>
<file context>
@@ -0,0 +1,22 @@
+ disabled={disabled || readonly}
+ placeholder={placeholder}
+ onChange={(e) => onChange(e.target.value === "" ? undefined : e.target.value)}
+ onBlur={() => onBlur(id, value)}
+ onFocus={() => onFocus(id, value)}
+ className="w-full"
</file context>
Comment on lines
+109
to
+116
| handler: async (input: { connectionId: string }) => { | ||
| // Stop the connection - this marks it as stopped | ||
| await stdioManager.stop(input.connectionId); | ||
| // The next spawn() call will automatically restart it | ||
| return { | ||
| success: true, | ||
| message: `Connection ${input.connectionId} stopped. Will restart on next request.`, | ||
| }; |
There was a problem hiding this comment.
P2: Handler always returns success: true even when the connection doesn't exist. Consider checking stdioManager.isRunning() first, similar to stopStdioConnection, to return an appropriate error message when the connection is not found.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/tools/stdio/index.ts, line 109:
<comment>Handler always returns `success: true` even when the connection doesn't exist. Consider checking `stdioManager.isRunning()` first, similar to `stopStdioConnection`, to return an appropriate error message when the connection is not found.</comment>
<file context>
@@ -92,9 +92,76 @@ export const stopAllStdioConnections = defineTool({
+ success: z.boolean(),
+ message: z.string(),
+ }),
+ handler: async (input: { connectionId: string }) => {
+ // Stop the connection - this marks it as stopped
+ await stdioManager.stop(input.connectionId);
</file context>
Suggested change
| handler: async (input: { connectionId: string }) => { | |
| // Stop the connection - this marks it as stopped | |
| await stdioManager.stop(input.connectionId); | |
| // The next spawn() call will automatically restart it | |
| return { | |
| success: true, | |
| message: `Connection ${input.connectionId} stopped. Will restart on next request.`, | |
| }; | |
| handler: async (input: { connectionId: string }) => { | |
| const isRunning = stdioManager.isRunning(input.connectionId); | |
| if (!isRunning) { | |
| return { | |
| success: false, | |
| message: `Connection ${input.connectionId} is not running`, | |
| }; | |
| } | |
| // Stop the connection - this marks it as stopped | |
| await stdioManager.stop(input.connectionId); | |
| // The next spawn() call will automatically restart it | |
| return { | |
| success: true, | |
| message: `Connection ${input.connectionId} stopped. Will restart on next request.`, | |
| }; | |
| }, |
✅ Addressed in 6235cb0
|
|
||
| function parseTypeScriptToSchema(code: string, typeName: string): JSONSchema7 { | ||
| // Find the interface or type definition | ||
| const interfaceMatch = new RegExp( |
There was a problem hiding this comment.
P1: Regex injection vulnerability: typeName is directly interpolated into RegExp without escaping special characters. If typeName contains regex metacharacters (e.g., ., *, [), it could cause unexpected matches or ReDoS attacks. Escape the input before using it in the regex.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/studio/src/lib/schema-extractor.ts, line 20:
<comment>Regex injection vulnerability: `typeName` is directly interpolated into RegExp without escaping special characters. If `typeName` contains regex metacharacters (e.g., `.`, `*`, `[`), it could cause unexpected matches or ReDoS attacks. Escape the input before using it in the regex.</comment>
<file context>
@@ -0,0 +1,199 @@
+
+function parseTypeScriptToSchema(code: string, typeName: string): JSONSchema7 {
+ // Find the interface or type definition
+ const interfaceMatch = new RegExp(
+ `(?:export\\s+)?interface\\s+${typeName}\\s*\\{([^}]+(?:\\{[^}]*\\}[^}]*)*)\\}`,
+ "s"
</file context>
| return; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
P1: The try-catch block is ineffective - return existing.client cannot throw. This means the connection health check doesn't work as intended. Consider actually performing a lightweight operation like ping() to verify the connection is alive, or remove the try-catch if the onclose handler is sufficient for detecting dead connections.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/stdio/stdio-manager.ts, line 115:
<comment>The try-catch block is ineffective - `return existing.client` cannot throw. This means the connection health check doesn't work as intended. Consider actually performing a lightweight operation like `ping()` to verify the connection is alive, or remove the try-catch if the `onclose` handler is sufficient for detecting dead connections.</comment>
<file context>
@@ -50,28 +66,152 @@ class StdioConnectionManager {
+ return;
+ }
+
+ try {
+ // Send a ping to keep the connection alive
+ // MCP SDK Client has a ping method
</file context>
✅ Addressed in 8f80c93
| app.route("/api", modelsRoutes); | ||
|
|
||
| // STDIO connection management routes (logs, info, restart) | ||
| app.route("/api/stdio", stdioLogsRoutes); |
There was a problem hiding this comment.
P1: Missing authentication middleware for STDIO management routes. These endpoints expose sensitive operations (listing connections, viewing logs, stopping/starting processes) without requiring authentication. Apply mcpAuth middleware similar to /mcp/* routes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/app.ts, line 409:
<comment>Missing authentication middleware for STDIO management routes. These endpoints expose sensitive operations (listing connections, viewing logs, stopping/starting processes) without requiring authentication. Apply `mcpAuth` middleware similar to `/mcp/*` routes.</comment>
<file context>
@@ -404,6 +405,9 @@ export function createApp(options: CreateAppOptions = {}) {
app.route("/api", modelsRoutes);
+ // STDIO connection management routes (logs, info, restart)
+ app.route("/api/stdio", stdioLogsRoutes);
+
// ============================================================================
</file context>
Suggested change
| app.route("/api/stdio", stdioLogsRoutes); | |
| app.use("/api/stdio/*", mcpAuth); | |
| app.route("/api/stdio", stdioLogsRoutes); |
vibegui
force-pushed
the
feat/stdio-npx-support
branch
from
afb64fa to
92fde40
Compare
- Added support for STDIO transport in the proxy, allowing for the management of stdio-based MCP connections. - Introduced a new StdioConnectionManager to handle the lifecycle of stdio connections, including spawning and monitoring processes. - Updated connection schema to include STDIO as a valid connection type and modified related tools for fetching and managing connections. - Enhanced the UI to support STDIO connections, including new form fields for NPX package management. - Improved error handling and logging for stdio connections to facilitate better debugging and monitoring.
- Added new routes for managing STDIO connections, including fetching logs, connection info, and starting/stopping processes. - Introduced a `StdioStatusPanel` component in the UI to display process status and logs for STDIO connections. - Enhanced the `StdioConnectionManager` to support logging of process events and connection status. - Updated tools to include functionalities for restarting STDIO connections and retrieving logs. - Improved error handling and user feedback for STDIO operations.
- Rewrite 'resource' parameter in authorize/token endpoints to origin MCP URL - Some auth servers (like Supabase) validate resource matches their endpoint - Make OAuth scope optional and configurable instead of hardcoded 'mcp' - Let servers decide default scope when not explicitly provided
vibegui
force-pushed
the
feat/stdio-npx-support
branch
from
92fde40 to
86340c7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 cubic
Adds STDIO transport with a managed process runner for MCP servers, plus NPX-driven setup and a status/logs panel. Also improves OAuth by rewriting the resource parameter and making scopes configurable.
New Features
Migration
Written for commit 86340c7. Summary will update automatically on new commits.