fix windows mcp shell

src/backend/base/langflow/api/utils/mcp/default_servers.py

@@ -1,5 +1,7 @@
 """Orchestrator that auto-installs the default MCP servers (registry-driven) for every user."""
 
+import platform
+
 from fastapi import HTTPException
 from lfx.log.logger import logger
 from lfx.services.deps import get_settings_service
@@ -17,6 +19,35 @@ from langflow.services.database.models.user.model import User
 from langflow.services.deps import get_service
 from langflow.services.schema import ServiceType
 
+# Metadata fields the spec authors. We compare these (and only these) when
+# deciding whether a persisted auto-configured entry has drifted from the
+# canonical spec — extra metadata keys (added by future API extensions or
+# user tooling) must not trigger spurious reconciliations.
+_SPEC_OWNED_METADATA_KEYS: tuple[str, ...] = (
+    "description",
+    "auto_configured",
+    "langflow_internal",
+    "startup_timeout_seconds",
+)
+
+
+def _is_persisted_payload_in_sync(persisted: dict, canonical: dict) -> bool:
+    """Return True iff the persisted MCP entry already matches the canonical spec.
+
+    Compares only the fields the spec controls (command, args, env, and the
+    spec-owned metadata keys). Extra fields on either side are ignored so we
+    don't thrash on derived / user-added properties.
+    """
+    if persisted.get("command") != canonical["command"]:
+        return False
+    if list(persisted.get("args") or []) != list(canonical["args"]):
+        return False
+    if dict(persisted.get("env") or {}) != dict(canonical["env"]):
+        return False
+    persisted_meta = persisted.get("metadata") or {}
+    canonical_meta = canonical["metadata"]
+    return all(persisted_meta.get(key) == canonical_meta.get(key) for key in _SPEC_OWNED_METADATA_KEYS)
+
 
 def _build_server_payload(spec: DefaultMcpServerSpec) -> dict:
     """Build the persisted payload, re-validating via MCPServerConfig.
@@ -70,15 +101,41 @@ async def auto_configure_default_mcp_servers(session: AsyncSession) -> None:
         payload = _build_server_payload(spec)
         for user in users:
             try:
-                existing = await get_server_list(user, session, storage_service, settings_service)
-                if server_name in existing.get("mcpServers", {}):
-                    await logger.adebug(
-                        "default_mcp_server_skipped",
+                existing_servers = (await get_server_list(user, session, storage_service, settings_service)).get(
+                    "mcpServers", {}
+                )
+                persisted = existing_servers.get(server_name)
+                if persisted is not None:
+                    persisted_meta = persisted.get("metadata") or {}
+                    if not persisted_meta.get("auto_configured"):
+                        # User-owned entry (no `auto_configured` flag): never overwrite.
+                        await logger.adebug(
+                            "default_mcp_server_skipped",
+                            user_id=str(user.id),
+                            server_name=server_name,
+                            reason="user_owned",
+                        )
+                        continue
+                    if _is_persisted_payload_in_sync(persisted, payload):
+                        await logger.adebug(
+                            "default_mcp_server_skipped",
+                            user_id=str(user.id),
+                            server_name=server_name,
+                            reason="already_in_sync",
+                        )
+                        continue
+                    # Spec drifted (e.g. user upgraded across a commit that changed the
+                    # canonical payload). Reconcile by overwriting with the new spec —
+                    # `auto_configured: True` was our marker that the entry is ours.
+                    # Platform context lets ops correlate reconciliations against the
+                    # reported OS when the prior version's payload is too short for
+                    # that OS (notably first-run npx timeouts on Windows).
+                    await logger.ainfo(
+                        "default_mcp_server_reconciled",
                         user_id=str(user.id),
                         server_name=server_name,
-                        reason="already_exists",
+                        platform=platform.system(),
                     )
-                    continue
                 await update_server(
                     server_name=server_name,
                     server_config=payload,

src/backend/tests/unit/api/utils/mcp/test_default_servers_orchestrator.py

@@ -142,6 +142,99 @@ class TestIdempotency:
         patched_orchestrator_deps.update_server.assert_not_called()
 
 
+class TestReconcileAutoConfiguredEntries:
+    """Reconcile auto-configured entries when the canonical spec drifts.
+
+    When the canonical spec evolves (e.g. a new `metadata.startup_timeout_seconds`
+    field is added), users who installed Langflow on an earlier build keep a
+    stale persisted payload — the orchestrator's old skip-if-exists behavior
+    would lock them out of fixes forever (notably: the 60s opt-in needed to
+    survive the first `npx -y @wonderwhy-er/desktop-commander@latest` download
+    on Windows + slow networks).
+
+    Reconciliation is gated on `metadata.auto_configured == True`: only entries
+    that we wrote can be overwritten. Anything missing the flag (e.g. the user
+    swapped in their own server) is preserved unconditionally.
+    """
+
+    async def test_should_reconcile_auto_configured_entry_when_persisted_payload_diverges_from_spec(
+        self, fake_session_with_users, patched_orchestrator_deps
+    ):
+        """Stale entry from a pre-`startup_timeout_seconds` build must be updated."""
+        patched_orchestrator_deps.get_server_list.return_value = {
+            "mcpServers": {
+                "shell-execution": {
+                    "command": "npx",
+                    "args": ["-y", "@wonderwhy-er/desktop-commander@latest"],
+                    "env": {},
+                    "metadata": {
+                        "description": (
+                            "Cross-platform shell execution + filesystem control (wonderwhy-er/desktop-commander)."
+                        ),
+                        "auto_configured": True,
+                        "langflow_internal": True,
+                        # NOTE: no startup_timeout_seconds — this is the stale field.
+                    },
+                }
+            }
+        }
+
+        await auto_configure_default_mcp_servers(fake_session_with_users)
+
+        patched_orchestrator_deps.update_server.assert_awaited_once()
+        cfg = patched_orchestrator_deps.update_server.await_args.kwargs["server_config"]
+        assert cfg["metadata"].get("startup_timeout_seconds") == 60
+
+    async def test_should_not_overwrite_entry_lacking_auto_configured_flag(
+        self, fake_session_with_users, patched_orchestrator_deps
+    ):
+        """User-owned entry (no `auto_configured` flag) must be preserved."""
+        patched_orchestrator_deps.get_server_list.return_value = {
+            "mcpServers": {
+                "shell-execution": {
+                    "command": "npx",
+                    "args": ["-y", "@user/custom-fork"],
+                    "env": {},
+                    "metadata": {"description": "user-owned override"},
+                }
+            }
+        }
+
+        await auto_configure_default_mcp_servers(fake_session_with_users)
+
+        patched_orchestrator_deps.update_server.assert_not_called()
+
+    async def test_should_skip_when_auto_configured_entry_payload_already_matches_spec(
+        self, fake_session_with_users, patched_orchestrator_deps
+    ):
+        """No-op when the persisted payload already matches the canonical spec.
+
+        Avoids needless disk writes and noisy logs on every Langflow startup.
+        """
+        from langflow.api.utils.mcp.default_servers_specs import DEFAULT_MCP_SERVERS
+
+        spec = DEFAULT_MCP_SERVERS["shell-execution"]
+        patched_orchestrator_deps.get_server_list.return_value = {
+            "mcpServers": {
+                "shell-execution": {
+                    "command": spec.config.command,
+                    "args": list(spec.config.args),
+                    "env": dict(spec.config.env),
+                    "metadata": {
+                        "description": spec.description,
+                        "auto_configured": True,
+                        "langflow_internal": True,
+                        "startup_timeout_seconds": spec.startup_timeout_seconds,
+                    },
+                }
+            }
+        }
+
+        await auto_configure_default_mcp_servers(fake_session_with_users)
+
+        patched_orchestrator_deps.update_server.assert_not_called()
+
+
 class TestRegistryRevalidation:
     """Defense-in-depth on the registry.