From 8ed9edbc8f0b5a923df724aced42ac0c86268d51 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Tue, 13 Oct 2026 16:33:13 +0800 Subject: [PATCH] fix(mcp): make paired settings/credential writes atomic Restore the settings half of an MCP server entry when OAuth credential cleanup fails after the config mutation, instead of leaving a silent half-paired write. If the rollback itself also fails, raise loudly with both residual states (C1 unverified semantics). --- core/application/mcp_service.py | 60 +++++++- tests/application/test_management_services.py | 139 +++++++++++++++++- 2 files changed, 195 insertions(+), 4 deletions(-) diff --git a/core/application/mcp_service.py b/core/application/mcp_service.py index 0e24ae54..51952941 100644 --- a/core/application/mcp_service.py +++ b/core/application/mcp_service.py @@ -18,7 +18,11 @@ from pydantic import ValidationError from core.application.config_store import ConfigStore, deep_merge -from core.application.errors import InvalidArgumentError, ProjectNotTrustedError +from core.application.errors import ( + ApplicationError, + InvalidArgumentError, + ProjectNotTrustedError, +) from core.application.project_service import ProjectService from core.config import home_config_path, project_config_path from core.domain.project import TrustState @@ -509,7 +513,11 @@ def mutate(current: dict[str, Any]) -> dict[str, Any]: return _replace_servers(current, selected) store.mutate(mutate) - _remove_replaced_oauth_credentials(clean_name, previous, patch) + try: + _remove_replaced_oauth_credentials(clean_name, previous, patch) + except Exception as credential_error: + _restore_server_entry(store, clean_name, previous, cause=credential_error) + raise self._publish_change() return self.list(project_id) @@ -537,7 +545,11 @@ def mutate(current: dict[str, Any]) -> dict[str, Any]: return _replace_servers(current, selected) store.mutate(mutate) - _remove_oauth_credentials(clean_name, previous) + try: + _remove_oauth_credentials(clean_name, previous) + except Exception as credential_error: + _restore_server_entry(store, clean_name, previous, cause=credential_error) + raise self._publish_change() return self.list(project_id) @@ -700,6 +712,48 @@ def _remove_oauth_credentials(name: str, previous: Any) -> None: McpOAuthCredentialStore.delete_endpoint(name, previous_url) +def _restore_server_entry( + store: ConfigStore, + name: str, + previous: Any, + *, + cause: BaseException, +) -> None: + """Roll back the settings half of a paired write whose credential half failed. + + A paired write (config entry + OAuth credentials) must land together or + not at all: if the credential cleanup fails after the settings mutation, + restore the exact ``previous`` entry (or drop it when the server was just + created) so the pair keeps its old version. If the restore itself fails, + raise loudly naming both residual states (C1 ``unverified`` semantics) — + never leave a silent half-pair behind. + """ + + def restore(current: dict[str, Any]) -> dict[str, Any]: + selected = _raw_servers(current) + if previous is None: + selected.pop(name, None) + else: + selected[name] = previous + return _replace_servers(current, selected) + + try: + store.mutate(restore) + except Exception as rollback_error: + raise ApplicationError( + "Paired MCP write ended half-applied: the settings entry and its " + f"OAuth credentials may disagree for {name!r}. Credential cleanup " + f"failed with {cause!r} and the settings rollback also failed with " + f"{rollback_error!r}. Inspect the MCP config and stored credentials " + "before retrying.", + details={ + "server": name, + "credential_error": repr(cause), + "rollback_error": repr(rollback_error), + }, + ) from rollback_error + + def _server_info( name: str, raw: Any, diff --git a/tests/application/test_management_services.py b/tests/application/test_management_services.py index 6b0c9e62..796ce505 100644 --- a/tests/application/test_management_services.py +++ b/tests/application/test_management_services.py @@ -7,9 +7,15 @@ import pytest from core.application import DeepCodeApplication -from core.application.errors import InvalidArgumentError, ProjectNotTrustedError +from core.application.config_store import ConfigStore +from core.application.errors import ( + ApplicationError, + InvalidArgumentError, + ProjectNotTrustedError, +) from core.application.views import mcp_server_view from core.domain import TrustState +from core.mcp.oauth import McpOAuthCredentialStore from core.persistence.migrations import LATEST_SCHEMA_VERSION from core.plugins.registry import LocalPluginRegistry from core.plugins.resolver import resolve_plugin @@ -857,3 +863,134 @@ def test_settings_update_refuses_a_stale_config_revision(tmp_path, monkeypatch): # Omitting the revision keeps the historical last-write-wins behavior. service.update({"agents": {"defaults": {"model": "gpt-5-mini"}}}) + + +def _seed_oauth_mcp_server(home: Path) -> dict: + entry = { + "type": "streamableHttp", + "url": "https://old.example.test/rpc", + "auth": "oauth", + "enabled": True, + } + _write_json(home / "deepcode_config.json", {"mcpServers": {"demo": dict(entry)}}) + return entry + + +def test_mcp_pair_writes_roll_back_settings_on_credential_cleanup_failure( + tmp_path: Path, + monkeypatch, +) -> None: + home = tmp_path / "home" + workspace = tmp_path / "workspace" + home.mkdir() + workspace.mkdir() + monkeypatch.setenv("DEEPCODE_HOME", str(home)) + monkeypatch.chdir(workspace) + original_entry = _seed_oauth_mcp_server(home) + application = DeepCodeApplication.open(tmp_path / "state.sqlite3") + try: + + def boom(*args: object, **kwargs: object) -> None: + raise OSError("credential store unavailable") + + monkeypatch.setattr(McpOAuthCredentialStore, "delete_endpoint", boom) + + with pytest.raises(OSError, match="credential store unavailable"): + application.mcp.upsert( + name="demo", + patch={"url": "https://new.example.test/rpc"}, + scope="user", + ) + config = json.loads((home / "deepcode_config.json").read_text(encoding="utf-8")) + assert config["mcpServers"]["demo"] == original_entry + + with pytest.raises(OSError, match="credential store unavailable"): + application.mcp.remove(name="demo", scope="user") + config = json.loads((home / "deepcode_config.json").read_text(encoding="utf-8")) + assert config["mcpServers"]["demo"] == original_entry + finally: + application.close() + + +def test_mcp_pair_writes_land_together_and_clean_credential_on_success( + tmp_path: Path, + monkeypatch, +) -> None: + home = tmp_path / "home" + workspace = tmp_path / "workspace" + home.mkdir() + workspace.mkdir() + monkeypatch.setenv("DEEPCODE_HOME", str(home)) + monkeypatch.chdir(workspace) + _seed_oauth_mcp_server(home) + deleted: list[tuple[str, str]] = [] + + def record(name: str, url: str) -> bool: + deleted.append((name, url)) + return True + + monkeypatch.setattr(McpOAuthCredentialStore, "delete_endpoint", record) + application = DeepCodeApplication.open(tmp_path / "state.sqlite3") + try: + application.mcp.upsert( + name="demo", + patch={"url": "https://new.example.test/rpc"}, + scope="user", + ) + config = json.loads((home / "deepcode_config.json").read_text(encoding="utf-8")) + assert config["mcpServers"]["demo"]["url"] == "https://new.example.test/rpc" + assert deleted == [("demo", "https://old.example.test/rpc")] + + application.mcp.remove(name="demo", scope="user") + config = json.loads((home / "deepcode_config.json").read_text(encoding="utf-8")) + assert "demo" not in config.get("mcpServers", {}) + assert deleted == [ + ("demo", "https://old.example.test/rpc"), + ("demo", "https://new.example.test/rpc"), + ] + finally: + application.close() + + +def test_mcp_pair_rollback_failure_raises_loud_unverified_error( + tmp_path: Path, + monkeypatch, +) -> None: + home = tmp_path / "home" + workspace = tmp_path / "workspace" + home.mkdir() + workspace.mkdir() + monkeypatch.setenv("DEEPCODE_HOME", str(home)) + monkeypatch.chdir(workspace) + _seed_oauth_mcp_server(home) + application = DeepCodeApplication.open(tmp_path / "state.sqlite3") + try: + + def boom(*args: object, **kwargs: object) -> None: + raise OSError("credential store unavailable") + + monkeypatch.setattr(McpOAuthCredentialStore, "delete_endpoint", boom) + + real_mutate = ConfigStore.mutate + calls = {"count": 0} + + def flaky(self, transform, *, expected_revision=None): + calls["count"] += 1 + if calls["count"] > 1: + raise OSError("settings rollback failed") + return real_mutate(self, transform, expected_revision=expected_revision) + + monkeypatch.setattr(ConfigStore, "mutate", flaky) + + with pytest.raises(ApplicationError, match="half-applied") as excinfo: + application.mcp.upsert( + name="demo", + patch={"url": "https://new.example.test/rpc"}, + scope="user", + ) + message = str(excinfo.value) + assert "credential store unavailable" in message + assert "settings rollback failed" in message + assert excinfo.value.details["server"] == "demo" + finally: + application.close()