diff --git a/docsgpt/api/user/tools/mcp.py b/docsgpt/api/user/tools/mcp.py index 8738d579..a3712a47 100644 --- a/docsgpt/api/user/tools/mcp.py +++ b/docsgpt/api/user/tools/mcp.py @@ -5,6 +5,7 @@ from urllib.parse import urlencode, urlparse from flask import current_app, jsonify, make_response, redirect, request from flask_restx import Namespace, Resource, fields +from docsgpt.agents.tool_pins import carry_pins_between from docsgpt.agents.tools.mcp_tool import MCPOAuthManager, MCPTool from docsgpt.api import api from docsgpt.api.user.tools.routes import transform_actions @@ -414,7 +415,8 @@ class MCPServerSave(Resource): "custom_name": display_name, "description": description, "config": storage_config, - "actions": transformed_actions, + # Fixed values the user set survive a re-save. + "actions": carry_pins_between(existing_doc.get("actions"), transformed_actions), "status": status_bool, "connection_id": connection_id, }, @@ -442,7 +444,7 @@ class MCPServerSave(Resource): "custom_name": display_name, "description": description, "config": storage_config, - "actions": transformed_actions, + "actions": carry_pins_between(existing_by_name.get("actions"), transformed_actions), "status": status_bool, "connection_id": connection_id, }, diff --git a/docsgpt/connectors/mcp.py b/docsgpt/connectors/mcp.py index ff4302f1..6087c2ba 100644 --- a/docsgpt/connectors/mcp.py +++ b/docsgpt/connectors/mcp.py @@ -2,6 +2,7 @@ from __future__ import annotations +from docsgpt.agents.tool_pins import carry_pins from docsgpt.connectors import catalog, service from docsgpt.connectors.permissions import apply_default_permissions from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository @@ -43,9 +44,10 @@ def discover_builtin_actions(user_id: str, connection: dict) -> list[dict]: def refresh_mcp_tools(user_id: str, connection: dict) -> dict: """Re-read the actions of every MCP tool on a connection. - Actions that still exist keep the permissions the user chose; new ones - get the defaults from their annotations (reads always allowed, writes - need approval); removed ones disappear. + Actions that still exist keep the permissions the user chose and the + fixed values of parameters they still have; new ones get the defaults + from their annotations (reads always allowed, writes need approval); + removed ones disappear. Returns: ``{"added": [...], "removed": [...], "tools": [...]}``. @@ -64,7 +66,7 @@ def refresh_mcp_tools(user_id: str, connection: dict) -> dict: old = previous.get(action.get("name")) if old is not None: action = { - **action, + **carry_pins(old, action), "active": old.get("active", True), "require_approval": bool(old.get("require_approval")), } diff --git a/tests/api/user/test_tools_mcp_pg.py b/tests/api/user/test_tools_mcp_pg.py index 0fc602fb..1b02f80d 100644 --- a/tests/api/user/test_tools_mcp_pg.py +++ b/tests/api/user/test_tools_mcp_pg.py @@ -404,6 +404,47 @@ class TestMCPServerSave: response = MCPServerSave().post() assert response.status_code in (200, 201) + def test_saving_an_existing_server_keeps_fixed_values(self, app, pg_conn): + from docsgpt.api.user.tools.mcp import MCPServerSave + from docsgpt.storage.db.repositories.user_tools import UserToolsRepository + + user = "u-mcp-resave" + repo = UserToolsRepository(pg_conn) + existing = repo.create( + user, "mcp_tool", + config={"server_url": "https://example.com/mcp", "auth_type": "none"}, + display_name="My MCP", + actions=[{ + "name": "search", + "active": True, + "parameters": {"properties": { + "q": {"type": "string", "filled_by_llm": True, "value": ""}, + "team": {"type": "string", "filled_by_llm": False, "value": "ENG"}, + }}, + }], + ) + fake_tool = MagicMock() + fake_tool.get_actions_metadata.return_value = [{ + "name": "search", + "parameters": {"properties": {"q": {"type": "string"}, "team": {"type": "string"}}}, + }] + with _patch_db(pg_conn), patch( + "docsgpt.api.user.tools.mcp.MCPTool", return_value=fake_tool, + ), app.test_request_context( + "/api/mcp_server/save", method="POST", + json={ + "id": str(existing["id"]), + "displayName": "My MCP", + "config": {"transport_type": "http", "server_url": "https://example.com/mcp", "auth_type": "none"}, + }, + ): + from flask import request + request.decoded_token = {"sub": user} + response = MCPServerSave().post() + assert response.status_code == 200 + team = repo.get_any(str(existing["id"]), user)["actions"][0]["parameters"]["properties"]["team"] + assert team["filled_by_llm"] is False and team["value"] == "ENG" + class TestMCPOAuthCallback: def test_error_param_redirects_error(self, app): diff --git a/tests/connectors/test_mcp_refresh.py b/tests/connectors/test_mcp_refresh.py index f03be8a0..957df212 100644 --- a/tests/connectors/test_mcp_refresh.py +++ b/tests/connectors/test_mcp_refresh.py @@ -81,6 +81,27 @@ class TestRefresh: assert actions["create_issue"]["parameters"]["properties"]["q"]["filled_by_llm"] is True assert [t["id"] for t in result["tools"]] == [tool_id] + def test_fixed_values_survive_for_parameters_that_still_exist(self, pg_conn): + connection = _connection(pg_conn) + pinned = _action("search_issues") + pinned["parameters"]["properties"] = { + "q": {"type": "string", "filled_by_llm": True, "value": ""}, + "team": {"type": "string", "filled_by_llm": False, "value": "ENG"}, + "gone": {"type": "string", "filled_by_llm": False, "value": "x"}, + } + tool_id = _tool(pg_conn, connection["id"], [pinned]) + fresh = _action("search_issues") + fresh["parameters"]["properties"] = {"q": {"type": "string"}, "team": {"type": "string"}} + with _db(pg_conn), patch.object(mcp, "_discover", return_value=[fresh]): + mcp.refresh_mcp_tools("alice", connection) + action = pg_conn.execute( + text("SELECT actions FROM user_tools WHERE id = CAST(:i AS uuid)"), {"i": tool_id} + ).scalar()[0] + properties = action["parameters"]["properties"] + assert properties["team"] == {"type": "string", "filled_by_llm": False, "value": "ENG"} + assert properties["q"]["filled_by_llm"] is True + assert "gone" not in properties + def test_other_tools_on_the_connection_are_left_alone(self, pg_conn): connection = _connection(pg_conn) pg_conn.execute(