diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 61cd052f99b..e884d74b6f1 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -45,13 +45,18 @@ agent_framework/ display a mask. String-only APIs such as `str.join()` and JSON encoding reject it unless callers explicitly convert it. Use `get_secret_value()` when passing credentials to provider SDKs; `str(secret)` returns the mask. - **`load_settings`** accepts plain string overrides for `SecretString` fields and wraps them, as it does for - environment and `.env` values. Existing `SecretString` overrides are preserved. + environment and `.env` values. Existing `SecretString` overrides are preserved. Invalid supplied numeric and + boolean environment or `.env` values raise `ValueError` identifying the field and source instead of falling back + to raw strings. ### Agents (`_agents.py`) - **`SupportsAgentRun`** - Protocol defining the agent interface - **`BaseAgent`** - Abstract base class for agents - **`Agent`** - Main agent class wrapping a chat client with tools, instructions, and middleware +- **`RawAgent.open()` / `close()`** (inherited by `Agent`) - Explicitly enter the client's and configured MCP tools' + async contexts, then release them along with lazily connected MCP tools. `async with agent` delegates to these + methods; a partially failed `open()` closes resources already entered. ### Chat Clients (`_clients.py`) diff --git a/python/packages/core/agent_framework/_agents.py b/python/packages/core/agent_framework/_agents.py index 379601fa256..11e317dcb80 100644 --- a/python/packages/core/agent_framework/_agents.py +++ b/python/packages/core/agent_framework/_agents.py @@ -892,8 +892,7 @@ def get_weather(location: str) -> str: name="weather-agent", instructions="You are a weather assistant.", tools=get_weather, - temperature=0.7, - max_tokens=500, + default_options={"temperature": 0.7, "max_tokens": 500}, ) # Use streaming responses @@ -912,20 +911,30 @@ def get_weather(location: str) -> str: client = OpenAIChatClient(model="gpt-4o") agent: Agent[OpenAIChatOptions] = Agent( client=client, - name="reasoning-agent", - instructions="You are a reasoning assistant.", - options={ + name="typed-agent", + instructions="You are a helpful assistant.", + default_options={ "temperature": 0.7, "max_tokens": 500, - "reasoning_effort": "high", # OpenAI-specific, IDE will autocomplete! + "include": ["message.output_text.logprobs"], # OpenAI-specific option }, ) - # Or pass options at runtime + # Or override default options at runtime response = await agent.run( "What is 25 * 47?", - options={"temperature": 0.0, "logprobs": True}, + options={"temperature": 0.0}, ) + + Explicit resource management (equivalent to ``async with agent``): + + .. code-block:: python + + await agent.open() + try: + response = await agent.run("Hello") + finally: + await agent.close() """ AGENT_PROVIDER_NAME: ClassVar[str] = "microsoft.agent_framework" @@ -1049,23 +1058,29 @@ def __init__( self._async_exit_stack = AsyncExitStack() self._update_agent_name_and_description() - async def __aenter__(self) -> Self: - """Enter the async context manager. + async def open(self) -> Self: + """Open the client's and configured MCP tools' async contexts. - If any of the client or local_mcp_tools are context managers, - they will be entered into the async exit stack to ensure proper cleanup. - - Note: - This list might be extended in the future. + Call ``await agent.close()`` when finished, or use ``async with agent`` + to open and close automatically. Already-entered contexts are closed if + a later context fails to open. Returns: - The Agent instance. + The agent instance. """ - for context_manager in chain([self.client], self.mcp_tools): - if isinstance(context_manager, AbstractAsyncContextManager): - await self._async_exit_stack.enter_async_context(context_manager) + try: + for context_manager in chain([self.client], self.mcp_tools): + if isinstance(context_manager, AbstractAsyncContextManager): + await self._async_exit_stack.enter_async_context(context_manager) + except BaseException: + await self.close() + raise return self + async def __aenter__(self) -> Self: + """Enter the async context manager by opening managed resources.""" + return await self.open() + def _get_history_providers(self) -> list[HistoryProvider]: return [provider for provider in self.context_providers if isinstance(provider, HistoryProvider)] @@ -1095,22 +1110,18 @@ def _resolve_per_service_call_history_providers( ) return history_providers + async def close(self) -> None: + """Close managed client and MCP contexts, including lazily opened MCP tools.""" + await self._async_exit_stack.aclose() + async def __aexit__( self, exc_type: type[BaseException] | None, exc_val: BaseException | None, exc_tb: Any, ) -> None: - """Exit the async context manager. - - Close the async exit stack to ensure all context managers are exited properly. - - Args: - exc_type: The exception type if an exception was raised, None otherwise. - exc_val: The exception value if an exception was raised, None otherwise. - exc_tb: The exception traceback if an exception was raised, None otherwise. - """ - await self._async_exit_stack.aclose() + """Exit the async context manager by closing managed resources.""" + await self.close() def _update_agent_name_and_description(self) -> None: """Update the agent name in the chat client. @@ -1944,6 +1955,9 @@ class Agent( - OpenTelemetry-based telemetry for observability For a minimal implementation without these features, use :class:`RawAgent`. + + Use ``await agent.open()`` and ``await agent.close()`` for explicit resource + management, or ``async with agent`` to close managed resources automatically. """ @overload diff --git a/python/packages/core/agent_framework/_settings.py b/python/packages/core/agent_framework/_settings.py index 1629eb5ff96..d674c47f99e 100644 --- a/python/packages/core/agent_framework/_settings.py +++ b/python/packages/core/agent_framework/_settings.py @@ -34,7 +34,7 @@ class MySettings(TypedDict, total=False): import sys from collections.abc import Callable, Sequence from contextlib import suppress -from typing import Any, Union, get_args, get_origin, get_type_hints +from typing import Any, Literal, Union, cast, get_args, get_origin, get_type_hints from dotenv import dotenv_values @@ -156,9 +156,25 @@ def _coerce_value(value: str, target_type: type) -> Any: if args and type(None) in args: for arg in args: if arg is not type(None): + if arg is Any: + return value + runtime_type = _runtime_class(arg) + if runtime_type is None and get_origin(arg) is not Literal: + continue with suppress(ValueError, TypeError): - return _coerce_value(value, arg) - return value + coerced = _coerce_value(value, arg) + if runtime_type is None or isinstance(coerced, runtime_type): + return coerced + raise ValueError("Value cannot be converted to any allowed type.") + + if origin is Literal: + for choice in args: + choice_type = cast(type[Any], type(choice)) + with suppress(ValueError, TypeError): + coerced = _coerce_value(value, choice_type) + if type(coerced) is choice_type and coerced == choice: + return coerced + raise ValueError("Value is not an allowed literal.") # Handle SecretString if target_type is SecretString or (isinstance(target_type, type) and issubclass(target_type, SecretString)): @@ -172,11 +188,25 @@ def _coerce_value(value: str, target_type: type) -> Any: if target_type is float: return float(value) if target_type is bool: - return value.lower() in ("true", "1", "yes", "on") + normalized = value.strip().lower() + if normalized in ("true", "1", "yes", "on"): + return True + if normalized in ("false", "0", "no", "off"): + return False + raise ValueError("Value is not a recognized boolean.") return value +def _coerce_from_source(value: str, target_type: type, *, field_name: str, source: str) -> Any: + """Coerce a supplied value or identify the setting and source on failure.""" + try: + return _coerce_value(value, target_type) + except (ValueError, TypeError): + pass + raise ValueError(f"Invalid value for setting '{field_name}' from {source}.") + + def _runtime_class(annotation: Any) -> type | None: """Return the class ``isinstance`` can test *annotation* against, or ``None``. @@ -185,6 +215,8 @@ def _runtime_class(annotation: Any) -> type | None: is always preferred. Annotations without a runtime class, such as ``Literal[...]``, return ``None`` so callers can skip validation instead of guessing. """ + if annotation is Any: + return None origin = get_origin(annotation) candidate = annotation if origin is None else origin return candidate if isinstance(candidate, type) else None @@ -245,7 +277,8 @@ def _check_override_type(value: Any, field_type: type, field_name: str) -> None: allowed_names = ", ".join(t.__name__ for t in allowed) raise ValueError( - f"Invalid type for setting '{field_name}': expected {allowed_names}, got {type(value).__name__}." + f"Invalid type for setting '{field_name}' from explicit override: " + f"expected {allowed_names}, got {type(value).__name__}." ) @@ -293,7 +326,8 @@ def load_settings( FileNotFoundError: If *env_file_path* was provided but the file does not exist. SettingNotFoundError: If a required field could not be resolved from any source, or if a mutually exclusive constraint is violated. - ValueError: If an override value has an incompatible type. + ValueError: If an override value has an incompatible type or an environment + or ``.env`` value cannot be coerced to the setting's type. """ encoding = env_file_encoding or "utf-8" @@ -332,19 +366,17 @@ def load_settings( if loaded_dotenv_values: dotenv_value = loaded_dotenv_values.get(env_var_name) if dotenv_value is not None: - try: - result[field_name] = _coerce_value(dotenv_value, field_type) - except (ValueError, TypeError): - result[field_name] = dotenv_value + result[field_name] = _coerce_from_source( + dotenv_value, field_type, field_name=field_name, source=f".env file '{env_file_path}'" + ) continue # 3. Environment variable env_value = os.getenv(env_var_name) if env_value is not None: - try: - result[field_name] = _coerce_value(env_value, field_type) - except (ValueError, TypeError): - result[field_name] = env_value + result[field_name] = _coerce_from_source( + env_value, field_type, field_name=field_name, source=f"environment variable '{env_var_name}'" + ) continue # 4. Default from TypedDict class-level defaults, or None for optional fields diff --git a/python/packages/core/tests/core/test_agents.py b/python/packages/core/tests/core/test_agents.py index c3e132656aa..e0e79153288 100644 --- a/python/packages/core/tests/core/test_agents.py +++ b/python/packages/core/tests/core/test_agents.py @@ -35,6 +35,7 @@ InMemoryStore, Message, MessageInjectionMiddleware, + RawAgent, ResponseStream, ServiceSessionId, SessionContext, @@ -93,6 +94,46 @@ def get_mcp_client(self) -> contextlib.AbstractAsyncContextManager[Any]: # type raise NotImplementedError +class _LifecycleClient(MockBaseChatClient): + def __init__(self, events: list[str]) -> None: + super().__init__() + self.events = events + self.exit_exception_type: type[BaseException] | None = None + + async def __aenter__(self) -> "_LifecycleClient": + self.events.append("client.enter") + return self + + async def __aexit__(self, exc_type: type[BaseException] | None, exc_val: BaseException | None, exc_tb: Any) -> None: + self.exit_exception_type = exc_type + self.events.append("client.exit") + + +class _LifecycleMCPTool(_ConnectedMCPTool): + def __init__(self, name: str, events: list[str], *, fail_on_enter: bool = False) -> None: + super().__init__(name=name, function_names=[]) + self.events = events + self.fail_on_enter = fail_on_enter + self.exit_exception_type: type[BaseException] | None = None + + async def __aenter__(self) -> "_LifecycleMCPTool": + self.events.append(f"{self.name}.enter") + if self.fail_on_enter: + raise RuntimeError("MCP entry failed") + self.is_connected = True + return self + + async def __aexit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: Any, + ) -> None: + self.exit_exception_type = exc_type + self.events.append(f"{self.name}.exit") + self.is_connected = False + + class _RecordingHistoryProvider(HistoryProvider): def __init__(self, source_id: str = "recording_history") -> None: super().__init__(source_id=source_id) @@ -162,6 +203,85 @@ def test_agent_type(agent: SupportsAgentRun) -> None: assert isinstance(agent, SupportsAgentRun) +@pytest.mark.parametrize("agent_type", [Agent, RawAgent]) +async def test_agent_explicit_open_close(agent_type: type[RawAgent]) -> None: + events: list[str] = [] + client = _LifecycleClient(events) + mcp_tool = _LifecycleMCPTool("mcp", events) + agent = agent_type(client=client, tools=[mcp_tool]) + + assert await agent.open() is agent + assert events == ["client.enter", "mcp.enter"] + + await agent.close() + await agent.close() + + assert events == ["client.enter", "mcp.enter", "mcp.exit", "client.exit"] + + +@pytest.mark.parametrize("agent_type", [Agent, RawAgent]) +async def test_agent_async_with_delegates_to_open_close(agent_type: type[RawAgent]) -> None: + events: list[str] = [] + client = _LifecycleClient(events) + mcp_tool = _LifecycleMCPTool("mcp", events) + agent = agent_type(client=client, tools=[mcp_tool]) + + with ( + patch.object(agent, "open", wraps=agent.open) as open_mock, + patch.object(agent, "close", wraps=agent.close) as close_mock, + pytest.raises(RuntimeError, match="body failed"), + ): + async with agent as entered_agent: + assert entered_agent is agent + assert events == ["client.enter", "mcp.enter"] + raise RuntimeError("body failed") + + open_mock.assert_awaited_once_with() + close_mock.assert_awaited_once_with() + assert events == ["client.enter", "mcp.enter", "mcp.exit", "client.exit"] + assert client.exit_exception_type is None + assert mcp_tool.exit_exception_type is None + + +@pytest.mark.parametrize("agent_type", [Agent, RawAgent]) +async def test_agent_failed_open_closes_entered_contexts(agent_type: type[RawAgent]) -> None: + events: list[str] = [] + client = _LifecycleClient(events) + first_tool = _LifecycleMCPTool("first", events) + failing_tool = _LifecycleMCPTool("failing", events, fail_on_enter=True) + agent = agent_type(client=client, tools=[first_tool, failing_tool]) + + with pytest.raises(RuntimeError, match="MCP entry failed"): + await agent.open() + + assert events == ["client.enter", "first.enter", "failing.enter", "first.exit", "client.exit"] + assert first_tool.exit_exception_type is None + assert client.exit_exception_type is None + + events.clear() + failing_tool.fail_on_enter = False + assert await agent.open() is agent + await agent.close() + assert events == ["client.enter", "first.enter", "failing.enter", "failing.exit", "first.exit", "client.exit"] + + +async def test_agent_close_cleans_up_mcp_tool_opened_during_run(chat_client_base: MockBaseChatClient) -> None: + events: list[str] = [] + mcp_tool = _LifecycleMCPTool("lazy", events) + mcp_tool.is_connected = False + agent = Agent(client=chat_client_base, tools=[mcp_tool]) + + await agent.run("hello") + + assert events == ["lazy.enter"] + assert mcp_tool.is_connected + + await agent.close() + + assert events == ["lazy.enter", "lazy.exit"] + assert not mcp_tool.is_connected + + async def test_agent_run(agent: SupportsAgentRun) -> None: response = await agent.run("test") assert response.messages[0].role == "assistant" diff --git a/python/packages/core/tests/core/test_settings.py b/python/packages/core/tests/core/test_settings.py index 9070c31a6ed..af480ffe833 100644 --- a/python/packages/core/tests/core/test_settings.py +++ b/python/packages/core/tests/core/test_settings.py @@ -147,6 +147,48 @@ def test_overrides_beat_dotenv_and_env_vars(self, monkeypatch: pytest.MonkeyPatc finally: os.unlink(env_path) + def test_override_ignores_invalid_lower_priority_values( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + env_file = tmp_path / ".env" + env_file.write_text("TEST_APP_TIMEOUT=invalid\n", encoding="utf-8") + monkeypatch.setenv("TEST_APP_TIMEOUT", "also-invalid") + + settings = load_settings(SimpleSettings, env_prefix="TEST_APP_", env_file_path=str(env_file), timeout=60) + + assert settings["timeout"] == 60 + + @pytest.mark.parametrize( + ("field", "invalid", "environment_value"), + [ + ("timeout", "not-an-int", "30"), + ("rate_limit", "not-a-float", "2.5"), + ("enabled", "maybe", "true"), + ], + ) + def test_invalid_dotenv_value_raises_without_falling_back_to_environment( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, field: str, invalid: str, environment_value: str + ) -> None: + env_var = f"TEST_APP_{field.upper()}" + env_file = tmp_path / ".env" + env_file.write_text(f"{env_var}={invalid}\n", encoding="utf-8") + monkeypatch.setenv(env_var, environment_value) + + with pytest.raises(ValueError, match=rf"setting '{field}' from \.env file") as exc_info: + load_settings(SimpleSettings, env_prefix="TEST_APP_", env_file_path=str(env_file)) + + assert str(env_file) in str(exc_info.value) + assert invalid not in str(exc_info.value) + + def test_bool_and_float_from_dotenv(self, tmp_path: Path) -> None: + env_file = tmp_path / ".env" + env_file.write_text("TEST_APP_ENABLED=ON\nTEST_APP_RATE_LIMIT=2.5\n", encoding="utf-8") + + settings = load_settings(SimpleSettings, env_prefix="TEST_APP_", env_file_path=str(env_file)) + + assert settings["enabled"] is True + assert settings["rate_limit"] == 2.5 + def test_missing_dotenv_file_raises(self) -> None: with pytest.raises(FileNotFoundError): load_settings(SimpleSettings, env_prefix="TEST_APP_", env_file_path="/nonexistent/.env") @@ -365,6 +407,126 @@ def test_bool_coercion_false_values(self, monkeypatch: pytest.MonkeyPatch) -> No settings = load_settings(SimpleSettings, env_prefix="TEST_APP_") assert settings["enabled"] is False, f"Failed for {false_val}" + def test_bool_coercion_ignores_surrounding_whitespace(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("TEST_APP_ENABLED", " YeS ") + + assert load_settings(SimpleSettings, env_prefix="TEST_APP_")["enabled"] is True + + @pytest.mark.parametrize( + ("field", "invalid"), + [("timeout", "sk-invalid-number"), ("rate_limit", "not-a-float")], + ) + def test_invalid_optional_numeric_from_environment_raises( + self, monkeypatch: pytest.MonkeyPatch, field: str, invalid: str + ) -> None: + env_var = f"TEST_APP_{field.upper()}" + monkeypatch.setenv(env_var, invalid) + + with pytest.raises(ValueError, match=f"setting '{field}' from environment variable '{env_var}'") as exc_info: + load_settings(SimpleSettings, env_prefix="TEST_APP_") + + assert invalid not in str(exc_info.value) + assert exc_info.value.__cause__ is None + assert exc_info.value.__context__ is None + + @pytest.mark.parametrize("invalid", ["maybe", "2", ""]) + def test_invalid_bool_from_environment_raises(self, monkeypatch: pytest.MonkeyPatch, invalid: str) -> None: + monkeypatch.setenv("TEST_APP_ENABLED", invalid) + + with pytest.raises(ValueError, match="setting 'enabled' from environment variable 'TEST_APP_ENABLED'"): + load_settings(SimpleSettings, env_prefix="TEST_APP_") + + def test_unsupported_union_arm_does_not_accept_invalid_numeric_string( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + class MixedSettings(TypedDict, total=False): + value: int | dict[str, Any] | None + + monkeypatch.setenv("MIXED_VALUE", "invalid") + + with pytest.raises(ValueError, match="setting 'value' from environment variable 'MIXED_VALUE'"): + load_settings(MixedSettings, env_prefix="MIXED_") + + def test_string_arm_preserves_provider_specific_parsing(self, monkeypatch: pytest.MonkeyPatch) -> None: + class MixedSettings(TypedDict, total=False): + telemetry: dict[str, Any] | str | None + + monkeypatch.setenv("MIXED_TELEMETRY", '{"enabled": true}') + + settings = load_settings(MixedSettings, env_prefix="MIXED_") + + assert settings["telemetry"] == '{"enabled": true}' + + def test_optional_any_preserves_environment_value(self, monkeypatch: pytest.MonkeyPatch) -> None: + class AnySettings(TypedDict, total=False): + value: Any | None + + monkeypatch.setenv("TEST_VALUE", '{"enabled": true}') + + settings = load_settings(AnySettings, env_prefix="TEST_") + + assert settings["value"] == '{"enabled": true}' + + def test_optional_any_preserves_dotenv_value(self, tmp_path: Path) -> None: + class AnySettings(TypedDict, total=False): + value: Any | None + + env_file = tmp_path / ".env" + env_file.write_text("TEST_VALUE=provider-specific-value\n", encoding="utf-8") + + settings = load_settings(AnySettings, env_prefix="TEST_", env_file_path=str(env_file)) + + assert settings["value"] == "provider-specific-value" + + def test_literal_union_accepts_valid_and_rejects_invalid_environment_values( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + class LiteralSettings(TypedDict, total=False): + mode: Literal["fast", "safe"] | None + + monkeypatch.setenv("TEST_MODE", "fast") + assert load_settings(LiteralSettings, env_prefix="TEST_")["mode"] == "fast" + + monkeypatch.setenv("TEST_MODE", "unknown") + with pytest.raises(ValueError, match="setting 'mode' from environment variable 'TEST_MODE'"): + load_settings(LiteralSettings, env_prefix="TEST_") + + def test_non_string_literal_union_coerces_environment_values(self, monkeypatch: pytest.MonkeyPatch) -> None: + class LiteralSettings(TypedDict, total=False): + attempts: Literal[1, 2] | None + enabled: Literal[True, False] | None + + monkeypatch.setenv("TEST_ATTEMPTS", "2") + monkeypatch.setenv("TEST_ENABLED", "false") + + settings = load_settings(LiteralSettings, env_prefix="TEST_") + + assert settings["attempts"] == 2 + assert type(settings["attempts"]) is int + assert settings["enabled"] is False + assert type(settings["enabled"]) is bool + + def test_non_string_literal_union_coerces_dotenv_values(self, tmp_path: Path) -> None: + class LiteralSettings(TypedDict, total=False): + attempts: Literal[3, 4] | None + + env_file = tmp_path / ".env" + env_file.write_text("TEST_ATTEMPTS=4\n", encoding="utf-8") + + settings = load_settings(LiteralSettings, env_prefix="TEST_", env_file_path=str(env_file)) + + assert settings["attempts"] == 4 + assert type(settings["attempts"]) is int + + def test_non_string_literal_uses_strict_type_equality(self, monkeypatch: pytest.MonkeyPatch) -> None: + class LiteralSettings(TypedDict, total=False): + value: Literal[1] | None + + monkeypatch.setenv("TEST_VALUE", "true") + + with pytest.raises(ValueError, match="setting 'value' from environment variable 'TEST_VALUE'"): + load_settings(LiteralSettings, env_prefix="TEST_") + class TestRequiredFields: """Test required field validation.""" @@ -403,8 +565,7 @@ class TestOverrideTypeValidation: """Test override type validation.""" def test_invalid_type_raises(self) -> None: - - with pytest.raises(ValueError, match="Invalid type for setting 'api_key'"): + with pytest.raises(ValueError, match="Invalid type for setting 'api_key' from explicit override"): load_settings(SimpleSettings, env_prefix="TEST_", api_key={"bad": "type"}) def test_valid_types_accepted(self) -> None: @@ -475,6 +636,17 @@ class LiteralUnionSettings(TypedDict, total=False): assert settings["mode"] == ["a", "b"] + def test_callable_override_is_preserved(self) -> None: + class CallableSettings(TypedDict, total=False): + token_provider: Callable[[], str] | None + + def token_provider() -> str: + return "token" + + settings = load_settings(CallableSettings, token_provider=token_provider) + + assert settings["token_provider"] is token_provider + class TestMutuallyExclusive: """Test mutually exclusive field validation via tuple entries in required_fields."""