feat: flow #2050
Open
feat: flow #2050
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
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 88 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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/storage/gateway.ts">
<violation number="1" location="apps/mesh/src/storage/gateway.ts:93">
P2: Non-transactional gateway creation with connections. If gateway insert succeeds but connections insert fails, the gateway will exist without its intended connections. Consider wrapping both operations in a transaction for consistency.</violation>
<violation number="2" location="apps/mesh/src/storage/gateway.ts:346">
P1: Non-transactional connection update can cause data loss. If the delete succeeds but the insert fails, all gateway connections will be permanently lost. Wrap the gateway update and connection operations in a transaction like the `isDefault=true` path does.</violation>
</file>
<file name="apps/mesh/src/tools/connection/update.ts">
<violation number="1" location="apps/mesh/src/tools/connection/update.ts:69">
P1: The `value` variable obtained via `prop(key, state)` is only used for the null/undefined check, but the subsequent code still uses `state[key]` directly (line 76). This creates an inconsistency: JSON paths like `'foo.bar'` will pass validation via `prop()`, but the connection ID extraction via `state[key]` won't work for nested paths. Consider using the already-fetched `value` instead of re-accessing with `state[key]`.</violation>
</file>
<file name="apps/mesh/src/auth/org.ts">
<violation number="1" location="apps/mesh/src/auth/org.ts:139">
P1: Bug: Passing all connection IDs in exclusion mode will **exclude all connections**, not include them. The comment correctly states the intent (empty list = include all), but the code does the opposite. In exclusion mode, the `connections` array specifies what to exclude, so passing all IDs excludes everything.</violation>
</file>
<file name="apps/mesh/src/api/app.ts">
<violation number="1" location="apps/mesh/src/api/app.ts:265">
P1: The `resource_metadata` URL in the WWW-Authenticate header appends `.well-known/oauth-protected-resource` to the full request pathname, which produces incorrect URLs for nested paths. For a request to `/mcp/myConnection/sse`, this generates `/mcp/myConnection/sse/.well-known/oauth-protected-resource` instead of the correct base path. Consider extracting just the base MCP path (e.g., `/mcp/:connectionId` or `/mcp/gateway/:gatewayId`) to construct the resource metadata URL.</violation>
</file>
<file name="apps/mesh/src/tools/gateway/get.ts">
<violation number="1" location="apps/mesh/src/tools/gateway/get.ts:72">
P2: Unsafe type assertion `as string` on date fields. If the storage layer returns Date objects, this assertion won't convert them to strings - it will only suppress TypeScript errors while leaving Date objects at runtime. Consider explicitly converting to ISO string format (e.g., `new Date(gateway.createdAt).toISOString()`) or using a type guard to handle both cases.</violation>
</file>
<file name="packages/runtime/src/bindings.ts">
<violation number="1" location="packages/runtime/src/bindings.ts:174">
P1: Function doesn't actually modify state in-place. The result of `traverseAndReplace` is discarded. Either assign the result back to `ctx.state` or change the return type.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| .execute(); | ||
|
|
||
| // Insert gateway connections | ||
| if (data.connections.length > 0) { |
There was a problem hiding this comment.
P2: Non-transactional gateway creation with connections. If gateway insert succeeds but connections insert fails, the gateway will exist without its intended connections. Consider wrapping both operations in a transaction for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/storage/gateway.ts, line 93:
<comment>Non-transactional gateway creation with connections. If gateway insert succeeds but connections insert fails, the gateway will exist without its intended connections. Consider wrapping both operations in a transaction for consistency.</comment>
<file context>
@@ -0,0 +1,577 @@
+ .execute();
+
+ // Insert gateway connections
+ if (data.connections.length > 0) {
+ await trx
+ .insertInto("gateway_connections")
</file context>
| .execute(); | ||
|
|
||
| // Update connections if provided | ||
| if (data.connections !== undefined) { |
There was a problem hiding this comment.
P1: Non-transactional connection update can cause data loss. If the delete succeeds but the insert fails, all gateway connections will be permanently lost. Wrap the gateway update and connection operations in a transaction like the isDefault=true path does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/storage/gateway.ts, line 346:
<comment>Non-transactional connection update can cause data loss. If the delete succeeds but the insert fails, all gateway connections will be permanently lost. Wrap the gateway update and connection operations in a transaction like the `isDefault=true` path does.</comment>
<file context>
@@ -0,0 +1,577 @@
+ .execute();
+
+ // Update connections if provided
+ if (data.connections !== undefined) {
+ await trx
+ .deleteFrom("gateway_connections")
</file context>
| for (const scope of scopes) { | ||
| // Parse scope format: "KEY::SCOPE" | ||
| const [key] = parseScope(scope); | ||
| const value = prop(key, state); |
There was a problem hiding this comment.
P1: The value variable obtained via prop(key, state) is only used for the null/undefined check, but the subsequent code still uses state[key] directly (line 76). This creates an inconsistency: JSON paths like 'foo.bar' will pass validation via prop(), but the connection ID extraction via state[key] won't work for nested paths. Consider using the already-fetched value instead of re-accessing with state[key].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/tools/connection/update.ts, line 69:
<comment>The `value` variable obtained via `prop(key, state)` is only used for the null/undefined check, but the subsequent code still uses `state[key]` directly (line 76). This creates an inconsistency: JSON paths like `'foo.bar'` will pass validation via `prop()`, but the connection ID extraction via `state[key]` won't work for nested paths. Consider using the already-fetched `value` instead of re-accessing with `state[key]`.</comment>
<file context>
@@ -65,9 +66,10 @@ async function validateConfiguration(
for (const scope of scopes) {
// Parse scope format: "KEY::SCOPE"
const [key] = parseScope(scope);
+ const value = prop(key, state);
// Check if this key exists in state
</file context>
| toolSelectionMode: "exclusion", | ||
| status: "active", | ||
| isDefault: true, | ||
| connections: createdConnectionIds.map((c) => ({ connectionId: c })), |
There was a problem hiding this comment.
P1: Bug: Passing all connection IDs in exclusion mode will exclude all connections, not include them. The comment correctly states the intent (empty list = include all), but the code does the opposite. In exclusion mode, the connections array specifies what to exclude, so passing all IDs excludes everything.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/auth/org.ts, line 139:
<comment>Bug: Passing all connection IDs in exclusion mode will **exclude all connections**, not include them. The comment correctly states the intent (empty list = include all), but the code does the opposite. In exclusion mode, the `connections` array specifies what to exclude, so passing all IDs excludes everything.</comment>
<file context>
@@ -106,15 +109,35 @@ export async function createDefaultOrgConnections(
+ toolSelectionMode: "exclusion",
+ status: "active",
+ isDefault: true,
+ connections: createdConnectionIds.map((c) => ({ connectionId: c })),
+ });
} catch (err) {
</file context>
Suggested change
| connections: createdConnectionIds.map((c) => ({ connectionId: c })), | |
| connections: [], |
| status: 401, | ||
| headers: { | ||
| "WWW-Authenticate": `Bearer realm="mcp",resource_metadata="${origin}/mcp/${connectionId}/.well-known/oauth-protected-resource"`, | ||
| "WWW-Authenticate": `Bearer realm="mcp",resource_metadata="${url.origin}${url.pathname}/.well-known/oauth-protected-resource"`, |
There was a problem hiding this comment.
P1: The resource_metadata URL in the WWW-Authenticate header appends .well-known/oauth-protected-resource to the full request pathname, which produces incorrect URLs for nested paths. For a request to /mcp/myConnection/sse, this generates /mcp/myConnection/sse/.well-known/oauth-protected-resource instead of the correct base path. Consider extracting just the base MCP path (e.g., /mcp/:connectionId or /mcp/gateway/:gatewayId) to construct the resource metadata URL.
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 265:
<comment>The `resource_metadata` URL in the WWW-Authenticate header appends `.well-known/oauth-protected-resource` to the full request pathname, which produces incorrect URLs for nested paths. For a request to `/mcp/myConnection/sse`, this generates `/mcp/myConnection/sse/.well-known/oauth-protected-resource` instead of the correct base path. Consider extracting just the base MCP path (e.g., `/mcp/:connectionId` or `/mcp/gateway/:gatewayId`) to construct the resource metadata URL.</comment>
<file context>
@@ -269,22 +254,27 @@ export function createApp(options: CreateAppOptions = {}) {
status: 401,
headers: {
- "WWW-Authenticate": `Bearer realm="mcp",resource_metadata="${origin}/mcp/${connectionId}/.well-known/oauth-protected-resource"`,
+ "WWW-Authenticate": `Bearer realm="mcp",resource_metadata="${url.origin}${url.pathname}/.well-known/oauth-protected-resource"`,
},
}));
</file context>
| connection_id: conn.connectionId, | ||
| selected_tools: conn.selectedTools, | ||
| })), | ||
| created_at: gateway.createdAt as string, |
There was a problem hiding this comment.
P2: Unsafe type assertion as string on date fields. If the storage layer returns Date objects, this assertion won't convert them to strings - it will only suppress TypeScript errors while leaving Date objects at runtime. Consider explicitly converting to ISO string format (e.g., new Date(gateway.createdAt).toISOString()) or using a type guard to handle both cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/tools/gateway/get.ts, line 72:
<comment>Unsafe type assertion `as string` on date fields. If the storage layer returns Date objects, this assertion won't convert them to strings - it will only suppress TypeScript errors while leaving Date objects at runtime. Consider explicitly converting to ISO string format (e.g., `new Date(gateway.createdAt).toISOString()`) or using a type guard to handle both cases.</comment>
<file context>
@@ -0,0 +1,79 @@
+ connection_id: conn.connectionId,
+ selected_tools: conn.selectedTools,
+ })),
+ created_at: gateway.createdAt as string,
+ updated_at: gateway.updatedAt as string,
+ created_by: gateway.createdBy,
</file context>
packages/runtime/src/bindings.ts
Outdated
| ctx: RequestContext, | ||
| ): void => { | ||
| // resolves the state in-place | ||
| traverseAndReplace(ctx.state, ctx) as ResolvedBindings<T, TBindings>; |
There was a problem hiding this comment.
P1: Function doesn't actually modify state in-place. The result of traverseAndReplace is discarded. Either assign the result back to ctx.state or change the return type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/runtime/src/bindings.ts, line 174:
<comment>Function doesn't actually modify state in-place. The result of `traverseAndReplace` is discarded. Either assign the result back to `ctx.state` or change the return type.</comment>
<file context>
@@ -36,65 +114,72 @@ export const proxyConnectionForId = (
+ ctx: RequestContext,
+): void => {
+ // resolves the state in-place
+ traverseAndReplace(ctx.state, ctx) as ResolvedBindings<T, TBindings>;
+};
+
</file context>
✅ Addressed in ffe0104
tlgimenes
force-pushed
the
feat/flow
branch
2 times, most recently
from
acab7a3 to
531bd87
Compare
- Added a metrics mode selector to switch between Requests, Errors, and Latency. - Updated GatewayNode and ServerNode components to display metrics based on the selected mode. - Introduced a new useNodeMetrics hook to fetch and aggregate metrics for gateways and connections. - Removed unused MonitoringKPIs, RecentActivity, and TopTools components to streamline the codebase.
…ogic for gateways and connections. Updated comments for clarity and ensured consistent node positioning based on sorted order.
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
Introduces MCP Gateways and a new mesh mini map that visualizes gateways → MCP Mesh → servers with Requests/Errors/Latency metrics.
New Features
Migration
Written for commit ec03b50. Summary will update automatically on new commits.