fix: silent error handling and defensive improvements

apps/agentstack-sdk-py/src/agentstack_sdk/server/server.py

@@ -55,7 +55,7 @@ def __init__(self) -> None:
         self._self_registration_client: PlatformClient | None = None
         self._self_registration_id: str | None = None
         self._provider_id: str | None = None
-        self._all_configured_variables: set[str] = set()
+        self._all_configured_variables: dict[str, str] = {}
 
     @functools.wraps(agent_decorator)
     def agent(self, *args, **kwargs) -> Callable:
@@ -312,14 +312,14 @@ async def _load_variables(self, first_run: bool = False) -> None:
             return
 
         variables = await Provider.list_variables(self._provider_id, client=self._self_registration_client)
-        old_variables = self._all_configured_variables.copy()
+        old_variables = dict(self._all_configured_variables)
 
-        for variable in list(self._all_configured_variables - variables.keys()):  # reset removed variables
+        for variable in list(self._all_configured_variables.keys() - variables.keys()):  # reset removed variables
             os.environ.pop(variable, None)
-            self._all_configured_variables.remove(variable)
+            del self._all_configured_variables[variable]
 
         os.environ.update(variables)
-        self._all_configured_variables.update(variables.keys())
+        self._all_configured_variables.update(variables)
 
         if dirty := old_variables != self._all_configured_variables:
             logger.info(f"Environment variables reloaded dynamically: {self._all_configured_variables}")

apps/agentstack-sdk-py/tests/unit/test_load_variables.py

@@ -0,0 +1,96 @@
+# Copyright 2025 © BeeAI a Series of LF Projects, LLC
+# SPDX-License-Identifier: Apache-2.0
+
+from __future__ import annotations
+
+import os
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+from agentstack_sdk.server.server import Server
+
+pytestmark = pytest.mark.unit
+
+
+def _make_server_with_agent() -> Server:
+    """Create a Server instance with minimal mocks for _load_variables testing."""
+    server = Server()
+    server._provider_id = "test-provider-id"
+
+    # Mock the uvicorn server
+    mock_uvicorn = MagicMock()
+    mock_uvicorn.config = MagicMock()
+    server.server = mock_uvicorn
+
+    # Mock a minimal agent with no extensions
+    mock_agent = MagicMock()
+    mock_agent.card.capabilities.extensions = []
+    server._agent = mock_agent
+
+    return server
+
+
+@patch("agentstack_sdk.server.server.Provider")
+async def test_load_variables_detects_value_changes(mock_provider_cls):
+    """Bug 1: _load_variables dirty check must detect when env var VALUES change,
+    even if the set of keys stays the same."""
+    server = _make_server_with_agent()
+
+    # First load: FOO=bar
+    mock_provider_cls.list_variables = AsyncMock(return_value={"FOO": "bar"})
+    await server._load_variables(first_run=True)
+    assert os.environ.get("FOO") == "bar"
+
+    # Second load: FOO=baz (value changed, key set unchanged)
+    mock_provider_cls.list_variables = AsyncMock(return_value={"FOO": "baz"})
+    await server._load_variables(first_run=False)
+
+    # The new value must be in os.environ
+    assert os.environ.get("FOO") == "baz"
+
+    # The internal tracker must reflect the new value so dirty=True was detected
+    assert server._all_configured_variables.get("FOO") == "baz"
+
+    # Cleanup
+    os.environ.pop("FOO", None)
+
+
+@patch("agentstack_sdk.server.server.Provider")
+async def test_load_variables_detects_key_removal(mock_provider_cls):
+    """Existing behaviour: removed keys should be cleaned from os.environ."""
+    server = _make_server_with_agent()
+
+    mock_provider_cls.list_variables = AsyncMock(return_value={"A": "1", "B": "2"})
+    await server._load_variables(first_run=True)
+    assert os.environ.get("A") == "1"
+    assert os.environ.get("B") == "2"
+
+    # Remove key B
+    mock_provider_cls.list_variables = AsyncMock(return_value={"A": "1"})
+    await server._load_variables(first_run=False)
+    assert os.environ.get("A") == "1"
+    assert os.environ.get("B") is None
+    assert "B" not in server._all_configured_variables
+
+    # Cleanup
+    os.environ.pop("A", None)
+
+
+@patch("agentstack_sdk.server.server.Provider")
+async def test_load_variables_no_change_is_not_dirty(mock_provider_cls):
+    """When nothing changed, the dirty flag should be False."""
+    server = _make_server_with_agent()
+
+    mock_provider_cls.list_variables = AsyncMock(return_value={"X": "1"})
+    await server._load_variables(first_run=True)
+
+    # Same values again
+    mock_provider_cls.list_variables = AsyncMock(return_value={"X": "1"})
+    await server._load_variables(first_run=False)
+
+    # Internal state unchanged
+    assert server._all_configured_variables == {"X": "1"}
+
+    # Cleanup
+    os.environ.pop("X", None)

apps/agentstack-server/src/agentstack_server/service_layer/services/model_providers.py

@@ -162,10 +162,10 @@ async def patch_provider(
         updated_provider.description = description if description is not None else provider.description
         updated_provider.type = type or updated_provider.type
         updated_provider.base_url = base_url or updated_provider.base_url
-        updated_provider.watsonx_project_id = watsonx_project_id or updated_provider.watsonx_project_id
-        updated_provider.watsonx_space_id = watsonx_space_id or updated_provider.watsonx_space_id
+        updated_provider.watsonx_project_id = watsonx_project_id if watsonx_project_id is not None else updated_provider.watsonx_project_id
+        updated_provider.watsonx_space_id = watsonx_space_id if watsonx_space_id is not None else updated_provider.watsonx_space_id
 
-        updated_api_key = api_key or old_api_key
+        updated_api_key = api_key if api_key is not None else old_api_key
 
         should_update = provider != updated_provider or (updated_api_key != old_api_key)
 

apps/agentstack-server/src/agentstack_server/service_layer/services/providers.py

@@ -377,7 +377,7 @@ async def update_provider_env(
             await self._rotate_provider(provider=provider, env=new_env)
         except Exception as ex:
             if not provider:
-                return
+                raise
             logger.error(f"Exception occurred while updating env, rolling back to previous state: {ex}")
             async with self._uow() as uow:
                 orig_env = await uow.env.get_all(parent_entity=EnvStoreEntity.PROVIDER, parent_entity_ids=[provider_id])

apps/agentstack-server/tests/unit/service_layer/services/test_model_provider_patch.py

@@ -0,0 +1,123 @@
+# Copyright 2025 © BeeAI a Series of LF Projects, LLC
+# SPDX-License-Identifier: Apache-2.0
+
+"""
+Test for Bug 3: patch_provider in ModelProviderService must use `is not None`
+instead of `or` for optional fields, so that:
+  - watsonx_project_id can be changed from one value to another
+  - watsonx_space_id can be changed from one value to another
+  - api_key can be changed even when old key is truthy
+"""
+
+from __future__ import annotations
+
+import pytest
+
+pytestmark = pytest.mark.unit
+
+
+class TestPatchProviderNullCoalescing:
+    """Test that patch_provider correctly applies `is not None` logic
+    for optional fields rather than `or`.
+
+    This tests the in-memory logic only, without requiring database or
+    external dependencies. We mirror the pattern used in the actual code.
+    """
+
+    @staticmethod
+    def _apply_patch_logic(
+        *,
+        old_watsonx_project_id: str | None,
+        old_watsonx_space_id: str | None,
+        old_api_key: str,
+        new_watsonx_project_id: str | None = None,
+        new_watsonx_space_id: str | None = None,
+        new_api_key: str | None = None,
+    ) -> tuple[str | None, str | None, str]:
+        """Replicate the fixed patch logic from model_providers.py."""
+        updated_watsonx_project_id = (
+            new_watsonx_project_id if new_watsonx_project_id is not None else old_watsonx_project_id
+        )
+        updated_watsonx_space_id = new_watsonx_space_id if new_watsonx_space_id is not None else old_watsonx_space_id
+        updated_api_key = new_api_key if new_api_key is not None else old_api_key
+        return updated_watsonx_project_id, updated_watsonx_space_id, updated_api_key
+
+    @staticmethod
+    def _apply_old_buggy_logic(
+        *,
+        old_watsonx_project_id: str | None,
+        old_watsonx_space_id: str | None,
+        old_api_key: str,
+        new_watsonx_project_id: str | None = None,
+        new_watsonx_space_id: str | None = None,
+        new_api_key: str | None = None,
+    ) -> tuple[str | None, str | None, str]:
+        """Replicate the OLD buggy logic using `or`."""
+        updated_watsonx_project_id = new_watsonx_project_id or old_watsonx_project_id
+        updated_watsonx_space_id = new_watsonx_space_id or old_watsonx_space_id
+        updated_api_key = new_api_key or old_api_key
+        return updated_watsonx_project_id, updated_watsonx_space_id, updated_api_key
+
+    def test_change_watsonx_project_id(self):
+        """Changing watsonx_project_id from 'old-project' to 'new-project' should succeed."""
+        proj, space, key = self._apply_patch_logic(
+            old_watsonx_project_id="old-project",
+            old_watsonx_space_id="space",
+            old_api_key="key",
+            new_watsonx_project_id="new-project",
+        )
+        assert proj == "new-project"  # Fixed: this must work
+        assert space == "space"  # Unchanged
+        assert key == "key"  # Unchanged
+
+    def test_none_does_not_change_value(self):
+        """Passing None should keep the original value (no change intended)."""
+        proj, space, key = self._apply_patch_logic(
+            old_watsonx_project_id="original-project",
+            old_watsonx_space_id="original-space",
+            old_api_key="original-key",
+            # All None = no changes
+        )
+        assert proj == "original-project"
+        assert space == "original-space"
+        assert key == "original-key"
+
+    def test_empty_string_clears_value(self):
+        """An empty string should be applied as the new value (unlike `or` which would keep the old one)."""
+        proj, space, key = self._apply_patch_logic(
+            old_watsonx_project_id="old-project",
+            old_watsonx_space_id="old-space",
+            old_api_key="old-key",
+            new_watsonx_project_id="",
+            new_watsonx_space_id="",
+            new_api_key="",
+        )
+        # With `is not None`, empty string IS the new value
+        assert proj == ""
+        assert space == ""
+        assert key == ""
+
+    def test_buggy_or_logic_fails_on_empty_string(self):
+        """Demonstrate the OLD bug: `or` ignores empty strings and keeps old values."""
+        proj, space, key = self._apply_old_buggy_logic(
+            old_watsonx_project_id="old-project",
+            old_watsonx_space_id="old-space",
+            old_api_key="old-key",
+            new_watsonx_project_id="",
+            new_watsonx_space_id="",
+            new_api_key="",
+        )
+        # With `or`, empty string is falsy -> old value is kept (this IS the bug!)
+        assert proj == "old-project"  # Bug: empty string was ignored
+        assert space == "old-space"  # Bug: empty string was ignored
+        assert key == "old-key"  # Bug: empty string was ignored
+
+    def test_change_api_key(self):
+        """Changing API key from one truthy value to another should work."""
+        _, _, key = self._apply_patch_logic(
+            old_watsonx_project_id=None,
+            old_watsonx_space_id=None,
+            old_api_key="old-key-abc",
+            new_api_key="new-key-xyz",
+        )
+        assert key == "new-key-xyz"

apps/agentstack-server/tests/unit/service_layer/services/test_update_provider_env.py

@@ -0,0 +1,115 @@
+# Copyright 2025 © BeeAI a Series of LF Projects, LLC
+# SPDX-License-Identifier: Apache-2.0
+
+from __future__ import annotations
+
+from contextlib import asynccontextmanager
+from unittest.mock import AsyncMock, MagicMock
+from uuid import uuid4
+
+import pytest
+
+from agentstack_server.domain.models.user import User, UserRole
+from agentstack_server.exceptions import EntityNotFoundError
+
+pytestmark = pytest.mark.unit
+
+
+def _make_mock_uow_factory(*, get_side_effect=None, provider=None):
+    """Create a mock UnitOfWork factory.
+
+    Args:
+        get_side_effect: Exception to raise when uow.providers.get() is called.
+        provider: Provider object to return from uow.providers.get() (ignored if get_side_effect is set).
+    """
+    mock_uow = AsyncMock()
+
+    if get_side_effect:
+        mock_uow.providers.get = AsyncMock(side_effect=get_side_effect)
+    else:
+        mock_uow.providers.get = AsyncMock(return_value=provider)
+
+    mock_uow.env.update = AsyncMock()
+    mock_uow.env.get_all = AsyncMock(return_value={})
+    mock_uow.commit = AsyncMock()
+
+    @asynccontextmanager
+    async def uow_context():
+        yield mock_uow
+
+    return uow_context
+
+
+def _make_admin_user():
+    return User(id=uuid4(), role=UserRole.ADMIN, email="admin@test.com")
+
+
+def _make_regular_user():
+    return User(id=uuid4(), role=UserRole.USER, email="user@test.com")
+
+
+async def test_update_provider_env_propagates_entity_not_found():
+    """Bug 2: update_provider_env must propagate EntityNotFoundError when provider_id does not exist,
+    instead of silently returning None."""
+    from agentstack_server.service_layer.services.providers import ProviderService
+
+    provider_id = uuid4()
+    user = _make_regular_user()
+
+    uow_factory = _make_mock_uow_factory(
+        get_side_effect=EntityNotFoundError("provider", id=str(provider_id))
+    )
+    deployment_manager = AsyncMock()
+
+    service = ProviderService.__new__(ProviderService)
+    service._uow = uow_factory  # type: ignore[assignment]
+    service._deployment_manager = deployment_manager
+
+    with pytest.raises(EntityNotFoundError):
+        await service.update_provider_env(
+            provider_id=provider_id,
+            env={"KEY": "value"},
+            user=user,
+        )
+
+
+async def test_update_provider_env_propagates_value_error_for_registry():
+    """Bug 2: When provider.registry is set and allow_registry_update=False,
+    the ValueError should propagate, not be swallowed."""
+    from agentstack_server.service_layer.services.providers import ProviderService
+
+    provider_id = uuid4()
+    user = _make_regular_user()
+
+    mock_provider = MagicMock()
+    mock_provider.registry = MagicMock()  # truthy = has registry
+    mock_provider.id = provider_id
+
+    uow_factory = _make_mock_uow_factory(provider=mock_provider)
+    # Override env.get_all to return correct structure for rollback path
+    # (the except block accesses result[provider_id])
+    original_factory = uow_factory
+
+    @asynccontextmanager
+    async def patched_uow_context():
+        async with original_factory() as uow:
+            uow.env.get_all = AsyncMock(return_value={provider_id: {}})
+            yield uow
+
+    uow_factory = patched_uow_context
+    deployment_manager = AsyncMock()
+
+    service = ProviderService.__new__(ProviderService)
+    service._uow = uow_factory  # type: ignore[assignment]
+    service._deployment_manager = deployment_manager
+
+    # The ValueError from "Cannot update variables for a provider added from registry"
+    # should propagate. Before the fix, provider is truthy so it enters the rollback path
+    # which is correct for this case. But EntityNotFoundError (above test) was the silent one.
+    with pytest.raises(ValueError, match="Cannot update variables for a provider added from registry"):
+        await service.update_provider_env(
+            provider_id=provider_id,
+            env={"KEY": "value"},
+            user=user,
+            allow_registry_update=False,
+        )