Add remote mcp support #63
Conversation
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…ovements Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…support Add remote MCP server support with settings UI
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Enable BuildConfig generation for memory-vault debug build
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
[WIP] Check AI models access to MCP servers and fix any issues
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…ns field Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
- Fix parseResponse() to always try SSE parsing regardless of transport type - Rename test class from McpServerIntegrationTest to McpServerTest - Use exact assertions instead of permissive contains() checks - Add helper function documentation and UUID format validation - Reduce UUID test iterations from 100 to 10 for efficiency Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…p-server Add MCP server integration tests
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for remote MCP (Model Context Protocol) servers, enabling the application to connect to external tools and services. The implementation includes database persistence, UI management screens, service layer integration with the LLM, and comprehensive test coverage.
Changes:
- Added complete MCP server infrastructure including database entity, DAO, repository, and client service for managing remote server configurations
- Integrated MCP tool calling into the chat workflow with automatic tool synchronization and execution
- Implemented a full-featured UI for managing MCP servers with connection testing, real-time status tracking, and security warnings
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/java/com/dark/tool_neuron/models/table_schema/McpServer.kt | Defines the MCP server entity with transport types and connection status enums |
| app/src/main/java/com/dark/tool_neuron/database/dao/McpServerDao.kt | Room DAO interface for CRUD operations on MCP servers |
| app/src/main/java/com/dark/tool_neuron/database/AppDatabase.kt | Adds migration 4→5 to create mcp_servers table |
| app/src/main/java/com/dark/tool_neuron/models/converters/Converters.kt | Type converters for McpTransportType enum |
| app/src/main/java/com/dark/tool_neuron/repo/McpServerRepository.kt | Repository layer with URL validation and connection status tracking |
| app/src/main/java/com/dark/tool_neuron/service/McpClientService.kt | HTTP client for MCP protocol communication with SSE support |
| app/src/main/java/com/dark/tool_neuron/service/McpToolMapper.kt | Maps MCP tools to LLM-compatible function calling format |
| app/src/main/java/com/dark/tool_neuron/viewmodel/McpServerViewModel.kt | ViewModel for managing server list and connection testing |
| app/src/main/java/com/dark/tool_neuron/viewmodel/ChatViewModel.kt | Integrates MCP tool calling into chat generation flow |
| app/src/main/java/com/dark/tool_neuron/viewmodel/factory/ChatViewModelFactory.kt | Updates factory to inject MCP dependencies |
| app/src/main/java/com/dark/tool_neuron/ui/screen/McpServersScreen.kt | Complete UI for managing MCP servers with dialogs and validation |
| app/src/main/java/com/dark/tool_neuron/ui/screen/home_screen/HomeScreen.kt | Adds navigation callback for MCP servers screen |
| app/src/main/java/com/dark/tool_neuron/ui/screen/home_screen/HomeDrawerScreen.kt | Adds cloud icon button to access MCP servers |
| app/src/main/java/com/dark/tool_neuron/activity/MainActivity.kt | Adds navigation route and composable for MCP servers screen |
| app/src/main/java/com/dark/tool_neuron/worker/LlmModelWorker.kt | Adds methods to set and clear GGUF tools JSON |
| app/src/main/java/com/dark/tool_neuron/di/HiltModules.kt | Provides McpServerRepository and McpClientService via Hilt |
| app/src/main/java/com/dark/tool_neuron/di/AppContainer.kt | Registers MCP dependencies in manual DI container |
| app/src/test/java/com/dark/tool_neuron/service/McpToolMapperTest.kt | Unit tests for tool mapping functionality |
| app/src/test/java/com/dark/tool_neuron/integration/McpServerTest.kt | Integration tests for MCP protocol and tool handling |
| .github/workflows/build-debug-apk.yml | CI workflow to build and upload debug APKs |
| memory-vault/build.gradle.kts | Enables buildConfig feature |
| gradle/libs.versions.toml | Adds org.json test dependency version |
| app/build.gradle.kts | Adds junit and org.json test dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| androidx-espresso-core = "3.7.0" | ||
| androidx-junit = "1.3.0" | ||
| junit = "4.13.2" | ||
| org-json = "20240303" |
There was a problem hiding this comment.
The org.json dependency is being added for testing, but this library is already included in Android SDK. Android includes org.json as part of the platform since API level 1, so this dependency is unnecessary and redundant. Consider removing this test dependency to avoid potential version conflicts or duplicate classes.
| org-json = "20240303" |
| // Shut down the executor service used by the dispatcher | ||
| httpClient.dispatcher.executorService.shutdown() | ||
| // Evict all connections from the connection pool | ||
| httpClient.connectionPool.evictAll() | ||
| // Close any configured cache | ||
| httpClient.cache?.close() | ||
| } catch (e: Exception) { | ||
| Log.w(TAG, "Error while closing OkHttpClient resources", e) |
There was a problem hiding this comment.
The close method shuts down the OkHttpClient resources, but this singleton service may be reused after close() is called. Once the dispatcher is shut down and connections are evicted, subsequent HTTP requests will fail. Consider whether this close() method should be called at all for a singleton service, or implement a mechanism to prevent usage after closing.
| // Shut down the executor service used by the dispatcher | |
| httpClient.dispatcher.executorService.shutdown() | |
| // Evict all connections from the connection pool | |
| httpClient.connectionPool.evictAll() | |
| // Close any configured cache | |
| httpClient.cache?.close() | |
| } catch (e: Exception) { | |
| Log.w(TAG, "Error while closing OkHttpClient resources", e) | |
| // For a @Singleton-scoped service, we avoid fully shutting down the OkHttpClient, | |
| // as that would make the client unusable for any subsequent requests. | |
| // Evicting connections is safe and helps clean up idle resources without | |
| // preventing future use of this client. | |
| httpClient.connectionPool.evictAll() | |
| Log.i(TAG, "McpClientService.close() called on singleton; evicted idle connections but kept OkHttpClient active for reuse.") | |
| } catch (e: Exception) { | |
| Log.w(TAG, "Error while cleaning up OkHttpClient resources", e) |
| private fun parseSseResponse(responseBody: String): String { | ||
| // Check if this is an SSE response | ||
| if (!responseBody.contains("data:")) { | ||
| // Not SSE format, return as-is | ||
| return responseBody | ||
| } | ||
|
|
||
| // Split by double newlines to separate events | ||
| val events = responseBody.split("\n\n") | ||
|
|
||
| // Find the last event with data (for request/response pattern) | ||
| for (event in events.reversed()) { | ||
| val lines = event.lines() | ||
| val dataLines = lines.filter { it.startsWith("data:") } | ||
|
|
||
| if (dataLines.isNotEmpty()) { | ||
| // Extract JSON from "data: {...}" format | ||
| // Multiple data lines in same event should be joined with newlines per SSE spec | ||
| val joinedData = dataLines.joinToString("\n") { it.removePrefix("data:").trim() } | ||
|
|
||
| // Validate that the joined data is valid JSON to avoid propagating malformed JSON-RPC | ||
| return try { | ||
| JSONObject(joinedData) | ||
| joinedData | ||
| } catch (e: Exception) { | ||
| Log.w(TAG, "SSE data is not valid JSON; returning raw SSE response body", e) | ||
| responseBody | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: return original response | ||
| return responseBody | ||
| } |
There was a problem hiding this comment.
The parseSseResponse method handles SSE format by splitting on double newlines and reversing to find the last event. However, if the response contains multiple events with the last event being incomplete or malformed, this could return incorrect data. The validation only checks if joined data is valid JSON but doesn't verify it contains expected JSON-RPC fields. Consider adding validation that the parsed result is a valid JSON-RPC response object.
| } catch (e: Exception) { | ||
| Log.e(TAG, "Failed to list tools: ${e.message}", e) | ||
| emptyList() | ||
| } |
There was a problem hiding this comment.
The error handling returns an empty list when listTools fails, which makes it impossible to distinguish between a server that has no tools and a server where the request failed. This could lead to silent failures where users think a server has no tools when actually the connection failed. Consider logging the error or propagating it to the caller so they can handle it appropriately.
| } catch (e: Exception) { | ||
| val message = "Failed to refresh MCP tools: ${e.message}" | ||
| _error.value = message | ||
| AppStateManager.setError(message) | ||
| } | ||
| } |
There was a problem hiding this comment.
The syncMcpTools method catches all exceptions and sets an error state, but then continues execution. When an exception occurs during tool synchronization, the method should ensure that tools are cleared and the LLM is not left in an inconsistent state with partially loaded tools. Consider explicitly clearing tools in the catch block.
| showSecurityWarning -> { | ||
| { | ||
| Text( | ||
| "Warning: Using HTTP with an API key is insecure. Use HTTPS for secure connections.", | ||
| color = MaterialTheme.colorScheme.error | ||
| ) | ||
| } |
There was a problem hiding this comment.
In the UI dialog, when both the URL uses HTTP (not HTTPS) and an API key is present, a security warning is shown. However, this warning is only displayed in the UI and doesn't prevent the user from saving an insecure configuration. Consider adding a similar validation in the repository layer that either prevents saving insecure configurations or at least logs a warning, to catch cases where the UI might be bypassed.
|
|
||
| // Tests | ||
| testImplementation(libs.junit) | ||
| testImplementation(libs.org.json) |
There was a problem hiding this comment.
The org.json dependency is being added for testing, but this library is already included in the Android SDK. This creates redundant dependencies and potential version conflicts. Consider removing this dependency since org.json is available by default in Android.
| testImplementation(libs.org.json) |
| val reference = mcpToolRegistry[toolCall.name] | ||
| ?: return Result.failure(Exception("Tool not found: ${toolCall.name}")) | ||
| return mcpClientService.callTool(reference.server, reference.toolName, toolCall.argsJson) |
There was a problem hiding this comment.
The executeToolCall method looks up a tool in mcpToolRegistry but this registry is populated asynchronously by syncMcpTools(). If the registry hasn't been populated yet or was cleared due to an error, the tool lookup will fail. There's a race condition where a tool call could be executed before syncMcpTools() completes, especially if a chat is continued immediately after the ViewModel is created.
| val reference = mcpToolRegistry[toolCall.name] | |
| ?: return Result.failure(Exception("Tool not found: ${toolCall.name}")) | |
| return mcpClientService.callTool(reference.server, reference.toolName, toolCall.argsJson) | |
| // Try to resolve the tool from the current registry snapshot. | |
| var reference = mcpToolRegistry[toolCall.name] | |
| // If the registry has not been populated yet or was cleared, attempt to sync tools | |
| // and retry the lookup to avoid a race with asynchronous initialization. | |
| if (reference == null) { | |
| try { | |
| // Ensure MCP tools are synchronized before giving up. | |
| syncMcpTools() | |
| reference = mcpToolRegistry[toolCall.name] | |
| } catch (e: Exception) { | |
| return Result.failure( | |
| Exception( | |
| "Failed to initialize MCP tools for ${toolCall.name}: ${e.message}", | |
| e | |
| ) | |
| ) | |
| } | |
| } | |
| val resolvedReference = reference | |
| ?: return Result.failure( | |
| Exception("Tool not found or MCP tools not initialized: ${toolCall.name}") | |
| ) | |
| return mcpClientService.callTool( | |
| resolvedReference.server, | |
| resolvedReference.toolName, | |
| toolCall.argsJson | |
| ) |
|
|
||
| // Use fixed ID for deterministic test behavior | ||
| val request = JSONObject().apply { | ||
| put("jsonrpc", "2.0") | ||
| put("id", 123L) | ||
| put("method", "tools/call") | ||
| put("params", JSONObject().apply { | ||
| put("name", toolName) | ||
| put("arguments", arguments) | ||
| }) | ||
| } | ||
|
|
||
| assertEquals("2.0", request.getString("jsonrpc")) | ||
| assertEquals(123L, request.getLong("id")) | ||
| assertEquals("tools/call", request.getString("method")) | ||
|
|
||
| val params = request.getJSONObject("params") | ||
| assertEquals(toolName, params.getString("name")) | ||
|
|
There was a problem hiding this comment.
The test builds a tool call request with a fixed ID of 123, but in the actual implementation (McpClientService.callToolInternal), the ID is generated using System.currentTimeMillis(). This inconsistency between test and production code could mask issues. Consider using the same ID generation strategy in tests or documenting why they differ.
| // Use fixed ID for deterministic test behavior | |
| val request = JSONObject().apply { | |
| put("jsonrpc", "2.0") | |
| put("id", 123L) | |
| put("method", "tools/call") | |
| put("params", JSONObject().apply { | |
| put("name", toolName) | |
| put("arguments", arguments) | |
| }) | |
| } | |
| assertEquals("2.0", request.getString("jsonrpc")) | |
| assertEquals(123L, request.getLong("id")) | |
| assertEquals("tools/call", request.getString("method")) | |
| val params = request.getJSONObject("params") | |
| assertEquals(toolName, params.getString("name")) | |
| // Use same ID generation strategy as production while keeping the test deterministic | |
| val requestId = System.currentTimeMillis() | |
| val request = JSONObject().apply { | |
| put("jsonrpc", "2.0") | |
| put("id", requestId) | |
| put("method", "tools/call") | |
| put("params", JSONObject().apply { | |
| put("name", toolName) | |
| put("arguments", arguments) | |
| }) | |
| } | |
| assertEquals("2.0", request.getString("jsonrpc")) | |
| assertEquals(requestId, request.getLong("id")) | |
| assertEquals("tools/call", request.getString("method")) | |
| val params = request.getJSONObject("params") | |
| assertEquals(toolName, params.getString("name")) |
| fun sanitizeIdentifier(value: String): String { | ||
| return value.lowercase() | ||
| .replace(Regex("[^a-z0-9]+"), "_") | ||
| .trim('_') | ||
| } |
There was a problem hiding this comment.
The sanitizeIdentifier method converts to lowercase and replaces non-alphanumeric characters with underscores, then trims underscores from the ends. However, if the input contains consecutive non-alphanumeric characters, this will create consecutive underscores in the middle of the identifier. For example, "My--Tool" becomes "my__tool". Consider replacing multiple consecutive underscores with a single underscore for cleaner identifiers.
|
@Godzilla675 if possible can you share a working apk ? No need to sign it just a debug one |
|
@Godzilla675 |
|
Till then add a MCP store or something + try including Termux like terminal, so users can run python in phone so more mcp servers get enabled |
|
@Siddhesh2377 hi I'm happy to help. Do you still need the debug apk? |
|
Not now I built it, just add those features, and I would love to merge it, also update your branch with my changes |
|
and for the MCP store should I add remote MCP servers like Zapier? or do you mean local servers? |
|
Can you join a Google meet ?, if you are comfortable ? |
2 participants
This pull request introduces support for managing MCP (Model Context Protocol) server configurations in the application. It adds a new Room entity and DAO for MCP servers, a repository for server management, dependency injection wiring, and UI navigation for a new MCP servers screen. Additionally, it includes a workflow for building debug APKs and adds test dependencies. The most important changes are grouped below:
MCP Server Management (Core Functionality):
McpServerentity,McpTransportTypeandMcpConnectionStatusenums for representing remote MCP server configurations inMcpServer.kt.McpServerDaointerface for CRUD operations on MCP servers in the Room database.McpServerRepositoryclass for managing MCP server configurations, including runtime connection status tracking and server validation.Database and Migration:
AppDatabaseto include themcp_serverstable and integrated a migration (version 4→5) to create this table. Also added the DAO to the database contract. [1] [2]McpTransportTypeenum. [1] [2]Dependency Injection and Service Wiring:
McpServerRepositoryandMcpClientServicein both the manualAppContainerand Hilt modules for dependency injection. [1] [2] [3] [4] [5] [6]UI Navigation Integration:
MainActivity.kt. [1] [2] [3] [4]CI and Testing:
junit,org.json) in the Gradle build file.All changes have been tested locally on my phone and are ready for production