diff --git a/apps/agentstack-sdk-py/src/agentstack_sdk/server/server.py b/apps/agentstack-sdk-py/src/agentstack_sdk/server/server.py index 736553e974..e1291ebc33 100644 --- a/apps/agentstack-sdk-py/src/agentstack_sdk/server/server.py +++ b/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}") diff --git a/apps/agentstack-sdk-py/tests/unit/test_load_variables.py b/apps/agentstack-sdk-py/tests/unit/test_load_variables.py new file mode 100644 index 0000000000..1701211fe0 --- /dev/null +++ b/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) diff --git a/apps/agentstack-server/src/agentstack_server/service_layer/services/model_providers.py b/apps/agentstack-server/src/agentstack_server/service_layer/services/model_providers.py index 254e6bfc08..86058fd6f1 100644 --- a/apps/agentstack-server/src/agentstack_server/service_layer/services/model_providers.py +++ b/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) diff --git a/apps/agentstack-server/src/agentstack_server/service_layer/services/providers.py b/apps/agentstack-server/src/agentstack_server/service_layer/services/providers.py index 0fecedde2b..b1f09de97f 100644 --- a/apps/agentstack-server/src/agentstack_server/service_layer/services/providers.py +++ b/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]) diff --git a/apps/agentstack-server/tests/unit/service_layer/services/test_model_provider_patch.py b/apps/agentstack-server/tests/unit/service_layer/services/test_model_provider_patch.py new file mode 100644 index 0000000000..b1915423a9 --- /dev/null +++ b/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" diff --git a/apps/agentstack-server/tests/unit/service_layer/services/test_update_provider_env.py b/apps/agentstack-server/tests/unit/service_layer/services/test_update_provider_env.py new file mode 100644 index 0000000000..f7c948af90 --- /dev/null +++ b/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, + )