fix(mcp): make paired settings/credential writes atomic

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,

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()