Skip to content

if lets say user has connected MCP servers with klavis, we want it to accessible#329

Open
shivammittal274 wants to merge 2 commits intomainfrom
feat/XUfT6NJd-if-lets-say-user-has-connected-mcp-serve
Open

if lets say user has connected MCP servers with klavis, we want it to accessible#329
shivammittal274 wants to merge 2 commits intomainfrom
feat/XUfT6NJd-if-lets-say-user-has-connected-mcp-serve

Conversation

@shivammittal274
Copy link
Contributor

Summary

if lets say user has connected MCP servers with klavis, we want it to accessible over browserOS MCP, so basically this will make only 1 mcp url then, if we can merge klavis to browseros mcp itself

Changes

apps/server/src/api/routes/chat.ts             |   3 -
 apps/server/src/api/routes/klavis.ts           |  25 ++-
 apps/server/src/api/routes/mcp.ts              |  39 +++-
 apps/server/src/api/server.ts                  |   7 +-
 apps/server/src/api/services/chat-service.ts   |  28 +--
 apps/server/src/api/types.ts                   |   4 +
 apps/server/src/lib/klavis-mcp-proxy.ts        | 176 ++++++++++++++++++
 apps/server/src/main.ts                        |  16 +-
 apps/server/tests/lib/klavis-mcp-proxy.test.ts | 242 +++++++++++++++++++++++++
 packages/shared/src/constants/timeouts.ts      |   6 +
 10 files changed, 510 insertions(+), 36 deletions(-)

Agent Metadata

  • Total cost: $12.6568
  • Stages:
    • ok setup ($0.0000, 43.0s)
    • ok plan ($6.4311, 1011.1s)
    • ok implement ($6.2257, 958.6s)

Generated by coding-agent v3

BrowserOS Coding Agent added 2 commits February 12, 2026 12:43
@github-actions
Copy link
Contributor

Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement.

To sign the CLA, please add a comment to this PR with the following text:

I have read the CLA Document and I hereby sign the CLA

You only need to sign once. After signing, this check will pass automatically.


Troubleshooting
  • Already signed but still failing? Comment recheck to trigger a re-verification.
  • Signed with a different email? Make sure your commit email matches your GitHub account email, or add your commit email to your GitHub account.
- - - I have read the CLA Document and I hereby sign the CLA - - - **BrowserOS Coding Agent** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).
You can retrigger this bot by commenting **recheck** in this Pull Request. Posted by the **CLA Assistant Lite bot**.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 12, 2026

Greptile Overview

Greptile Summary

This PR consolidates Klavis MCP server integration by introducing a proxy layer that connects to Klavis Strata on startup, discovers upstream tools, and makes them available through the BrowserOS MCP endpoint. Previously, Klavis tools were only accessible when chat sessions explicitly requested them - now they're always available if the user has authenticated integrations.

Key changes:

  • Created KlavisMcpProxy class to manage persistent connection to Klavis Strata
  • Integrated proxy into MCP routes to expose upstream Klavis tools alongside native BrowserOS tools
  • Removed duplicate Klavis Strata creation logic from chat service
  • Added automatic refresh when users add/remove/authenticate Klavis integrations
  • Implemented periodic refresh (5 minutes) to detect integration changes

Issues found:

  • Multiple interval timers accumulate during reconnections (line 146 in klavis-mcp-proxy.ts)
  • Race condition when tools change during active requests (lines 203-206 in mcp.ts)
  • Missing timeout on initial MCP client connection (line 60 in klavis-mcp-proxy.ts)

Confidence Score: 2/5

  • Not safe to merge - contains critical bugs that will cause resource leaks and incorrect behavior
  • The interval timer leak on line 146 will cause multiple refresh timers to run simultaneously after each reconnection, leading to resource exhaustion. The race condition on lines 203-206 means requests could use stale tool registrations. Missing connection timeout could cause startup hangs.
  • Pay close attention to apps/server/src/lib/klavis-mcp-proxy.ts (interval timer leak) and apps/server/src/api/routes/mcp.ts (race condition)

Important Files Changed

Filename Overview
apps/server/src/lib/klavis-mcp-proxy.ts New proxy class to connect to Klavis Strata, discover upstream MCP tools, and proxy tool calls with periodic refresh
apps/server/src/api/routes/mcp.ts Integrated Klavis MCP proxy to register upstream tools and rebuild server when tools change - potential race condition with tool registration
apps/server/src/main.ts Initialized KlavisMcpProxy on startup and passed to HTTP server
apps/server/src/api/routes/klavis.ts Added refresh calls to proxy after add/remove/submit operations
apps/server/src/api/services/chat-service.ts Removed duplicate Klavis Strata creation logic - now handled by proxy

Sequence Diagram

sequenceDiagram
    participant App as Application (main.ts)
    participant Proxy as KlavisMcpProxy
    participant Klavis as KlavisClient
    participant Strata as Klavis Strata
    participant MCP as MCP Routes
    participant Client as MCP Client

    Note over App,Strata: Server Startup
    App->>Proxy: new KlavisMcpProxy(klavisClient, browserosId)
    App->>Proxy: connect()
    Proxy->>Klavis: getUserIntegrations(browserosId)
    Klavis-->>Proxy: [authenticated integrations]
    Proxy->>Klavis: createStrata(browserosId, names)
    Klavis-->>Proxy: strataServerUrl
    Proxy->>Strata: client.connect(transport)
    Strata-->>Proxy: connected
    Proxy->>Strata: client.listTools()
    Strata-->>Proxy: [upstream tools]
    Note over Proxy: Start 5-min refresh interval
    
    Note over MCP,Client: MCP Request Handling
    Client->>MCP: MCP request
    MCP->>MCP: createMcpServerWithTools()
    Note over MCP: Register BrowserOS tools
    Note over MCP: Register Klavis proxy tools
    MCP->>Proxy: callTool(name, args)
    Proxy->>Strata: client.callTool()
    Strata-->>Proxy: result
    Proxy-->>MCP: CallToolResult
    MCP-->>Client: response
    
    Note over Proxy,Strata: Integration Changes
    Note over Proxy: User adds/removes integration
    Proxy->>Proxy: refresh()
    Proxy->>Klavis: getUserIntegrations()
    Klavis-->>Proxy: [updated integrations]
    alt Integration set changed
        Proxy->>Proxy: disconnectClient()
        Proxy->>Proxy: connect()
        Proxy->>MCP: onToolsChanged()
        MCP->>MCP: rebuild mcpServer
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

})

await this.disconnectClient()
await this.connect()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recursive connect() call during refresh() will restart the interval timer, causing multiple timers to run

Each call to connect() starts a new setInterval on line 78, but the old interval from the previous connection isn't cleared before reconnecting.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/lib/klavis-mcp-proxy.ts
Line: 146:146

Comment:
recursive `connect()` call during `refresh()` will restart the interval timer, causing multiple timers to run

Each call to `connect()` starts a new `setInterval` on line 78, but the old interval from the previous connection isn't cleared before reconnecting.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +203 to +206
deps.klavisMcpProxy.onToolsChanged = () => {
mcpServer = createMcpServerWithTools(deps)
logger.info('MCP server rebuilt with updated Klavis tools')
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

race condition: requests in-flight when tools change will use the old mcpServer instance

When onToolsChanged callback reassigns mcpServer, concurrent requests that already captured the old reference will continue using outdated tool registrations.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/api/routes/mcp.ts
Line: 203:206

Comment:
race condition: requests in-flight when tools change will use the old `mcpServer` instance

When `onToolsChanged` callback reassigns `mcpServer`, concurrent requests that already captured the old reference will continue using outdated tool registrations.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +60 to +64
await client.connect(transport)

const listResult = await client.listTools(undefined, {
signal: AbortSignal.timeout(TIMEOUTS.MCP_UPSTREAM_LIST_TOOLS),
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no timeout for client.connect() - could hang indefinitely

All other MCP operations use AbortSignal.timeout(), but the connection itself has no timeout protection.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/lib/klavis-mcp-proxy.ts
Line: 60:64

Comment:
no timeout for `client.connect()` - could hang indefinitely

All other MCP operations use `AbortSignal.timeout()`, but the connection itself has no timeout protection.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant