diff --git a/packages/claude-agents/src/launchdarkly_ai_claude_agents/handler.py b/packages/claude-agents/src/launchdarkly_ai_claude_agents/handler.py index 32ee9fd6..33b0dcd9 100644 --- a/packages/claude-agents/src/launchdarkly_ai_claude_agents/handler.py +++ b/packages/claude-agents/src/launchdarkly_ai_claude_agents/handler.py @@ -47,6 +47,8 @@ set_output_content_attributes, set_tool_call_content_attributes, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters from .spans import ( MCP_TOOL_PREFIX, @@ -66,6 +68,87 @@ tool_display_name, ) +#: Every field ``ClaudeAgentOptions`` declares, classified by hand into exactly one of: forwarded +#: (below), handler-owned (``model``, ``allowed_tools``, ``mcp_servers``, ``hooks``, ``tools``, +#: ``system_prompt``, set by each call site itself), or excluded (below). +#: ``TestClaudeAgentOptionsAcceptsExactlyTheseFields`` in this package's tests asserts this +#: classification stays exhaustive as the SDK's own dataclass changes. +#: +#: Only model and run settings are forwarded: how the model thinks, how long the run may go, and +#: what it may spend. Everything that configures the host process the SDK launches (its binary, +#: environment, working directory, file access, permissions, settings files, plugins, sandbox, +#: session state) stays under the application's control, never a config's. +#: +#: The SDK offers no ``temperature``/``top_p``/``top_k``/``max_tokens``/``stop_sequences``/ +#: ``tool_choice``/``metadata``, all of which the LaunchDarkly UI's model parameters panel offers +#: for other providers; they are dropped like any other key not listed here. +_CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS = frozenset( + { + "betas", + "effort", + "fallback_model", + "max_budget_usd", + "max_thinking_tokens", + "max_turns", + "output_format", + "thinking", + } +) + +#: Accepted by ``ClaudeAgentOptions`` but never forwarded, and why: +#: * ``cli_path``, ``env``, ``cwd``, ``add_dirs``, ``settings``, ``setting_sources``, ``plugins``, +#: ``skills``, ``sandbox``, ``user``, ``extra_args``: which binary runs, with what environment, +#: as which user, with what files, settings, plugins, and CLI arguments. A config that could set +#: these could run code on, or read files from, the host. +#: * ``permission_mode``, ``permission_prompt_tool_name``, ``can_use_tool``, ``disallowed_tools``, +#: ``strict_mcp_config``, ``agents``: what the agent is allowed to do and which tools or +#: subagents it gets. The handler wires tools and permissions itself. +#: * ``resume``, ``session_id``, ``fork_session``, ``continue_conversation``, ``session_store``, +#: ``session_store_flush``, ``enable_file_checkpointing``: session state on the host. +#: * ``stderr``, ``debug_stderr``, ``include_partial_messages``, ``include_hook_events``, +#: ``max_buffer_size``, ``load_timeout_ms``: process I/O and transport plumbing, including the +#: streamed message shape the handler reads. +#: * ``task_budget``: not one of the agreed run settings yet; ``max_turns`` and ``max_budget_usd`` +#: cover run limits. +#: +#: Named for the drift test and for review, not read at runtime: the forwarded list above already +#: leaves these out, so nothing needs to subtract them again. +_CLAUDE_AGENT_OPTIONS_EXCLUDED_KEYS = frozenset( + { + "add_dirs", + "agents", + "can_use_tool", + "cli_path", + "continue_conversation", + "cwd", + "debug_stderr", + "disallowed_tools", + "enable_file_checkpointing", + "env", + "extra_args", + "fork_session", + "include_hook_events", + "include_partial_messages", + "load_timeout_ms", + "max_buffer_size", + "permission_mode", + "permission_prompt_tool_name", + "plugins", + "resume", + "sandbox", + "session_id", + "session_store", + "session_store_flush", + "setting_sources", + "settings", + "skills", + "stderr", + "strict_mcp_config", + "task_budget", + "user", + } +) + # --------------------------------------------------------------------------- # Tool wiring # --------------------------------------------------------------------------- @@ -479,7 +562,11 @@ def _build_query_options( **extra: Any, ) -> ClaudeAgentOptions: all_allowed = [*mcp_allowed_tools, *native_tool_names] + params = select_forwarded_parameters( + model_parameters(config), _CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS + ) kwargs: dict[str, Any] = { + **params, "model": config["model"]["name"], "allowed_tools": all_allowed if all_allowed else [], "mcp_servers": {TOOL_MCP_NAME: tool_mcp} if tool_mcp else {}, diff --git a/packages/claude-agents/src/launchdarkly_ai_claude_agents/native_graph.py b/packages/claude-agents/src/launchdarkly_ai_claude_agents/native_graph.py index b0ffadb9..7a14733a 100644 --- a/packages/claude-agents/src/launchdarkly_ai_claude_agents/native_graph.py +++ b/packages/claude-agents/src/launchdarkly_ai_claude_agents/native_graph.py @@ -20,6 +20,8 @@ make_track_data, to_ld_context, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters try: from opentelemetry import trace @@ -30,6 +32,7 @@ _HAS_OTEL = False from launchdarkly_ai_claude_agents.handler import ( + _CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS, _build_hooks, build_prompt, build_query_prompt, @@ -149,7 +152,12 @@ async def _run_query( hooks = _build_hooks(native_tool_map) + params = select_forwarded_parameters( + model_parameters(node.config), _CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS + ) + options = ClaudeAgentOptions( + **params, # Explicitly set the available built-in tools (empty list disables all). # When no native tools are needed, disable built-in tools so Claude # cannot call WebSearch/Bash/etc. and get stuck waiting for permission diff --git a/packages/claude-agents/tests/test_handler.py b/packages/claude-agents/tests/test_handler.py index 5651391e..970565fc 100644 --- a/packages/claude-agents/tests/test_handler.py +++ b/packages/claude-agents/tests/test_handler.py @@ -43,6 +43,7 @@ partition_tools, ) from launchdarkly_ai_server import ConversationIdSpanProcessor, conversation_id +from tests.never_forwarded import NEVER_FORWARDED_BAG, find_leaks # --------------------------------------------------------------------------- # A real tracer provider, reset between tests @@ -1442,6 +1443,125 @@ async def test_ld_span_attributes_land_on_root_only( assert [e.name for e in root().events] == ["feature_flag"] +class TestModelParametersForwarding: + async def _run_and_capture_options( + self, config: dict[str, Any], monkeypatch: pytest.MonkeyPatch + ) -> Any: + captured: dict[str, Any] = {} + + async def _query(**kwargs: Any) -> AsyncIterator[Any]: + captured["options"] = kwargs["options"] + yield assistant_message() + yield result_message() + + monkeypatch.setattr(handler_mod, "query", _query) + await create_claude_agents_handler()(config, "q") + return captured["options"] + + async def test_max_turns_from_config_reaches_options( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + config = { + **BASE_CONFIG, + "model": {**BASE_CONFIG["model"], "parameters": {"max_turns": 3}}, + } + options = await self._run_and_capture_options(config, monkeypatch) + assert options.max_turns == 3 + + async def test_config_cannot_override_model_or_system_prompt( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + config = { + **BASE_CONFIG, + "model": { + **BASE_CONFIG["model"], + "parameters": { + "model": "not-the-real-model", + "system_prompt": "not-the-real-prompt", + }, + }, + } + options = await self._run_and_capture_options(config, monkeypatch) + assert options.model == BASE_CONFIG["model"]["name"] + assert options.system_prompt != "not-the-real-prompt" + + async def test_unset_when_no_parameters( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + options = await self._run_and_capture_options(BASE_CONFIG, monkeypatch) + assert options.max_turns is None + + async def test_ui_keys_the_sdk_rejects_are_dropped_without_raising( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """``ClaudeAgentOptions`` has no ``temperature``/``top_p``/``top_k``/``max_tokens``/ + ``stop_sequences``/``tool_choice``/``metadata`` fields, all of which the LaunchDarkly UI's + model parameters panel offers for other providers. Forwarding one unfiltered raises + ``TypeError`` before any request is made; the filter must drop them instead. + """ + config = { + **BASE_CONFIG, + "model": { + **BASE_CONFIG["model"], + "parameters": { + "temperature": 0.2, + "top_p": 0.5, + "top_k": 10, + "max_tokens": 256, + "stop_sequences": ["STOP"], + "tool_choice": "auto", + "metadata": {"user_id": "u1"}, + "max_turns": 3, + }, + }, + } + options = await self._run_and_capture_options(config, monkeypatch) + assert options.max_turns == 3 + for rejected in ( + "temperature", + "top_p", + "top_k", + "max_tokens", + "stop_sequences", + "tool_choice", + "metadata", + ): + assert not hasattr(options, rejected) or getattr(options, rejected) is None + + async def test_no_never_forwarded_key_reaches_the_query_options( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Every credential, endpoint, request-injection, remote-tool, and host-process key, + including the real ``ClaudeAgentOptions`` fields ``cli_path``, ``env``, ``cwd``, + ``add_dirs``, ``permission_mode`` and ``can_use_tool``, is dropped on the way to + ``query``; the agreed run setting still lands.""" + config = { + **BASE_CONFIG, + "model": { + **BASE_CONFIG["model"], + "parameters": {**NEVER_FORWARDED_BAG, "max_turns": 2}, + }, + } + options = await self._run_and_capture_options(config, monkeypatch) + assert options.max_turns == 2 + assert not find_leaks(options) + + async def test_temperature_top_p_and_max_turns_run_without_type_error( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A realistic combination of UI-offered keys must not raise, and the one real field + (``max_turns``) must still land.""" + config = { + **BASE_CONFIG, + "model": { + **BASE_CONFIG["model"], + "parameters": {"temperature": 0.3, "top_p": 0.8, "max_turns": 5}, + }, + } + options = await self._run_and_capture_options(config, monkeypatch) + assert options.max_turns == 5 + + class TestFinishReasonMapping: async def test_tool_use_maps_to_tool_calls( self, monkeypatch: pytest.MonkeyPatch diff --git a/packages/claude-agents/tests/test_native_graph.py b/packages/claude-agents/tests/test_native_graph.py index e01900f4..aa243a18 100644 --- a/packages/claude-agents/tests/test_native_graph.py +++ b/packages/claude-agents/tests/test_native_graph.py @@ -14,6 +14,7 @@ import launchdarkly_ai_claude_agents.native_graph as _claude_ng from launchdarkly_ai_claude_agents.native_graph import to_claude_agents from launchdarkly_ai_server import GraphDefinition, GraphEdge, GraphNode, NativeTool +from tests.never_forwarded import NEVER_FORWARDED_BAG, find_leaks # --------------------------------------------------------------------------- # Helpers @@ -777,3 +778,47 @@ async def test_stamps_conversation_id_on_graph_span(self) -> None: assert (graph_spans[0].attributes or {}).get( GEN_AI_CONVERSATION_ID ) == "thread-graph" + + +class TestNativeGraphModelParameters: + @pytest.mark.asyncio + async def test_node_options_take_run_settings_and_nothing_never_forwarded( + self, + ) -> None: + """Each node runs its own query with options built from its own config, so a node's + ``max_turns`` applies to that node.""" + mock_sdk = _make_sdk_mock("done") + nodes = { + "root": { + "key": "root", + "config": { + "model": { + "name": "claude-3", + "parameters": {**NEVER_FORWARDED_BAG, "max_turns": 7}, + }, + "instructions": "be helpful", + }, + "meta": {"variationKey": "v1", "version": 1}, + "edges": [], + "is_terminal": True, + } + } + graph_def = _make_graph_def(nodes=nodes) + + captured_options: list[dict[str, Any]] = [] + mock_sdk.ClaudeAgentOptions = MagicMock( + side_effect=lambda **kw: (captured_options.append(kw), kw)[1] + ) + + with patch( + "importlib.import_module", + side_effect=lambda n: ( + mock_sdk if n == "claude_agent_sdk" else __import__(n) + ), + ): + await to_claude_agents(_make_def_promise(graph_def)).invoke("hi") + + assert captured_options + for opts in captured_options: + assert opts["max_turns"] == 7 + assert not find_leaks(opts) diff --git a/packages/claude-agents/tests/test_parameter_forwarding.py b/packages/claude-agents/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..336fde80 --- /dev/null +++ b/packages/claude-agents/tests/test_parameter_forwarding.py @@ -0,0 +1,133 @@ +""" +Drift test for the ``ClaudeAgentOptions`` parameter classification in ``handler.py``. + +``_CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS`` is a literal, hand-maintained list. This test reads +``ClaudeAgentOptions``'s own dataclass fields and asserts every field it declares is classified in +exactly one of forwarded, handler-owned, or excluded, so an SDK field nobody has classified yet +fails loudly by name, and so does a list entry that is not a real SDK field. +""" + +from __future__ import annotations + +import dataclasses +from typing import Any + +from claude_agent_sdk import ClaudeAgentOptions + +from launchdarkly_ai_claude_agents.handler import ( + _CLAUDE_AGENT_OPTIONS_EXCLUDED_KEYS as _EXCLUDED_KEYS, +) +from launchdarkly_ai_claude_agents.handler import ( + _CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS, + _build_query_options, +) +from tests.never_forwarded import NEVER_FORWARDED_BAG, find_leaks + +#: Handler-owned: popped from the filtered params before ``ClaudeAgentOptions(**kwargs)`` is +#: constructed, at every call site (``handler.py`` and ``native_graph.py``). +_OWNED_KEYS = frozenset( + {"model", "allowed_tools", "mcp_servers", "hooks", "tools", "system_prompt"} +) + + +class TestClaudeAgentOptionsAcceptsExactlyTheseFields: + def test_every_field_is_classified_exactly_once(self) -> None: + accepted = frozenset(f.name for f in dataclasses.fields(ClaudeAgentOptions)) + classified = _CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS | _OWNED_KEYS | _EXCLUDED_KEYS + + unclassified = accepted - classified + assert not unclassified, ( + f"ClaudeAgentOptions now declares {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in claude-agents handler.py" + ) + + overlap = ( + (_CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS & _OWNED_KEYS) + | (_CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS & _EXCLUDED_KEYS) + | (_OWNED_KEYS & _EXCLUDED_KEYS) + ) + assert not overlap, f"fields classified more than once: {sorted(overlap)}" + + def test_every_classified_field_is_real(self) -> None: + accepted = frozenset(f.name for f in dataclasses.fields(ClaudeAgentOptions)) + stale = ( + _CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS | _OWNED_KEYS | _EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in claude-agents handler.py but " + "ClaudeAgentOptions does not declare them" + ) + + +def _config(parameters: dict[str, Any]) -> dict[str, Any]: + return { + "model": {"name": "claude-opus-4-5", "parameters": parameters}, + "provider": {"name": "Anthropic"}, + "instructions": "You are helpful.", + } + + +class TestHostProcessSettingsAreNeverForwarded: + def test_cli_path_env_permission_mode_and_add_dirs_do_not_reach_the_options( + self, + ) -> None: + """A config that could set these could launch its own binary as the agent process, + point the customer's API key at another host, skip every permission prompt, and open the + whole filesystem. ``_build_query_options`` must leave all four at their defaults.""" + options = _build_query_options( + _config( + { + "cli_path": "/tmp/attacker-binary", + "env": {"ANTHROPIC_BASE_URL": "https://attacker.example"}, + "permission_mode": "bypassPermissions", + "add_dirs": ["/"], + } + ), + None, + [], + [], + None, + None, + ) + defaults = ClaudeAgentOptions() + assert options.cli_path is None + assert options.env == defaults.env == {} + assert options.permission_mode is None + assert options.add_dirs == defaults.add_dirs == [] + + def test_no_never_forwarded_key_reaches_the_options(self) -> None: + options = _build_query_options( + _config(dict(NEVER_FORWARDED_BAG)), None, [], [], None, None + ) + assert not find_leaks(options) + + def test_the_agreed_run_settings_still_land(self) -> None: + options = _build_query_options( + _config( + { + **NEVER_FORWARDED_BAG, + "max_turns": 4, + "max_thinking_tokens": 2048, + "thinking": {"type": "adaptive"}, + "effort": "high", + "max_budget_usd": 1.5, + "fallback_model": "claude-sonnet-4-5", + "output_format": {"type": "json_schema", "schema": {}}, + "betas": ["context-1m-2025-08-07"], + } + ), + None, + [], + [], + None, + None, + ) + assert options.max_turns == 4 + assert options.max_thinking_tokens == 2048 + assert options.thinking == {"type": "adaptive"} + assert options.effort == "high" + assert options.max_budget_usd == 1.5 + assert options.fallback_model == "claude-sonnet-4-5" + assert options.output_format == {"type": "json_schema", "schema": {}} + assert options.betas == ["context-1m-2025-08-07"] + assert not find_leaks(options) diff --git a/packages/claude-messages/src/launchdarkly_ai_claude_messages/handler.py b/packages/claude-messages/src/launchdarkly_ai_claude_messages/handler.py index 0de014a3..04fd9e49 100644 --- a/packages/claude-messages/src/launchdarkly_ai_claude_messages/handler.py +++ b/packages/claude-messages/src/launchdarkly_ai_claude_messages/handler.py @@ -24,6 +24,8 @@ set_tool_call_content_attributes, to_semconv_finish_reason, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters from .spans import ( RawRunUsage, @@ -42,12 +44,81 @@ to_tool_definitions, ) -try: - import anthropic as _anthropic_mod # noqa: F401 +#: Every key ``AsyncMessages.create``/``.stream`` accept, classified by hand into exactly one of: +#: forwarded (below), handler-owned (``model``, ``messages``, ``system``, ``tools``, set by each +#: call site itself), or excluded (below). ``TestMessagesCreateAcceptsExactlyTheseKeys`` / +#: ``TestMessagesStreamAcceptsExactlyTheseKeys`` in this package's tests assert this +#: classification stays exhaustive as the SDK's own signatures change. +#: +#: Only model and run settings are forwarded, and the same list serves ``invoke`` and ``stream``, +#: so one config behaves the same whichever is called. +_MESSAGES_FORWARDED_KEYS = frozenset( + { + "cache_control", + "max_tokens", + "metadata", + "output_config", + "service_tier", + "stop_sequences", + "temperature", + "thinking", + "tool_choice", + "top_k", + "top_p", + } +) + +#: Accepted by the API but never forwarded, and why: +#: * ``stream``: the handler chooses blocking vs. streaming itself, not via a kwarg. +#: * ``output_format``: only ``.stream`` accepts it, so forwarding it would make ``invoke`` and +#: ``stream`` behave differently. ``output_config`` (forwarded) carries the same output format +#: on both. +#: * ``inference_geo``: the region inference runs in, which decides where data is processed. +#: * ``container``: selects server-side container state carried over from another request, not +#: a model setting. +#: * ``user_profile_id``: attributes the request to a party other than the caller, an identity +#: setting rather than a model setting. +#: * ``timeout``, ``extra_headers``, ``extra_query``, ``extra_body``: client/connection +#: configuration (a request timeout, raw HTTP overrides), never a config-controlled setting. +#: +#: Named for the drift test and for review, not read at runtime: the forwarded list above already +#: leaves these out, so nothing needs to subtract them again. +_MESSAGES_EXCLUDED_KEYS = frozenset( + { + "stream", + "output_format", + "inference_geo", + "container", + "user_profile_id", + "timeout", + "extra_headers", + "extra_query", + "extra_body", + } +) - _HAS_ANTHROPIC = True -except ImportError: - _HAS_ANTHROPIC = False + +def _rename_effort_to_output_config(params: dict[str, Any]) -> dict[str, Any]: + """Moves a top-level ``effort`` into ``output_config.effort``, the shape the Anthropic Messages + API actually accepts (there is no top-level ``effort`` parameter). + + An ``output_config`` the config itself already set wins: if it carries its own ``effort``, the + top-level one is dropped rather than overwriting it. This handler does not itself set + ``output_config`` (structured output goes through a system-prompt instruction instead, see + ``_build_messages``), so there is nothing here to merge with yet; a future call site that starts + setting ``output_config`` must merge so its own keys win and a config's ``effort`` survives. + """ + effort = params.pop("effort", None) + if effort is None: + return params + existing = params.get("output_config") + if isinstance(existing, dict) and "effort" in existing: + return params + params["output_config"] = { + **(existing if isinstance(existing, dict) else {}), + "effort": effort, + } + return params def _build_tools(config_tools: dict[str, Any]) -> list[dict[str, Any]]: @@ -201,9 +272,11 @@ async def _run_tool_loop( # difference predates this span work and changes what the model is offered, not what the span # reports, so it stays as it is: the catalog recorded below is the catalog actually sent. tools = _build_tools(config.get("tools") or {}) - max_tokens = (config.get("model", {}).get("parameters") or {}).get( - "max_tokens", 1024 + extra_params = select_forwarded_parameters( + _rename_effort_to_output_config(model_parameters(config)), + _MESSAGES_FORWARDED_KEYS, ) + max_tokens = extra_params.pop("max_tokens", 1024) conversation = list(messages) output = "" steps = 0 @@ -222,6 +295,7 @@ async def _run_tool_loop( open_model_span = model_span kwargs: dict[str, Any] = { + **extra_params, "model": config["model"]["name"], "max_tokens": max_tokens, "messages": conversation, @@ -494,9 +568,11 @@ async def _stream_gen( tools = _build_tools(config.get("tools") or {}) tool_definitions = to_tool_definitions(tools) - max_tokens = (config.get("model", {}).get("parameters") or {}).get( - "max_tokens", 1024 + extra_params = select_forwarded_parameters( + _rename_effort_to_output_config(model_parameters(config)), + _MESSAGES_FORWARDED_KEYS, ) + max_tokens = extra_params.pop("max_tokens", 1024) conversation = list(messages) full_output = "" steps = 0 @@ -528,6 +604,7 @@ async def _stream_gen( open_model_span = model_span kwargs: dict[str, Any] = { + **extra_params, "model": config["model"]["name"], "max_tokens": max_tokens, "messages": conversation, diff --git a/packages/claude-messages/tests/test_handler.py b/packages/claude-messages/tests/test_handler.py index f07336fe..d7331007 100644 --- a/packages/claude-messages/tests/test_handler.py +++ b/packages/claude-messages/tests/test_handler.py @@ -6,14 +6,22 @@ from __future__ import annotations +import json from collections.abc import AsyncGenerator, AsyncIterator from contextlib import asynccontextmanager from types import SimpleNamespace from typing import Any, ClassVar from unittest.mock import AsyncMock, MagicMock, patch +import httpx import pytest +from tests.never_forwarded import ( + NEVER_FORWARDED_BAG, + NEVER_FORWARDED_KEYS, + find_leaks, +) + # --------------------------------------------------------------------------- # Fake anthropic response helpers # --------------------------------------------------------------------------- @@ -364,6 +372,260 @@ async def test_custom_parameters_passthrough( # --------------------------------------------------------------------------- +class TestModelParametersForwarding: + async def test_snake_case_param_reaches_provider( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"top_p": 0.5}}, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["top_p"] == 0.5 + + async def test_max_tokens_default_preserved_without_parameters( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + h = create_claude_messages_handler() + await h(CONFIG, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["max_tokens"] == 1024 + + async def test_config_max_tokens_wins_over_default( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"max_tokens": 42}}, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["max_tokens"] == 42 + + async def test_config_cannot_override_model_or_messages( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "model": "not-the-real-model", + "messages": "not-the-real-messages", + "system": "not-the-real-system", + }, + }, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["model"] == CONFIG["model"]["name"] + assert call_kwargs["messages"] != "not-the-real-messages" + assert ( + "system" not in call_kwargs + or call_kwargs["system"] != "not-the-real-system" + ) + + async def test_call_unchanged_when_no_parameters_set( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + h = create_claude_messages_handler() + await h(CONFIG, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert set(call_kwargs.keys()) == {"model", "max_tokens", "messages", "system"} + + async def test_streaming_forwards_snake_case_param( + self, mock_anthropic: MagicMock + ) -> None: + import launchdarkly_ai_claude_messages.spans as spans_mod + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + ctx, _ = _make_stream_context(["hi"]) + mock_anthropic.messages.stream = MagicMock(return_value=ctx) + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"top_p": 0.3}}, + } + with patch.object(spans_mod, "_HAS_OTEL", False): + h = create_claude_messages_handler() + gen = await h.stream(config, "q", {}, {}) + async for _event in gen: + pass + call_kwargs = mock_anthropic.messages.stream.call_args.kwargs + assert call_kwargs["top_p"] == 0.3 + + async def test_top_p_and_max_tokens_reach_the_wire(self) -> None: + """Intercepts the real outgoing HTTP request with an httpx MockTransport, rather than + asserting only on a mock of our own call, so this proves the values actually leave the + process on the wire the real ``anthropic`` client builds. + """ + import anthropic + + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + captured: dict[str, Any] = {} + + def _handler(request: httpx.Request) -> httpx.Response: + captured["body"] = json.loads(request.content) + return httpx.Response( + 200, + json={ + "id": "msg_1", + "type": "message", + "role": "assistant", + "model": "claude-3-sonnet-20240229", + "content": [{"type": "text", "text": "hi"}], + "stop_reason": "end_turn", + "usage": {"input_tokens": 3, "output_tokens": 2}, + }, + ) + + transport = httpx.MockTransport(_handler) + real_client = anthropic.AsyncAnthropic( + api_key="test-key", http_client=httpx.AsyncClient(transport=transport) + ) + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": {"top_p": 0.4, "max_tokens": 256}, + }, + } + with patch("anthropic.AsyncAnthropic", return_value=real_client): + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + + assert captured["body"]["top_p"] == 0.4 + assert captured["body"]["max_tokens"] == 256 + + async def test_ui_key_the_sdk_rejects_is_dropped_without_raising( + self, mock_anthropic: MagicMock + ) -> None: + """``effort`` has no top-level equivalent on ``messages.create``; without the rename it + raises ``TypeError`` before this filter existed. Here it is set with no ``output_config``, + so the effort rename applies and the call must not raise. + """ + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"effort": "low"}}, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["output_config"] == {"effort": "low"} + + async def test_transport_key_is_never_forwarded( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": {"top_p": 0.5, "extra_body": {"secret": "value"}}, + }, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["top_p"] == 0.5 + assert "extra_body" not in call_kwargs + + async def test_stream_key_is_never_forwarded( + self, mock_anthropic: MagicMock + ) -> None: + """The handler picks blocking vs. streaming by which client method it calls, not by a + ``stream`` kwarg; a config setting it must not reach ``messages.create``.""" + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"stream": True}}, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert "stream" not in call_kwargs + + +class TestEffortRename: + async def test_explicit_output_config_effort_wins_over_top_level_effort( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "effort": "low", + "output_config": {"effort": "high"}, + }, + }, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["output_config"] == {"effort": "high"} + + async def test_top_level_effort_moves_into_output_config_when_absent( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"effort": "medium"}}, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["output_config"] == {"effort": "medium"} + + async def test_top_level_effort_merges_into_an_output_config_without_effort( + self, mock_anthropic: MagicMock + ) -> None: + """An ``output_config`` present without its own ``effort`` keeps its other keys and gains + the top-level ``effort``.""" + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "effort": "low", + "output_config": {"some_other_key": "kept"}, + }, + }, + } + h = create_claude_messages_handler() + await h(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert call_kwargs["output_config"] == { + "some_other_key": "kept", + "effort": "low", + } + + class TestToolExecutionLoop: async def test_single_tool_call_then_done(self, mock_anthropic: MagicMock) -> None: from launchdarkly_ai_claude_messages import create_claude_messages_handler @@ -2287,3 +2549,80 @@ def _explode_on_the_chat_span(span: Any, capture: bool, **kw: Any) -> None: assert chat.ended == 1 assert StatusCode.ERROR in chat.statuses assert "launchdarkly.stream.abandoned" not in chat.attributes + + +class TestNeverForwardedParameters: + """No credential, endpoint, request-injection, remote-tool, or host-process key in + ``model.parameters`` reaches ``messages.create`` or ``messages.stream``.""" + + async def test_invoke_forwards_none_of_them( + self, mock_anthropic: MagicMock + ) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": dict(NEVER_FORWARDED_BAG)}, + } + await create_claude_messages_handler()(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert not find_leaks(call_kwargs) + assert not set(call_kwargs) & NEVER_FORWARDED_KEYS + + async def test_stream_forwards_none_of_them( + self, mock_anthropic: MagicMock + ) -> None: + import launchdarkly_ai_claude_messages.spans as spans_mod + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + ctx, _ = _make_stream_context(["hi"]) + mock_anthropic.messages.stream = MagicMock(return_value=ctx) + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": dict(NEVER_FORWARDED_BAG)}, + } + with patch.object(spans_mod, "_HAS_OTEL", False): + h = create_claude_messages_handler() + async for _event in await h.stream(config, "q", {}, {}): + pass + call_kwargs = mock_anthropic.messages.stream.call_args.kwargs + assert not find_leaks(call_kwargs) + assert not set(call_kwargs) & NEVER_FORWARDED_KEYS + + +class TestInvokeAndStreamForwardTheSameKeys: + """``output_format`` is accepted by ``messages.stream`` only. Forwarding it there and not on + ``create`` made one config behave differently by call, so it is dropped on both, and + ``output_config`` (accepted by both) carries the output format instead.""" + + _PARAMS: ClassVar[dict[str, Any]] = { + "output_format": {"type": "json_schema", "schema": {}}, + "output_config": {"format": {"type": "json_schema", "schema": {}}}, + "top_p": 0.4, + } + + async def test_invoke(self, mock_anthropic: MagicMock) -> None: + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + config = {**CONFIG, "model": {**CONFIG["model"], "parameters": self._PARAMS}} + await create_claude_messages_handler()(config, "q", {}, {}) + call_kwargs = mock_anthropic.messages.create.call_args.kwargs + assert "output_format" not in call_kwargs + assert call_kwargs["output_config"] == self._PARAMS["output_config"] + assert call_kwargs["top_p"] == 0.4 + + async def test_stream(self, mock_anthropic: MagicMock) -> None: + import launchdarkly_ai_claude_messages.spans as spans_mod + from launchdarkly_ai_claude_messages import create_claude_messages_handler + + ctx, _ = _make_stream_context(["hi"]) + mock_anthropic.messages.stream = MagicMock(return_value=ctx) + config = {**CONFIG, "model": {**CONFIG["model"], "parameters": self._PARAMS}} + with patch.object(spans_mod, "_HAS_OTEL", False): + h = create_claude_messages_handler() + async for _event in await h.stream(config, "q", {}, {}): + pass + call_kwargs = mock_anthropic.messages.stream.call_args.kwargs + assert "output_format" not in call_kwargs + assert call_kwargs["output_config"] == self._PARAMS["output_config"] + assert call_kwargs["top_p"] == 0.4 diff --git a/packages/claude-messages/tests/test_parameter_forwarding.py b/packages/claude-messages/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..5c6a33b8 --- /dev/null +++ b/packages/claude-messages/tests/test_parameter_forwarding.py @@ -0,0 +1,92 @@ +""" +Drift test for the Anthropic Messages API parameter classification in ``handler.py``. + +``_MESSAGES_FORWARDED_KEYS`` is a literal, hand-maintained list, and the same list serves ``invoke`` and ``stream``. +This test reads ``AsyncMessages.create``/``.stream``'s own signatures and asserts every parameter either +accepts is classified in exactly one of forwarded, handler-owned, or excluded, so an SDK parameter +nobody has classified yet fails loudly by name. It also asserts every forwarded or handler-owned key +is accepted by both calls, so ``invoke`` and ``stream`` cannot drift apart again, and that every +excluded key is a real parameter of at least one of them. +""" + +from __future__ import annotations + +import inspect +from collections.abc import Callable + +from anthropic.resources.messages import AsyncMessages + +from launchdarkly_ai_claude_messages.handler import ( + _MESSAGES_EXCLUDED_KEYS, + _MESSAGES_FORWARDED_KEYS, +) + +#: Handler-owned: set by each call site itself, never taken from the config. +_OWNED_KEYS = frozenset({"model", "messages", "system", "tools"}) + + +def _signature_keys(fn: Callable[..., object]) -> frozenset[str]: + keys: set[str] = set() + for name, param in inspect.signature(fn).parameters.items(): + if name == "self" or param.kind is inspect.Parameter.VAR_POSITIONAL: + continue + assert param.kind is not inspect.Parameter.VAR_KEYWORD, ( + f"{fn!r} now accepts **{name}; this test can no longer enumerate its accept-set" + ) + keys.add(name) + return frozenset(keys) + + +_CREATE_KEYS = _signature_keys(AsyncMessages.create) +_STREAM_KEYS = _signature_keys(AsyncMessages.stream) + + +class TestMessagesCreateAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + classified = _MESSAGES_FORWARDED_KEYS | _OWNED_KEYS | _MESSAGES_EXCLUDED_KEYS + + unclassified = _CREATE_KEYS - classified + assert not unclassified, ( + f"AsyncMessages.create now accepts {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in claude-messages handler.py" + ) + + overlap = ( + (_MESSAGES_FORWARDED_KEYS & _OWNED_KEYS) + | (_MESSAGES_FORWARDED_KEYS & _MESSAGES_EXCLUDED_KEYS) + | (_OWNED_KEYS & _MESSAGES_EXCLUDED_KEYS) + ) + assert not overlap, f"keys classified more than once: {sorted(overlap)}" + + def test_every_forwarded_or_owned_key_is_a_real_parameter(self) -> None: + stale = (_MESSAGES_FORWARDED_KEYS | _OWNED_KEYS) - _CREATE_KEYS + assert not stale, ( + f"{sorted(stale)} forwarded or handler-owned in claude-messages handler.py but " + "AsyncMessages.create does not accept them" + ) + + +class TestMessagesStreamAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + classified = _MESSAGES_FORWARDED_KEYS | _OWNED_KEYS | _MESSAGES_EXCLUDED_KEYS + + unclassified = _STREAM_KEYS - classified + assert not unclassified, ( + f"AsyncMessages.stream now accepts {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in claude-messages handler.py" + ) + + def test_every_forwarded_or_owned_key_is_a_real_parameter(self) -> None: + stale = (_MESSAGES_FORWARDED_KEYS | _OWNED_KEYS) - _STREAM_KEYS + assert not stale, ( + f"{sorted(stale)} forwarded or handler-owned in claude-messages handler.py but " + "AsyncMessages.stream does not accept them" + ) + + +def test_every_excluded_key_is_a_real_parameter() -> None: + stale = _MESSAGES_EXCLUDED_KEYS - (_CREATE_KEYS | _STREAM_KEYS) + assert not stale, ( + f"{sorted(stale)} excluded in claude-messages handler.py but neither " + "AsyncMessages.create nor .stream accepts them" + ) diff --git a/packages/client/src/launchdarkly_ai_server/parameter_forwarding.py b/packages/client/src/launchdarkly_ai_server/parameter_forwarding.py new file mode 100644 index 00000000..87463e1f --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/parameter_forwarding.py @@ -0,0 +1,38 @@ +""" +Shared filtering for ``model.parameters`` before it reaches a provider SDK call. + +``model.parameters`` is a free-form dict the LaunchDarkly UI writes, offering keys that make +sense across providers (``temperature``, ``top_p``, ``max_tokens``, ``tool_choice``, ...). No +single provider SDK accepts all of them, and each handler package classifies every key its own +provider entry point accepts into exactly one written-down list: forwarded (model and run settings +only), handler-owned (set by the call site itself), or excluded (accepted by the provider but never +forwarded: credentials, endpoints and connection settings, raw request injection such as extra +headers or ``model_kwargs``, remote tools, host-process settings, and anything that would break +the handler). Only the forwarded list is read at runtime; every other key is dropped. Those lists are +literal, next to each handler's own call sites, not derived from the provider SDK at runtime: a +hand-maintained list is reviewable and a runtime-derived one is not, and each handler package has a +drift test asserting its lists still cover everything its provider SDK accepts. + +This module is the one shared piece: given a params dict and the literal forwarded-keys list, keep +only the keys on that list. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any + + +def select_forwarded_parameters( + params: Mapping[str, Any], forwarded_keys: frozenset[str] +) -> dict[str, Any]: + """Returns the subset of *params* whose key is in *forwarded_keys*. + + *forwarded_keys* is a handler's literal, hand-maintained list of the keys it forwards to its + provider call: never the provider's full accept-set, since a handler's own owned and excluded + keys (including client/connection configuration) are already left off that list. A key present + in *params* but not in *forwarded_keys* is silently dropped rather than raising, matching this + SDK's behaviour before ``model.parameters`` forwarding existed: an unrecognised tuning value is + ignored, not a hard failure. + """ + return {k: v for k, v in params.items() if k in forwarded_keys} diff --git a/packages/client/src/launchdarkly_ai_server/utils.py b/packages/client/src/launchdarkly_ai_server/utils.py index 2958de6e..beedc553 100644 --- a/packages/client/src/launchdarkly_ai_server/utils.py +++ b/packages/client/src/launchdarkly_ai_server/utils.py @@ -36,6 +36,26 @@ def create_handler( ) +def model_parameters(config: AiConfigRep) -> dict[str, Any]: + """ + Returns ``config['model']['parameters']`` as a fresh dict, or ``{}`` when it + is absent or not a usable mapping. + + This is the single place handler packages read provider tuning values from. + Values are forwarded to the provider as-is, keyed by whatever name the + LaunchDarkly UI wrote (already snake_case for every Python provider SDK + here), so no case conversion happens on the way through. Callers must + filter the result to their own forwarded-keys list (see + ``parameter_forwarding.select_forwarded_parameters``) before merging it + into a provider call; nothing else here is safe to forward as-is. + + Never reads ``model.custom`` — that field is not forwarded to providers. + """ + model = config.get("model") if isinstance(config, dict) else None + parameters = model.get("parameters") if isinstance(model, dict) else None + return dict(parameters) if isinstance(parameters, dict) else {} + + def collapse_messages_to_instructions(config: AiConfigRep) -> AiConfigRep: """ When only an agent handler is available for a messages-mode config, collapse diff --git a/packages/client/tests/test_parameter_forwarding.py b/packages/client/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..a40da7ed --- /dev/null +++ b/packages/client/tests/test_parameter_forwarding.py @@ -0,0 +1,29 @@ +"""Tests for the shared model.parameters filter: select_forwarded_parameters.""" + +from __future__ import annotations + +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters + + +class TestSelectForwardedParameters: + def test_keeps_only_forwarded_keys(self) -> None: + params = {"temperature": 0.5, "unknown": 1} + result = select_forwarded_parameters(params, frozenset({"temperature"})) + assert result == {"temperature": 0.5} + + def test_drops_a_key_the_handler_never_classified_as_forwarded(self) -> None: + params = {"temperature": 0.5, "api_key": "secret"} + result = select_forwarded_parameters(params, frozenset({"temperature"})) + assert result == {"temperature": 0.5} + + def test_empty_params_returns_empty(self) -> None: + assert select_forwarded_parameters({}, frozenset({"temperature"})) == {} + + def test_empty_forwarded_keys_returns_empty(self) -> None: + params = {"temperature": 0.5} + assert select_forwarded_parameters(params, frozenset()) == {} + + def test_does_not_mutate_input(self) -> None: + params = {"temperature": 0.5, "unknown": 1} + select_forwarded_parameters(params, frozenset({"temperature"})) + assert params == {"temperature": 0.5, "unknown": 1} diff --git a/packages/client/tests/test_utils.py b/packages/client/tests/test_utils.py index 88f05298..7494bd7f 100644 --- a/packages/client/tests/test_utils.py +++ b/packages/client/tests/test_utils.py @@ -18,6 +18,7 @@ parse_template, parse_usage, ) +from launchdarkly_ai_server.utils import model_parameters # --------------------------------------------------------------------------- # ?3.1 parse_template @@ -348,3 +349,48 @@ def test_does_not_mutate_input(self) -> None: td = {"runId": "r", "modelKey": "m"} omit_model_stamps(td) assert td == {"runId": "r", "modelKey": "m"} + + +class TestModelParameters: + def test_returns_the_parameters_dict(self) -> None: + config = {"model": {"name": "gpt-4o", "parameters": {"temperature": 0.2}}} + assert model_parameters(config) == {"temperature": 0.2} + + def test_returns_a_fresh_dict_not_the_original(self) -> None: + original = {"temperature": 0.2} + config = {"model": {"name": "gpt-4o", "parameters": original}} + result = model_parameters(config) + result["temperature"] = 99 + assert original["temperature"] == 0.2 + + @pytest.mark.parametrize( + "model", + [None, {}, {"name": "gpt-4o"}, {"name": "gpt-4o", "parameters": None}], + ) + def test_returns_empty_dict_when_parameters_absent(self, model: Any) -> None: + config: dict[str, Any] = {} + if model is not None: + config["model"] = model + assert model_parameters(config) == {} + + @pytest.mark.parametrize("parameters", ["not-a-dict", 1, ["a", "b"], True]) + def test_returns_empty_dict_when_parameters_not_a_mapping( + self, parameters: Any + ) -> None: + config = {"model": {"name": "gpt-4o", "parameters": parameters}} + assert model_parameters(config) == {} + + def test_ignores_non_dict_config(self) -> None: + assert model_parameters("not-a-config") == {} # type: ignore[arg-type] + + def test_never_reads_custom(self) -> None: + config = { + "model": { + "name": "gpt-4o", + "parameters": {"temperature": 0.2}, + "custom": {"secret": "value"}, + } + } + result = model_parameters(config) + assert "secret" not in result + assert "custom" not in result diff --git a/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/handler.py b/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/handler.py index 41c69ed6..d4b083b9 100644 --- a/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/handler.py +++ b/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/handler.py @@ -30,6 +30,8 @@ set_input_content_attributes, set_output_content_attributes, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters from .messages import to_lang_chain_messages from .spans import ( @@ -43,6 +45,232 @@ to_tool_definitions, ) +#: Handler-owned, per model class. The handler always sets the model itself, and each +#: class exposes that one constructor field under two names (the field and its alias), so a +#: config value for either must never be forwarded: it would collide with the model the +#: handler resolves, or override it. +_CHAT_OPENAI_OWNED_KEYS = frozenset({"model", "model_name"}) +_CHAT_ANTHROPIC_OWNED_KEYS = frozenset({"model", "model_name"}) +_CHAT_BEDROCK_CONVERSE_OWNED_KEYS = frozenset({"model", "model_id"}) + +#: LangChain's own runtime fields, shared by every chat model class: caching, callbacks, rate +#: limiting, tracing tags and metadata, streaming mode, message format, and token counting. They +#: configure how LangChain runs the model in this process, not the model request, and several are +#: objects or callables a config cannot express. Never forwarded. +_LANGCHAIN_RUNTIME_KEYS = frozenset( + { + "cache", + "callbacks", + "custom_get_token_ids", + "disable_streaming", + "metadata", + "name", + "output_version", + "profile", + "rate_limiter", + "streaming", + "tags", + "verbose", + } +) + +#: Every key ``ChatOpenAI`` accepts (field names plus pydantic aliases), classified by hand into +#: exactly one of: forwarded (below), handler-owned (``model``, always overwritten by the +#: resolved model name, see ``_model_constructor_kwargs``), or excluded (below). +#: ``TestChatOpenAIAcceptsExactlyTheseKeys`` in this package's tests asserts this classification +#: stays exhaustive as the SDK's own pydantic model changes. +#: +#: Only model request settings are forwarded. +_CHAT_OPENAI_FORWARDED_KEYS = frozenset( + { + "context_management", + "frequency_penalty", + "include", + "logit_bias", + "logprobs", + "max_completion_tokens", + "max_tokens", + "n", + "presence_penalty", + "reasoning", + "reasoning_effort", + "seed", + "service_tier", + "stop", + "stop_sequences", + "store", + "temperature", + "top_logprobs", + "top_p", + "truncation", + "verbosity", + } +) + +#: Accepted by ``ChatOpenAI`` but never forwarded, and why: +#: * ``api_key``, ``openai_api_key``, ``organization``, ``openai_organization``: credentials. +#: * ``base_url``, ``openai_api_base``, ``openai_proxy``: where requests go. +#: * ``client``, ``async_client``, ``root_client``, ``root_async_client``, ``http_client``, +#: ``http_async_client``, ``http_socket_options``: raw HTTP client objects/settings. +#: * ``default_headers``, ``default_query``, ``extra_body``, ``model_kwargs``: raw request +#: injection. ``model_kwargs`` is merged straight into the request payload, so it would carry +#: ``extra_headers``/``extra_query`` or any other excluded key past this list. +#: * ``max_retries``, ``request_timeout``, ``timeout``, ``stream_chunk_timeout``: retries and +#: timeouts. +#: * ``stream_usage``, ``include_response_headers``, ``disabled_params``, ``tiktoken_model_name``, +#: ``use_responses_api``, ``use_previous_response_id``: which API and response shape the handler +#: gets back, and how usage is reported to it. +#: * Everything in :data:`_LANGCHAIN_RUNTIME_KEYS`. +#: +#: Named for the drift test and for review, not read at runtime: the forwarded list above already +#: leaves these out, so nothing needs to subtract them again. +_CHAT_OPENAI_EXCLUDED_KEYS = _LANGCHAIN_RUNTIME_KEYS | { + "api_key", + "openai_api_key", + "organization", + "openai_organization", + "base_url", + "openai_api_base", + "openai_proxy", + "client", + "async_client", + "root_client", + "root_async_client", + "http_client", + "http_async_client", + "http_socket_options", + "default_headers", + "default_query", + "extra_body", + "model_kwargs", + "max_retries", + "request_timeout", + "timeout", + "stream_chunk_timeout", + "stream_usage", + "include_response_headers", + "disabled_params", + "tiktoken_model_name", + "use_responses_api", + "use_previous_response_id", +} + +#: Every key ``ChatAnthropic`` accepts (field names plus pydantic aliases), classified the same way +#: as :data:`_CHAT_OPENAI_FORWARDED_KEYS`. ``TestChatAnthropicAcceptsExactlyTheseKeys`` in this +#: package's tests asserts this classification stays exhaustive. +_CHAT_ANTHROPIC_FORWARDED_KEYS = frozenset( + { + "betas", + "context_management", + "effort", + "max_tokens", + "max_tokens_to_sample", + "output_config", + "stop", + "stop_sequences", + "temperature", + "thinking", + "top_k", + "top_p", + } +) + +#: Accepted by ``ChatAnthropic`` but never forwarded, and why: +#: * ``anthropic_api_key``, ``api_key``: credentials (field plus alias). +#: * ``anthropic_api_url``, ``base_url``, ``anthropic_proxy``: where requests go. +#: * ``inference_geo``: the region inference runs in, which decides where data is processed. +#: * ``default_headers``, ``model_kwargs``: raw request injection. ``model_kwargs`` is merged +#: straight into the request, so it would carry any excluded key past this list. +#: * ``mcp_servers``: attaches remote MCP servers, which then receive the conversation. +#: * ``reuse_last_container``: reuses server-side container state from an earlier request, not +#: a model setting. +#: * ``default_request_timeout``, ``timeout``, ``max_retries``: timeouts and retries. +#: * ``stream_usage``: how usage is reported back to the handler. +#: * Everything in :data:`_LANGCHAIN_RUNTIME_KEYS`. +#: +#: Named for the drift test and for review, not read at runtime. +_CHAT_ANTHROPIC_EXCLUDED_KEYS = _LANGCHAIN_RUNTIME_KEYS | { + "anthropic_api_key", + "api_key", + "anthropic_api_url", + "base_url", + "anthropic_proxy", + "inference_geo", + "default_headers", + "model_kwargs", + "mcp_servers", + "reuse_last_container", + "default_request_timeout", + "timeout", + "max_retries", + "stream_usage", +} + +#: Every key ``ChatBedrockConverse`` accepts (field names plus pydantic aliases), classified the +#: same way as :data:`_CHAT_OPENAI_FORWARDED_KEYS`. ``langchain-aws`` is not a dependency of this +#: package (Bedrock support is opt-in, see ``_make_default_chat_model``), so +#: ``TestChatBedrockConverseAcceptsExactlyTheseKeys`` in this package's tests skips itself when it +#: is not installed rather than asserting nothing. +_CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS = frozenset( + { + "guard_last_turn_only", + "guardrail_config", + "guardrails", + "max_tokens", + "output_config", + "performance_config", + "reasoning_effort", + "request_metadata", + "service_tier", + "stop", + "stop_sequences", + "system", + "temperature", + "top_p", + } +) + +#: Accepted by ``ChatBedrockConverse`` but never forwarded, and why: +#: * ``bedrock_api_key``, ``api_key``, ``aws_access_key_id``, ``aws_secret_access_key``, +#: ``aws_session_token``, ``credentials_profile_name``: credentials. +#: * ``endpoint_url``, ``base_url``, ``region_name``: where requests go, and the region, which +#: decides where data is processed. +#: * ``client``, ``bedrock_client``, ``config``: raw boto3/botocore client objects and config. +#: * ``default_headers``, ``additional_model_request_fields``: raw request injection. +#: ``additional_model_request_fields`` is passed into the request body unfiltered. +#: * ``max_retries``, ``timeout``: retries and timeouts. +#: * ``base_model_id``, ``base_model``, ``provider``: which model the handler is talking to, the +#: same identity as ``model_id`` (handler-owned). +#: * ``additional_model_response_field_paths``, ``raw_blocks``, ``supports_tool_choice_values``: +#: the response shape and tool-calling behaviour the handler relies on. +#: * Everything in :data:`_LANGCHAIN_RUNTIME_KEYS`. +#: +#: Named for the drift test and for review, not read at runtime. +_CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS = _LANGCHAIN_RUNTIME_KEYS | { + "bedrock_api_key", + "api_key", + "aws_access_key_id", + "aws_secret_access_key", + "aws_session_token", + "credentials_profile_name", + "endpoint_url", + "base_url", + "region_name", + "client", + "bedrock_client", + "config", + "default_headers", + "additional_model_request_fields", + "max_retries", + "timeout", + "base_model_id", + "base_model", + "provider", + "additional_model_response_field_paths", + "raw_blocks", + "supports_tool_choice_values", +} + def _build_agent_tools( config_tools: dict[str, Any], @@ -176,13 +404,11 @@ def _config_for_model_call(config: AiConfigRep) -> AiConfigRep: def _model_constructor_kwargs( - config: AiConfigRep, fallback_name: str + config: AiConfigRep, + fallback_name: str, + forwarded_keys: frozenset[str], ) -> dict[str, Any]: - raw = (config.get("model") or {}).get("parameters") - parameters = dict(raw) if isinstance(raw, dict) else {} - provider = str((config.get("provider") or {}).get("name") or "").lower() - if provider == "bedrock": - parameters.pop("tools", None) + parameters = select_forwarded_parameters(model_parameters(config), forwarded_keys) parameters["model"] = _resolved_model_name(config, fallback_name) return parameters @@ -205,7 +431,9 @@ def _make_default_chat_model(config: AiConfigRep) -> Any: if provider == "anthropic": lc_anthropic = importlib.import_module("langchain_anthropic") return lc_anthropic.ChatAnthropic( - **_model_constructor_kwargs(config, "claude-3-5-sonnet-20241022") + **_model_constructor_kwargs( + config, "claude-3-5-sonnet-20241022", _CHAT_ANTHROPIC_FORWARDED_KEYS + ) ) if provider == "bedrock": try: @@ -215,9 +443,15 @@ def _make_default_chat_model(config: AiConfigRep) -> Any: "Using Bedrock models requires langchain-aws. " "Install it with: pip install langchain-aws" ) from exc - return lc_aws.ChatBedrockConverse(**_model_constructor_kwargs(config, "")) + return lc_aws.ChatBedrockConverse( + **_model_constructor_kwargs( + config, "", _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS + ) + ) lc_openai = importlib.import_module("langchain_openai") - return lc_openai.ChatOpenAI(**_model_constructor_kwargs(config, "gpt-4o")) + return lc_openai.ChatOpenAI( + **_model_constructor_kwargs(config, "gpt-4o", _CHAT_OPENAI_FORWARDED_KEYS) + ) async def _resolve_base_model(config: AiConfigRep, llm: Any) -> Any: diff --git a/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/native_graph.py b/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/native_graph.py index 190e5f87..6fc4a24d 100644 --- a/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/native_graph.py +++ b/packages/langchain-agents/src/launchdarkly_ai_langchain_agents/native_graph.py @@ -22,7 +22,10 @@ parse_template, to_ld_context, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters +from .handler import _CHAT_OPENAI_FORWARDED_KEYS from .messages import to_lang_chain_messages try: @@ -206,8 +209,11 @@ async def _traverse_node(node: GraphNode) -> None: else: lc_openai = importlib.import_module("langchain_openai") model_cfg = node.config.get("model") or {} - raw = model_cfg.get("parameters") - kwargs = dict(raw) if isinstance(raw, dict) else {} + kwargs = model_parameters(node.config) + if kwargs: + kwargs = select_forwarded_parameters( + kwargs, _CHAT_OPENAI_FORWARDED_KEYS + ) kwargs["model"] = model_cfg.get("name") or "gpt-4o" chat_model = lc_openai.ChatOpenAI(**kwargs) diff --git a/packages/langchain-agents/tests/test_handler.py b/packages/langchain-agents/tests/test_handler.py index e43cb3b0..f64b9204 100644 --- a/packages/langchain-agents/tests/test_handler.py +++ b/packages/langchain-agents/tests/test_handler.py @@ -13,7 +13,9 @@ import pydantic import pytest +from langchain_anthropic import ChatAnthropic as _REAL_CHAT_ANTHROPIC from langchain_core.language_models.chat_models import BaseChatModel +from langchain_openai import ChatOpenAI as _REAL_CHAT_OPENAI import launchdarkly_ai_langchain_agents.handler as handler_mod import launchdarkly_ai_langchain_agents.spans as spans_mod @@ -23,6 +25,16 @@ create_langchain_agents_handler, ) +# ``langchain_aws`` is not a dependency of this package's test environment, so a mocked +# ``ChatBedrockConverse`` gets a minimal hand-built ``model_fields`` shape (field name -> a +# duck-typed stand-in exposing ``.alias``/``.validation_alias``, just like a real pydantic +# ``FieldInfo``) covering the parameters these tests actually forward. +_FAKE_BEDROCK_FIELDS = { + "temperature": SimpleNamespace(alias=None, validation_alias=None), + "top_p": SimpleNamespace(alias=None, validation_alias=None), + "max_tokens": SimpleNamespace(alias=None, validation_alias=None), +} + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -2307,6 +2319,54 @@ async def test_a_reply_in_content_blocks_still_reaches_the_root(self) -> None: assert "a typed block" in str(rec.root.attributes["gen_ai.output.messages"]) +class TestModelParametersForwarding: + @pytest.mark.asyncio + async def test_ui_key_the_sdk_rejects_is_dropped_without_raising(self) -> None: + """``tools`` is not a ``ChatOpenAI`` constructor field (tools are bound via + ``bind_tools`` at call time); forwarding it unfiltered is a landmine, not a crash today + (pydantic ignores unknown constructor kwargs), but this handler must not rely on that.""" + ctx, _rec = _recording() + llm = _FakeToolModel(replies=[_ai_message("answer")]) + ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_OPENAI.model_fields + cfg = { + **BASE_CONFIG, + "model": { + "name": "gpt-4o", + "parameters": {"temperature": 0.2, "tools": [{"name": "x"}]}, + }, + } + with ( + ctx, + patch.dict("sys.modules", {"langchain_openai": MagicMock(ChatOpenAI=ctor)}), + ): + await create_langchain_agents_handler()(cfg, "q") + assert ctor.call_args.kwargs == {"temperature": 0.2, "model": "gpt-4o"} + + @pytest.mark.asyncio + async def test_transport_key_is_never_forwarded(self) -> None: + ctx, _rec = _recording() + llm = _FakeToolModel(replies=[_ai_message("answer")]) + ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_OPENAI.model_fields + cfg = { + **BASE_CONFIG, + "model": { + "name": "gpt-4o", + "parameters": { + "temperature": 0.2, + "extra_body": {"secret": "value"}, + }, + }, + } + with ( + ctx, + patch.dict("sys.modules", {"langchain_openai": MagicMock(ChatOpenAI=ctor)}), + ): + await create_langchain_agents_handler()(cfg, "q") + assert ctor.call_args.kwargs == {"temperature": 0.2, "model": "gpt-4o"} + + class TestModelSource: @pytest.mark.asyncio async def test_factory_receives_config_and_returned_model_is_used(self) -> None: @@ -2354,9 +2414,14 @@ async def test_prebuilt_instance_is_used_as_is(self) -> None: @pytest.mark.asyncio async def test_default_openai_constructor_receives_parameters(self) -> None: + """``tools`` is dropped: it is not a ``ChatOpenAI`` constructor field (tools are bound via + ``bind_tools`` at call time, not passed to the constructor), so the UI offering it must not + raise or silently no-op — it is filtered out before this handler ever reaches the + constructor.""" ctx, _rec = _recording() llm = _FakeToolModel(replies=[_ai_message("default-openai")]) ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_OPENAI.model_fields cfg = { **BASE_CONFIG, "model": { @@ -2376,7 +2441,6 @@ async def test_default_openai_constructor_receives_parameters(self) -> None: assert ctor.call_args.kwargs == { "temperature": 0.2, "max_tokens": 512, - "tools": [{"name": "openai-tool"}], "model": "gpt-4o", } @@ -2385,6 +2449,7 @@ async def test_default_anthropic_constructor_receives_parameters(self) -> None: ctx, _rec = _recording() llm = _FakeToolModel(replies=[_ai_message("default-anthropic")]) ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_ANTHROPIC.model_fields cfg = { **BASE_CONFIG, "provider": {"name": "Anthropic"}, @@ -2409,6 +2474,7 @@ async def test_bedrock_region_is_prepended_to_the_default_constructor( ctx, _rec = _recording() llm = _FakeToolModel(replies=[_ai_message("bedrock")]) ctor = MagicMock(return_value=llm) + ctor.model_fields = _FAKE_BEDROCK_FIELDS cfg = { **BASE_CONFIG, "provider": {"name": "Bedrock"}, @@ -2567,3 +2633,43 @@ def factory(config: Any) -> Any: assert any( e.get("type") == "chunk" and e.get("text") == "streamed" for e in events ) + + +class TestConnectionConfigIsNeverForwarded: + """``api_key``/``base_url`` in ``model.parameters`` are client/connection configuration; a + config author must never be able to redirect a call to a different endpoint or credential. + """ + + @pytest.mark.parametrize( + "provider,fallback", [("openai", "gpt-4o"), ("anthropic", "claude")] + ) + def test_api_key_and_base_url_are_never_forwarded( + self, provider: str, fallback: str + ) -> None: + from launchdarkly_ai_langchain_agents.handler import ( + _CHAT_ANTHROPIC_FORWARDED_KEYS, + _CHAT_OPENAI_FORWARDED_KEYS, + _model_constructor_kwargs, + ) + + forwarded_keys = ( + _CHAT_OPENAI_FORWARDED_KEYS + if provider == "openai" + else _CHAT_ANTHROPIC_FORWARDED_KEYS + ) + cfg = { + **BASE_CONFIG, + "provider": {"name": provider}, + "model": { + "name": fallback, + "parameters": { + "api_key": "stolen-key", + "base_url": "https://evil.example.com", + "temperature": 0.3, + }, + }, + } + kwargs = _model_constructor_kwargs(cfg, fallback, forwarded_keys) + assert "api_key" not in kwargs + assert "base_url" not in kwargs + assert kwargs["temperature"] == 0.3 diff --git a/packages/langchain-agents/tests/test_native_graph.py b/packages/langchain-agents/tests/test_native_graph.py index 2deb0a1e..4dc6c51f 100644 --- a/packages/langchain-agents/tests/test_native_graph.py +++ b/packages/langchain-agents/tests/test_native_graph.py @@ -12,6 +12,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from langchain_openai import ChatOpenAI as _REAL_CHAT_OPENAI from launchdarkly_ai_langchain_agents.native_graph import ( _build_node_tools, @@ -19,6 +20,7 @@ to_lang_graph, ) from launchdarkly_ai_server import GraphDefinition, GraphEdge, GraphNode +from tests.never_forwarded import NEVER_FORWARDED_BAG # --------------------------------------------------------------------------- # Helpers @@ -172,6 +174,7 @@ def compile(self) -> Any: mock_lc_openai = MagicMock() mock_lc_openai.ChatOpenAI = MagicMock(return_value=mock_chat_model) + mock_lc_openai.ChatOpenAI.model_fields = _REAL_CHAT_OPENAI.model_fields mock_gm = MagicMock() mock_gm.add_messages = MagicMock(return_value=MagicMock()) @@ -1006,3 +1009,41 @@ def model_factory(_node: Any) -> Any: assert isinstance(result, dict) assert "response" in result + + +class TestNativeGraphNeverForwardedParameters: + @pytest.mark.asyncio + async def test_default_chat_openai_receives_no_never_forwarded_key(self) -> None: + ai_msg = _make_ai_msg("final") + mocks = _make_langgraph_mocks(ai_msg) + graph_def = _make_graph_def( + nodes={ + "root": { + "key": "root", + "config": { + "model": { + "name": "gpt-4o", + "parameters": {**NEVER_FORWARDED_BAG, "temperature": 0.2}, + }, + "instructions": "help", + }, + "meta": {"variationKey": "v1", "version": 1}, + "edges": [], + "is_terminal": True, + } + } + ) + + async def _visit(fn: Any, ctx: Any = None) -> None: + if graph_def.root is not None: + await fn(graph_def.root) + + graph_def.traverse = _visit + + with _patch_imports(mocks): + await to_lang_graph(_make_def_promise(graph_def)).invoke("hi") + + assert mocks["langchain_openai"].ChatOpenAI.call_args.kwargs == { + "temperature": 0.2, + "model": "gpt-4o", + } diff --git a/packages/langchain-agents/tests/test_parameter_forwarding.py b/packages/langchain-agents/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..9e19cff6 --- /dev/null +++ b/packages/langchain-agents/tests/test_parameter_forwarding.py @@ -0,0 +1,250 @@ +""" +Drift test for the LangChain chat model parameter classification in ``handler.py``. + +``_CHAT_OPENAI_FORWARDED_KEYS`` / ``_CHAT_ANTHROPIC_FORWARDED_KEYS`` / +``_CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS`` are literal, hand-maintained lists. This test reads each +chat model's own ``model_fields`` (field names plus pydantic aliases) and asserts every key it +accepts is classified in exactly one of forwarded, handler-owned, or excluded, so a field nobody has +classified yet fails loudly by name, and so does a list entry that is not a real field. + +``langchain-aws`` is not a dependency of this package (Bedrock support is opt-in), so the +``ChatBedrockConverse`` test skips itself when it is not installed. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +import langchain_anthropic +import langchain_openai +import pytest + +from launchdarkly_ai_langchain_agents.handler import ( + _CHAT_ANTHROPIC_EXCLUDED_KEYS, + _CHAT_ANTHROPIC_FORWARDED_KEYS, + _CHAT_ANTHROPIC_OWNED_KEYS, + _CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS, + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS, + _CHAT_BEDROCK_CONVERSE_OWNED_KEYS, + _CHAT_OPENAI_EXCLUDED_KEYS, + _CHAT_OPENAI_FORWARDED_KEYS, + _CHAT_OPENAI_OWNED_KEYS, + _model_constructor_kwargs, +) +from tests.never_forwarded import NEVER_FORWARDED_BAG + + +def _accepted_keys(cls: Any) -> frozenset[str]: + """Every key *cls* (a pydantic model) accepts by construction: each field's own name plus any + string alias it declares.""" + fields: Mapping[str, Any] = cls.model_fields + accepted: set[str] = set() + for name, field in fields.items(): + accepted.add(name) + alias = getattr(field, "alias", None) + if isinstance(alias, str): + accepted.add(alias) + validation_alias = getattr(field, "validation_alias", None) + if isinstance(validation_alias, str): + accepted.add(validation_alias) + else: + choices = getattr(validation_alias, "choices", None) + if choices: + accepted.update(c for c in choices if isinstance(c, str)) + return frozenset(accepted) + + +class TestChatOpenAIAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + accepted = _accepted_keys(langchain_openai.ChatOpenAI) + classified = ( + _CHAT_OPENAI_FORWARDED_KEYS + | _CHAT_OPENAI_OWNED_KEYS + | _CHAT_OPENAI_EXCLUDED_KEYS + ) + + unclassified = accepted - classified + assert not unclassified, ( + f"ChatOpenAI now accepts {sorted(unclassified)}, not classified as forwarded, " + "handler-owned, or excluded in langchain-agents handler.py" + ) + + overlap = ( + (_CHAT_OPENAI_FORWARDED_KEYS & _CHAT_OPENAI_OWNED_KEYS) + | (_CHAT_OPENAI_FORWARDED_KEYS & _CHAT_OPENAI_EXCLUDED_KEYS) + | (_CHAT_OPENAI_OWNED_KEYS & _CHAT_OPENAI_EXCLUDED_KEYS) + ) + assert not overlap, f"keys classified more than once: {sorted(overlap)}" + + def test_every_classified_key_is_real(self) -> None: + accepted = _accepted_keys(langchain_openai.ChatOpenAI) + stale = ( + _CHAT_OPENAI_FORWARDED_KEYS + | _CHAT_OPENAI_OWNED_KEYS + | _CHAT_OPENAI_EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in langchain-agents handler.py but " + "ChatOpenAI does not accept them" + ) + + +class TestChatAnthropicAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + accepted = _accepted_keys(langchain_anthropic.ChatAnthropic) + classified = ( + _CHAT_ANTHROPIC_FORWARDED_KEYS + | _CHAT_ANTHROPIC_OWNED_KEYS + | _CHAT_ANTHROPIC_EXCLUDED_KEYS + ) + + unclassified = accepted - classified + assert not unclassified, ( + f"ChatAnthropic now accepts {sorted(unclassified)}, not classified as forwarded, " + "handler-owned, or excluded in langchain-agents handler.py" + ) + + def test_every_classified_key_is_real(self) -> None: + accepted = _accepted_keys(langchain_anthropic.ChatAnthropic) + stale = ( + _CHAT_ANTHROPIC_FORWARDED_KEYS + | _CHAT_ANTHROPIC_OWNED_KEYS + | _CHAT_ANTHROPIC_EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in langchain-agents handler.py but " + "ChatAnthropic does not accept them" + ) + + +class TestChatBedrockConverseAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + lc_aws = pytest.importorskip("langchain_aws") + accepted = _accepted_keys(lc_aws.ChatBedrockConverse) + classified = ( + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS + | _CHAT_BEDROCK_CONVERSE_OWNED_KEYS + | _CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS + ) + + unclassified = accepted - classified + assert not unclassified, ( + f"ChatBedrockConverse now accepts {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in langchain-agents handler.py" + ) + + def test_every_classified_key_is_real(self) -> None: + lc_aws = pytest.importorskip("langchain_aws") + accepted = _accepted_keys(lc_aws.ChatBedrockConverse) + stale = ( + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS + | _CHAT_BEDROCK_CONVERSE_OWNED_KEYS + | _CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in langchain-agents handler.py but " + "ChatBedrockConverse does not accept them" + ) + + +class TestModelFieldAliasesAreNeverForwarded: + """``model_name`` / ``model_id`` are the same constructor field as ``model``. A config value + for them must not be forwarded, or it would collide with the model the handler resolves.""" + + def _config(self, provider: str, parameters: dict[str, Any]) -> Any: + return { + "model": {"name": "configured-model", "parameters": parameters}, + "provider": {"name": provider}, + } + + def test_openai_model_name_is_dropped(self) -> None: + kwargs = _model_constructor_kwargs( + self._config("openai", {"model_name": "other", "temperature": 0.2}), + "fallback", + _CHAT_OPENAI_FORWARDED_KEYS, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + def test_anthropic_model_name_is_dropped(self) -> None: + kwargs = _model_constructor_kwargs( + self._config("anthropic", {"model_name": "other", "temperature": 0.2}), + "fallback", + _CHAT_ANTHROPIC_FORWARDED_KEYS, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + def test_bedrock_model_id_is_dropped(self) -> None: + kwargs = _model_constructor_kwargs( + self._config("bedrock", {"model_id": "other", "temperature": 0.2}), + "fallback", + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + +class TestNeverForwardedKeys: + """No credential, endpoint, request-injection, or remote-tool key in ``model.parameters`` + reaches any chat model constructor.""" + + def _config(self, provider: str, parameters: dict[str, Any]) -> Any: + return { + "model": {"name": "configured-model", "parameters": parameters}, + "provider": {"name": provider}, + } + + @pytest.mark.parametrize( + ("provider", "forwarded_keys"), + [ + ("openai", _CHAT_OPENAI_FORWARDED_KEYS), + ("anthropic", _CHAT_ANTHROPIC_FORWARDED_KEYS), + ("bedrock", _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS), + ], + ) + def test_constructor_kwargs_hold_none_of_them( + self, provider: str, forwarded_keys: frozenset[str] + ) -> None: + kwargs = _model_constructor_kwargs( + self._config(provider, {**NEVER_FORWARDED_BAG, "temperature": 0.2}), + "fallback", + forwarded_keys, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + +class TestModelKwargsCannotSmuggleRequestKeys: + """``ChatOpenAI`` merges ``model_kwargs`` straight into the request payload, so forwarding it + would carry ``extra_headers``/``extra_query`` past every exclusion.""" + + _SMUGGLED: ClassVar[dict[str, Any]] = { + "model_kwargs": { + "extra_headers": {"X-Smuggled": "1"}, + "extra_query": {"smuggled": "1"}, + } + } + + def _payload(self, **kwargs: Any) -> dict[str, Any]: + from langchain_core.messages import HumanMessage + + model = langchain_openai.ChatOpenAI(api_key="sk-test-not-a-real-key", **kwargs) + payload: dict[str, Any] = model._get_request_payload([HumanMessage("hi")]) + return payload + + def test_model_kwargs_would_reach_the_payload_if_forwarded(self) -> None: + """The control: passed to ``ChatOpenAI`` directly, both keys reach the payload.""" + payload = self._payload(model="gpt-4o", **self._SMUGGLED) + assert payload["extra_headers"] == {"X-Smuggled": "1"} + assert payload["extra_query"] == {"smuggled": "1"} + + def test_forwarded_model_parameters_keep_them_out_of_the_payload(self) -> None: + kwargs = _model_constructor_kwargs( + { + "model": {"name": "gpt-4o", "parameters": dict(self._SMUGGLED)}, + "provider": {"name": "openai"}, + }, + "fallback", + _CHAT_OPENAI_FORWARDED_KEYS, + ) + payload = self._payload(**kwargs) + assert "extra_headers" not in payload + assert "extra_query" not in payload diff --git a/packages/langchain-messages/src/launchdarkly_ai_langchain_messages/handler.py b/packages/langchain-messages/src/launchdarkly_ai_langchain_messages/handler.py index b30e8b80..4bcd0b3f 100644 --- a/packages/langchain-messages/src/launchdarkly_ai_langchain_messages/handler.py +++ b/packages/langchain-messages/src/launchdarkly_ai_langchain_messages/handler.py @@ -31,6 +31,8 @@ set_output_content_attributes, set_tool_call_content_attributes, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters from .spans import ( fail_span, @@ -45,6 +47,232 @@ to_tool_definitions, ) +#: Handler-owned, per model class. The handler always sets the model itself, and each +#: class exposes that one constructor field under two names (the field and its alias), so a +#: config value for either must never be forwarded: it would collide with the model the +#: handler resolves, or override it. +_CHAT_OPENAI_OWNED_KEYS = frozenset({"model", "model_name"}) +_CHAT_ANTHROPIC_OWNED_KEYS = frozenset({"model", "model_name"}) +_CHAT_BEDROCK_CONVERSE_OWNED_KEYS = frozenset({"model", "model_id"}) + +#: LangChain's own runtime fields, shared by every chat model class: caching, callbacks, rate +#: limiting, tracing tags and metadata, streaming mode, message format, and token counting. They +#: configure how LangChain runs the model in this process, not the model request, and several are +#: objects or callables a config cannot express. Never forwarded. +_LANGCHAIN_RUNTIME_KEYS = frozenset( + { + "cache", + "callbacks", + "custom_get_token_ids", + "disable_streaming", + "metadata", + "name", + "output_version", + "profile", + "rate_limiter", + "streaming", + "tags", + "verbose", + } +) + +#: Every key ``ChatOpenAI`` accepts (field names plus pydantic aliases), classified by hand into +#: exactly one of: forwarded (below), handler-owned (``model``, always overwritten by the +#: resolved model name, see ``_model_constructor_kwargs``), or excluded (below). +#: ``TestChatOpenAIAcceptsExactlyTheseKeys`` in this package's tests asserts this classification +#: stays exhaustive as the SDK's own pydantic model changes. +#: +#: Only model request settings are forwarded. +_CHAT_OPENAI_FORWARDED_KEYS = frozenset( + { + "context_management", + "frequency_penalty", + "include", + "logit_bias", + "logprobs", + "max_completion_tokens", + "max_tokens", + "n", + "presence_penalty", + "reasoning", + "reasoning_effort", + "seed", + "service_tier", + "stop", + "stop_sequences", + "store", + "temperature", + "top_logprobs", + "top_p", + "truncation", + "verbosity", + } +) + +#: Accepted by ``ChatOpenAI`` but never forwarded, and why: +#: * ``api_key``, ``openai_api_key``, ``organization``, ``openai_organization``: credentials. +#: * ``base_url``, ``openai_api_base``, ``openai_proxy``: where requests go. +#: * ``client``, ``async_client``, ``root_client``, ``root_async_client``, ``http_client``, +#: ``http_async_client``, ``http_socket_options``: raw HTTP client objects/settings. +#: * ``default_headers``, ``default_query``, ``extra_body``, ``model_kwargs``: raw request +#: injection. ``model_kwargs`` is merged straight into the request payload, so it would carry +#: ``extra_headers``/``extra_query`` or any other excluded key past this list. +#: * ``max_retries``, ``request_timeout``, ``timeout``, ``stream_chunk_timeout``: retries and +#: timeouts. +#: * ``stream_usage``, ``include_response_headers``, ``disabled_params``, ``tiktoken_model_name``, +#: ``use_responses_api``, ``use_previous_response_id``: which API and response shape the handler +#: gets back, and how usage is reported to it. +#: * Everything in :data:`_LANGCHAIN_RUNTIME_KEYS`. +#: +#: Named for the drift test and for review, not read at runtime: the forwarded list above already +#: leaves these out, so nothing needs to subtract them again. +_CHAT_OPENAI_EXCLUDED_KEYS = _LANGCHAIN_RUNTIME_KEYS | { + "api_key", + "openai_api_key", + "organization", + "openai_organization", + "base_url", + "openai_api_base", + "openai_proxy", + "client", + "async_client", + "root_client", + "root_async_client", + "http_client", + "http_async_client", + "http_socket_options", + "default_headers", + "default_query", + "extra_body", + "model_kwargs", + "max_retries", + "request_timeout", + "timeout", + "stream_chunk_timeout", + "stream_usage", + "include_response_headers", + "disabled_params", + "tiktoken_model_name", + "use_responses_api", + "use_previous_response_id", +} + +#: Every key ``ChatAnthropic`` accepts (field names plus pydantic aliases), classified the same way +#: as :data:`_CHAT_OPENAI_FORWARDED_KEYS`. ``TestChatAnthropicAcceptsExactlyTheseKeys`` in this +#: package's tests asserts this classification stays exhaustive. +_CHAT_ANTHROPIC_FORWARDED_KEYS = frozenset( + { + "betas", + "context_management", + "effort", + "max_tokens", + "max_tokens_to_sample", + "output_config", + "stop", + "stop_sequences", + "temperature", + "thinking", + "top_k", + "top_p", + } +) + +#: Accepted by ``ChatAnthropic`` but never forwarded, and why: +#: * ``anthropic_api_key``, ``api_key``: credentials (field plus alias). +#: * ``anthropic_api_url``, ``base_url``, ``anthropic_proxy``: where requests go. +#: * ``inference_geo``: the region inference runs in, which decides where data is processed. +#: * ``default_headers``, ``model_kwargs``: raw request injection. ``model_kwargs`` is merged +#: straight into the request, so it would carry any excluded key past this list. +#: * ``mcp_servers``: attaches remote MCP servers, which then receive the conversation. +#: * ``reuse_last_container``: reuses server-side container state from an earlier request, not +#: a model setting. +#: * ``default_request_timeout``, ``timeout``, ``max_retries``: timeouts and retries. +#: * ``stream_usage``: how usage is reported back to the handler. +#: * Everything in :data:`_LANGCHAIN_RUNTIME_KEYS`. +#: +#: Named for the drift test and for review, not read at runtime. +_CHAT_ANTHROPIC_EXCLUDED_KEYS = _LANGCHAIN_RUNTIME_KEYS | { + "anthropic_api_key", + "api_key", + "anthropic_api_url", + "base_url", + "anthropic_proxy", + "inference_geo", + "default_headers", + "model_kwargs", + "mcp_servers", + "reuse_last_container", + "default_request_timeout", + "timeout", + "max_retries", + "stream_usage", +} + +#: Every key ``ChatBedrockConverse`` accepts (field names plus pydantic aliases), classified the +#: same way as :data:`_CHAT_OPENAI_FORWARDED_KEYS`. ``langchain-aws`` is not a dependency of this +#: package (Bedrock support is opt-in, see ``_make_default_chat_model``), so +#: ``TestChatBedrockConverseAcceptsExactlyTheseKeys`` in this package's tests skips itself when it +#: is not installed rather than asserting nothing. +_CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS = frozenset( + { + "guard_last_turn_only", + "guardrail_config", + "guardrails", + "max_tokens", + "output_config", + "performance_config", + "reasoning_effort", + "request_metadata", + "service_tier", + "stop", + "stop_sequences", + "system", + "temperature", + "top_p", + } +) + +#: Accepted by ``ChatBedrockConverse`` but never forwarded, and why: +#: * ``bedrock_api_key``, ``api_key``, ``aws_access_key_id``, ``aws_secret_access_key``, +#: ``aws_session_token``, ``credentials_profile_name``: credentials. +#: * ``endpoint_url``, ``base_url``, ``region_name``: where requests go, and the region, which +#: decides where data is processed. +#: * ``client``, ``bedrock_client``, ``config``: raw boto3/botocore client objects and config. +#: * ``default_headers``, ``additional_model_request_fields``: raw request injection. +#: ``additional_model_request_fields`` is passed into the request body unfiltered. +#: * ``max_retries``, ``timeout``: retries and timeouts. +#: * ``base_model_id``, ``base_model``, ``provider``: which model the handler is talking to, the +#: same identity as ``model_id`` (handler-owned). +#: * ``additional_model_response_field_paths``, ``raw_blocks``, ``supports_tool_choice_values``: +#: the response shape and tool-calling behaviour the handler relies on. +#: * Everything in :data:`_LANGCHAIN_RUNTIME_KEYS`. +#: +#: Named for the drift test and for review, not read at runtime. +_CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS = _LANGCHAIN_RUNTIME_KEYS | { + "bedrock_api_key", + "api_key", + "aws_access_key_id", + "aws_secret_access_key", + "aws_session_token", + "credentials_profile_name", + "endpoint_url", + "base_url", + "region_name", + "client", + "bedrock_client", + "config", + "default_headers", + "additional_model_request_fields", + "max_retries", + "timeout", + "base_model_id", + "base_model", + "provider", + "additional_model_response_field_paths", + "raw_blocks", + "supports_tool_choice_values", +} + def _build_tools(config_tools: dict[str, Any]) -> list[dict[str, Any]]: # Not filtered to the tools that have a registered handler, unlike the TypeScript SDK's @@ -226,13 +454,11 @@ def _config_for_model_call(config: AiConfigRep) -> AiConfigRep: def _model_constructor_kwargs( - config: AiConfigRep, fallback_name: str + config: AiConfigRep, + fallback_name: str, + forwarded_keys: frozenset[str], ) -> dict[str, Any]: - raw = (config.get("model") or {}).get("parameters") - parameters = dict(raw) if isinstance(raw, dict) else {} - provider = str((config.get("provider") or {}).get("name") or "").lower() - if provider == "bedrock": - parameters.pop("tools", None) + parameters = select_forwarded_parameters(model_parameters(config), forwarded_keys) # Name from the config always wins over a colliding ``model`` key in the parameter bag. parameters["model"] = _resolved_model_name(config, fallback_name) return parameters @@ -254,7 +480,9 @@ def _make_default_chat_model(config: AiConfigRep, importlib: Any) -> Any: if provider == "anthropic": lc_anthropic = importlib.import_module("langchain_anthropic") return lc_anthropic.ChatAnthropic( - **_model_constructor_kwargs(config, "claude-3-5-sonnet-20241022") + **_model_constructor_kwargs( + config, "claude-3-5-sonnet-20241022", _CHAT_ANTHROPIC_FORWARDED_KEYS + ) ) if provider == "bedrock": try: @@ -264,9 +492,15 @@ def _make_default_chat_model(config: AiConfigRep, importlib: Any) -> Any: "Using Bedrock models requires langchain-aws. " "Install it with: pip install langchain-aws" ) from exc - return lc_aws.ChatBedrockConverse(**_model_constructor_kwargs(config, "")) + return lc_aws.ChatBedrockConverse( + **_model_constructor_kwargs( + config, "", _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS + ) + ) lc_openai = importlib.import_module("langchain_openai") - return lc_openai.ChatOpenAI(**_model_constructor_kwargs(config, "gpt-4o")) + return lc_openai.ChatOpenAI( + **_model_constructor_kwargs(config, "gpt-4o", _CHAT_OPENAI_FORWARDED_KEYS) + ) async def _resolve_base_model(config: AiConfigRep, llm: Any, importlib: Any) -> Any: diff --git a/packages/langchain-messages/tests/test_handler.py b/packages/langchain-messages/tests/test_handler.py index d00e43fe..6482beb0 100644 --- a/packages/langchain-messages/tests/test_handler.py +++ b/packages/langchain-messages/tests/test_handler.py @@ -8,10 +8,21 @@ import json import sys from collections.abc import AsyncGenerator +from types import SimpleNamespace from typing import Any, ClassVar from unittest.mock import AsyncMock, MagicMock, patch import pytest +from langchain_anthropic import ChatAnthropic as _REAL_CHAT_ANTHROPIC +from langchain_openai import ChatOpenAI as _REAL_CHAT_OPENAI + +# ``langchain_aws`` is not a dependency of this package's test environment, so a mocked +# ``ChatBedrockConverse`` gets a minimal hand-built ``model_fields`` shape (field name -> a +# duck-typed stand-in exposing ``.alias``/``.validation_alias``, just like a real pydantic +# ``FieldInfo``) covering the parameters these tests actually forward. +_FAKE_BEDROCK_FIELDS = { + "temperature": SimpleNamespace(alias=None, validation_alias=None), +} # --------------------------------------------------------------------------- # Fake LangChain message helpers @@ -2300,6 +2311,55 @@ def _counting(span: Any, capture: bool, *a: Any, **k: Any) -> None: assert calls["n"] == 0 +class TestModelParametersForwarding: + @pytest.mark.asyncio + async def test_ui_key_the_sdk_rejects_is_dropped_without_raising(self) -> None: + ctx, _rec = _recording() + from launchdarkly_ai_langchain_messages import create_langchain_messages_handler + + llm = _make_llm("answer") + ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_OPENAI.model_fields + cfg = { + **CONFIG, + "model": { + "name": "gpt-4o", + "parameters": {"temperature": 0.2, "tools": [{"name": "x"}]}, + }, + } + with ( + ctx, + patch.dict(sys.modules, {"langchain_openai": MagicMock(ChatOpenAI=ctor)}), + ): + await create_langchain_messages_handler()(cfg, "q", {}, {}) + assert ctor.call_args.kwargs == {"temperature": 0.2, "model": "gpt-4o"} + + @pytest.mark.asyncio + async def test_transport_key_is_never_forwarded(self) -> None: + ctx, _rec = _recording() + from launchdarkly_ai_langchain_messages import create_langchain_messages_handler + + llm = _make_llm("answer") + ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_OPENAI.model_fields + cfg = { + **CONFIG, + "model": { + "name": "gpt-4o", + "parameters": { + "temperature": 0.2, + "extra_body": {"secret": "value"}, + }, + }, + } + with ( + ctx, + patch.dict(sys.modules, {"langchain_openai": MagicMock(ChatOpenAI=ctor)}), + ): + await create_langchain_messages_handler()(cfg, "q", {}, {}) + assert ctor.call_args.kwargs == {"temperature": 0.2, "model": "gpt-4o"} + + class TestModelSource: @pytest.mark.asyncio async def test_factory_receives_config_and_returned_model_is_used(self) -> None: @@ -2362,6 +2422,7 @@ async def test_default_openai_constructor_receives_parameters(self) -> None: llm = _make_llm("default-openai") ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_OPENAI.model_fields cfg = { **CONFIG, "model": { @@ -2378,10 +2439,12 @@ async def test_default_openai_constructor_receives_parameters(self) -> None: patch.dict(sys.modules, {"langchain_openai": MagicMock(ChatOpenAI=ctor)}), ): await create_langchain_messages_handler()(cfg, "q", {}, {}) + # ``tools`` is dropped: it is not a ``ChatOpenAI`` constructor field (tools are bound via + # ``bind_tools`` at call time), so the UI offering it must be filtered out rather than + # forwarded or raising. assert ctor.call_args.kwargs == { "temperature": 0.2, "max_tokens": 512, - "tools": [{"name": "openai-tool"}], "model": "gpt-4o", } @@ -2392,6 +2455,7 @@ async def test_default_anthropic_constructor_receives_parameters(self) -> None: llm = _make_llm("default-anthropic") ctor = MagicMock(return_value=llm) + ctor.model_fields = _REAL_CHAT_ANTHROPIC.model_fields cfg = { **CONFIG, "provider": {"name": "Anthropic"}, @@ -2422,6 +2486,7 @@ async def test_bedrock_region_is_prepended_to_the_default_constructor( llm = _make_llm("bedrock") ctor = MagicMock(return_value=llm) + ctor.model_fields = _FAKE_BEDROCK_FIELDS cfg = { **CONFIG, "provider": {"name": "Bedrock"}, @@ -2586,3 +2651,110 @@ def factory(config: Any) -> Any: assert any( e.get("type") == "chunk" and e.get("text") == "streamed" for e in events ) + + +class TestModelParametersReachTheWire: + """Intercepts the real outgoing HTTP request with an httpx MockTransport, rather than + asserting only on a mock of our own call, so this proves ``top_p`` from ``model.parameters`` + actually leaves the process on the wire the real ``ChatOpenAI`` client builds. + """ + + @pytest.mark.asyncio + async def test_top_p_reaches_the_wire( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import httpx + from langchain_openai.chat_models import base as _chat_openai_base + + ctx, _rec = _recording() + from launchdarkly_ai_langchain_messages import create_langchain_messages_handler + + captured: dict[str, Any] = {} + + def _handler(request: httpx.Request) -> httpx.Response: + captured["body"] = json.loads(request.content) + return httpx.Response( + 200, + json={ + "id": "chatcmpl-1", + "object": "chat.completion", + "created": 1, + "model": "gpt-4o", + "choices": [ + { + "index": 0, + "message": {"role": "assistant", "content": "hi"}, + "finish_reason": "stop", + } + ], + "usage": { + "prompt_tokens": 1, + "completion_tokens": 1, + "total_tokens": 2, + }, + }, + ) + + transport = httpx.MockTransport(_handler) + mock_client = httpx.AsyncClient( + transport=transport, base_url="https://api.openai.com/v1" + ) + # ``http_async_client`` in ``model.parameters`` is client/connection configuration and is + # never forwarded any more (see ``_CHAT_OPENAI_FORWARDED_KEYS``), so the real ``ChatOpenAI`` + # is intercepted below its own default httpx client builder instead: the same mock transport, + # reached without a config value ever naming an HTTP client. The API key comes from the env + # var ``ChatOpenAI`` already falls back to, for the same reason. + monkeypatch.setattr( + _chat_openai_base, + "_get_default_async_httpx_client", + lambda *a, **kw: mock_client, + ) + monkeypatch.setenv("OPENAI_API_KEY", "test-key") + cfg = { + **CONFIG, + "model": {"name": "gpt-4o", "parameters": {"top_p": 0.5}}, + } + with ctx: + result = await create_langchain_messages_handler()(cfg, "q", {}, {}) + assert captured["body"]["top_p"] == 0.5 + assert result["output"] == "hi" + + +class TestConnectionConfigIsNeverForwarded: + """``api_key``/``base_url`` in ``model.parameters`` are client/connection configuration; a + config author must never be able to redirect a call to a different endpoint or credential. + """ + + @pytest.mark.parametrize( + "provider,fallback", [("openai", "gpt-4o"), ("anthropic", "claude")] + ) + def test_api_key_and_base_url_are_never_forwarded( + self, provider: str, fallback: str + ) -> None: + from launchdarkly_ai_langchain_messages.handler import ( + _CHAT_ANTHROPIC_FORWARDED_KEYS, + _CHAT_OPENAI_FORWARDED_KEYS, + _model_constructor_kwargs, + ) + + forwarded_keys = ( + _CHAT_OPENAI_FORWARDED_KEYS + if provider == "openai" + else _CHAT_ANTHROPIC_FORWARDED_KEYS + ) + cfg = { + **CONFIG, + "provider": {"name": provider}, + "model": { + "name": fallback, + "parameters": { + "api_key": "stolen-key", + "base_url": "https://evil.example.com", + "temperature": 0.3, + }, + }, + } + kwargs = _model_constructor_kwargs(cfg, fallback, forwarded_keys) + assert "api_key" not in kwargs + assert "base_url" not in kwargs + assert kwargs["temperature"] == 0.3 diff --git a/packages/langchain-messages/tests/test_parameter_forwarding.py b/packages/langchain-messages/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..3e079b30 --- /dev/null +++ b/packages/langchain-messages/tests/test_parameter_forwarding.py @@ -0,0 +1,250 @@ +""" +Drift test for the LangChain chat model parameter classification in ``handler.py``. + +``_CHAT_OPENAI_FORWARDED_KEYS`` / ``_CHAT_ANTHROPIC_FORWARDED_KEYS`` / +``_CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS`` are literal, hand-maintained lists. This test reads each +chat model's own ``model_fields`` (field names plus pydantic aliases) and asserts every key it +accepts is classified in exactly one of forwarded, handler-owned, or excluded, so a field nobody has +classified yet fails loudly by name, and so does a list entry that is not a real field. + +``langchain-aws`` is not a dependency of this package (Bedrock support is opt-in), so the +``ChatBedrockConverse`` test skips itself when it is not installed. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +import langchain_anthropic +import langchain_openai +import pytest + +from launchdarkly_ai_langchain_messages.handler import ( + _CHAT_ANTHROPIC_EXCLUDED_KEYS, + _CHAT_ANTHROPIC_FORWARDED_KEYS, + _CHAT_ANTHROPIC_OWNED_KEYS, + _CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS, + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS, + _CHAT_BEDROCK_CONVERSE_OWNED_KEYS, + _CHAT_OPENAI_EXCLUDED_KEYS, + _CHAT_OPENAI_FORWARDED_KEYS, + _CHAT_OPENAI_OWNED_KEYS, + _model_constructor_kwargs, +) +from tests.never_forwarded import NEVER_FORWARDED_BAG + + +def _accepted_keys(cls: Any) -> frozenset[str]: + """Every key *cls* (a pydantic model) accepts by construction: each field's own name plus any + string alias it declares.""" + fields: Mapping[str, Any] = cls.model_fields + accepted: set[str] = set() + for name, field in fields.items(): + accepted.add(name) + alias = getattr(field, "alias", None) + if isinstance(alias, str): + accepted.add(alias) + validation_alias = getattr(field, "validation_alias", None) + if isinstance(validation_alias, str): + accepted.add(validation_alias) + else: + choices = getattr(validation_alias, "choices", None) + if choices: + accepted.update(c for c in choices if isinstance(c, str)) + return frozenset(accepted) + + +class TestChatOpenAIAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + accepted = _accepted_keys(langchain_openai.ChatOpenAI) + classified = ( + _CHAT_OPENAI_FORWARDED_KEYS + | _CHAT_OPENAI_OWNED_KEYS + | _CHAT_OPENAI_EXCLUDED_KEYS + ) + + unclassified = accepted - classified + assert not unclassified, ( + f"ChatOpenAI now accepts {sorted(unclassified)}, not classified as forwarded, " + "handler-owned, or excluded in langchain-messages handler.py" + ) + + overlap = ( + (_CHAT_OPENAI_FORWARDED_KEYS & _CHAT_OPENAI_OWNED_KEYS) + | (_CHAT_OPENAI_FORWARDED_KEYS & _CHAT_OPENAI_EXCLUDED_KEYS) + | (_CHAT_OPENAI_OWNED_KEYS & _CHAT_OPENAI_EXCLUDED_KEYS) + ) + assert not overlap, f"keys classified more than once: {sorted(overlap)}" + + def test_every_classified_key_is_real(self) -> None: + accepted = _accepted_keys(langchain_openai.ChatOpenAI) + stale = ( + _CHAT_OPENAI_FORWARDED_KEYS + | _CHAT_OPENAI_OWNED_KEYS + | _CHAT_OPENAI_EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in langchain-messages handler.py but " + "ChatOpenAI does not accept them" + ) + + +class TestChatAnthropicAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + accepted = _accepted_keys(langchain_anthropic.ChatAnthropic) + classified = ( + _CHAT_ANTHROPIC_FORWARDED_KEYS + | _CHAT_ANTHROPIC_OWNED_KEYS + | _CHAT_ANTHROPIC_EXCLUDED_KEYS + ) + + unclassified = accepted - classified + assert not unclassified, ( + f"ChatAnthropic now accepts {sorted(unclassified)}, not classified as forwarded, " + "handler-owned, or excluded in langchain-messages handler.py" + ) + + def test_every_classified_key_is_real(self) -> None: + accepted = _accepted_keys(langchain_anthropic.ChatAnthropic) + stale = ( + _CHAT_ANTHROPIC_FORWARDED_KEYS + | _CHAT_ANTHROPIC_OWNED_KEYS + | _CHAT_ANTHROPIC_EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in langchain-messages handler.py but " + "ChatAnthropic does not accept them" + ) + + +class TestChatBedrockConverseAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + lc_aws = pytest.importorskip("langchain_aws") + accepted = _accepted_keys(lc_aws.ChatBedrockConverse) + classified = ( + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS + | _CHAT_BEDROCK_CONVERSE_OWNED_KEYS + | _CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS + ) + + unclassified = accepted - classified + assert not unclassified, ( + f"ChatBedrockConverse now accepts {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in langchain-messages handler.py" + ) + + def test_every_classified_key_is_real(self) -> None: + lc_aws = pytest.importorskip("langchain_aws") + accepted = _accepted_keys(lc_aws.ChatBedrockConverse) + stale = ( + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS + | _CHAT_BEDROCK_CONVERSE_OWNED_KEYS + | _CHAT_BEDROCK_CONVERSE_EXCLUDED_KEYS + ) - accepted + assert not stale, ( + f"{sorted(stale)} classified in langchain-messages handler.py but " + "ChatBedrockConverse does not accept them" + ) + + +class TestModelFieldAliasesAreNeverForwarded: + """``model_name`` / ``model_id`` are the same constructor field as ``model``. A config value + for them must not be forwarded, or it would collide with the model the handler resolves.""" + + def _config(self, provider: str, parameters: dict[str, Any]) -> Any: + return { + "model": {"name": "configured-model", "parameters": parameters}, + "provider": {"name": provider}, + } + + def test_openai_model_name_is_dropped(self) -> None: + kwargs = _model_constructor_kwargs( + self._config("openai", {"model_name": "other", "temperature": 0.2}), + "fallback", + _CHAT_OPENAI_FORWARDED_KEYS, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + def test_anthropic_model_name_is_dropped(self) -> None: + kwargs = _model_constructor_kwargs( + self._config("anthropic", {"model_name": "other", "temperature": 0.2}), + "fallback", + _CHAT_ANTHROPIC_FORWARDED_KEYS, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + def test_bedrock_model_id_is_dropped(self) -> None: + kwargs = _model_constructor_kwargs( + self._config("bedrock", {"model_id": "other", "temperature": 0.2}), + "fallback", + _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + +class TestNeverForwardedKeys: + """No credential, endpoint, request-injection, or remote-tool key in ``model.parameters`` + reaches any chat model constructor.""" + + def _config(self, provider: str, parameters: dict[str, Any]) -> Any: + return { + "model": {"name": "configured-model", "parameters": parameters}, + "provider": {"name": provider}, + } + + @pytest.mark.parametrize( + ("provider", "forwarded_keys"), + [ + ("openai", _CHAT_OPENAI_FORWARDED_KEYS), + ("anthropic", _CHAT_ANTHROPIC_FORWARDED_KEYS), + ("bedrock", _CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS), + ], + ) + def test_constructor_kwargs_hold_none_of_them( + self, provider: str, forwarded_keys: frozenset[str] + ) -> None: + kwargs = _model_constructor_kwargs( + self._config(provider, {**NEVER_FORWARDED_BAG, "temperature": 0.2}), + "fallback", + forwarded_keys, + ) + assert kwargs == {"model": "configured-model", "temperature": 0.2} + + +class TestModelKwargsCannotSmuggleRequestKeys: + """``ChatOpenAI`` merges ``model_kwargs`` straight into the request payload, so forwarding it + would carry ``extra_headers``/``extra_query`` past every exclusion.""" + + _SMUGGLED: ClassVar[dict[str, Any]] = { + "model_kwargs": { + "extra_headers": {"X-Smuggled": "1"}, + "extra_query": {"smuggled": "1"}, + } + } + + def _payload(self, **kwargs: Any) -> dict[str, Any]: + from langchain_core.messages import HumanMessage + + model = langchain_openai.ChatOpenAI(api_key="sk-test-not-a-real-key", **kwargs) + payload: dict[str, Any] = model._get_request_payload([HumanMessage("hi")]) + return payload + + def test_model_kwargs_would_reach_the_payload_if_forwarded(self) -> None: + """The control: passed to ``ChatOpenAI`` directly, both keys reach the payload.""" + payload = self._payload(model="gpt-4o", **self._SMUGGLED) + assert payload["extra_headers"] == {"X-Smuggled": "1"} + assert payload["extra_query"] == {"smuggled": "1"} + + def test_forwarded_model_parameters_keep_them_out_of_the_payload(self) -> None: + kwargs = _model_constructor_kwargs( + { + "model": {"name": "gpt-4o", "parameters": dict(self._SMUGGLED)}, + "provider": {"name": "openai"}, + }, + "fallback", + _CHAT_OPENAI_FORWARDED_KEYS, + ) + payload = self._payload(**kwargs) + assert "extra_headers" not in payload + assert "extra_query" not in payload diff --git a/packages/openai-agents/src/launchdarkly_ai_openai_agents/handler.py b/packages/openai-agents/src/launchdarkly_ai_openai_agents/handler.py index ac97a9aa..a2ff32d1 100644 --- a/packages/openai-agents/src/launchdarkly_ai_openai_agents/handler.py +++ b/packages/openai-agents/src/launchdarkly_ai_openai_agents/handler.py @@ -44,6 +44,8 @@ set_tool_call_content_attributes, text_message, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters from .spans import ( derive_finish_reason, @@ -72,6 +74,43 @@ except ImportError: # pragma: no cover - `agents` is a hard dependency of this package _RunHooksBase = object # type: ignore[assignment,misc] +#: Every field ``agents.ModelSettings`` declares, classified by hand into exactly one of: forwarded +#: (below) or excluded. ``ModelSettings`` has no handler-owned fields: ``model`` and ``max_turns`` +#: live outside it (on ``Agent``/``Runner.run``; ``max_turns`` is read separately for the run). +#: ``TestModelSettingsAcceptsExactlyTheseFields`` in this package's tests asserts this +#: classification stays exhaustive as the SDK's own dataclass changes. +#: +#: Excluded, and why: all client/connection configuration, never a config-controlled setting. +#: * ``retry``: an HTTP retry count. +#: * ``extra_headers``, ``extra_query``, ``extra_body``, ``extra_args``: raw HTTP/request overrides. +_MODEL_SETTINGS_FORWARDED_KEYS = frozenset( + { + "context_management", + "frequency_penalty", + "include_usage", + "max_tokens", + "metadata", + "parallel_tool_calls", + "presence_penalty", + "prompt_cache_retention", + "reasoning", + "response_include", + "store", + "temperature", + "tool_choice", + "top_logprobs", + "top_p", + "truncation", + "verbosity", + } +) + +#: Named for the drift test and for review, not read at runtime: the forwarded list above already +#: leaves these out, so nothing needs to subtract them again. +_MODEL_SETTINGS_EXCLUDED_KEYS = frozenset( + {"retry", "extra_headers", "extra_query", "extra_body", "extra_args"} +) + def _build_agent_tools( config_tools: dict[str, Any], @@ -214,9 +253,19 @@ def _build_agent_and_prompt( # change, not a telemetry one, so it is left alone. `_call_impl` still returns the parsed # `final_output` object as-is when `outputFormat` is configured, matching the pre-existing # return-shape contract. + # `max_turns` is a `Runner.run` option, not a `ModelSettings` field, so the filter drops it here + # and the run call reads it separately. + model_settings_params = select_forwarded_parameters( + model_parameters(config), _MODEL_SETTINGS_FORWARDED_KEYS + ) agent = Agent( name="assistant", model=config.get("model", {}).get("name", "gpt-4o"), + **( + {"model_settings": agents_mod.ModelSettings(**model_settings_params)} + if model_settings_params + else {} + ), **({"instructions": instructions} if instructions else {}), **({"tools": tools} if tools else {}), ) @@ -502,7 +551,9 @@ async def _call_impl( system_instructions=instructions, messages=to_request_span_messages(prompt), ) - result = await Runner.run(agent, prompt, hooks=hooks) + max_turns = model_parameters(config).get("max_turns") + run_kwargs = {"max_turns": max_turns} if max_turns is not None else {} + result = await Runner.run(agent, prompt, hooks=hooks, **run_kwargs) final_output = result.final_output set_output_content_attributes( span, @@ -632,7 +683,9 @@ async def _stream_gen( system_instructions=instructions, messages=to_request_span_messages(prompt), ) - streamed = Runner.run_streamed(agent, prompt, hooks=hooks) + max_turns = model_parameters(config).get("max_turns") + run_kwargs = {"max_turns": max_turns} if max_turns is not None else {} + streamed = Runner.run_streamed(agent, prompt, hooks=hooks, **run_kwargs) full_output = "" async for event in streamed.stream_events(): diff --git a/packages/openai-agents/src/launchdarkly_ai_openai_agents/native_graph.py b/packages/openai-agents/src/launchdarkly_ai_openai_agents/native_graph.py index ac8b0a90..acececd4 100644 --- a/packages/openai-agents/src/launchdarkly_ai_openai_agents/native_graph.py +++ b/packages/openai-agents/src/launchdarkly_ai_openai_agents/native_graph.py @@ -22,8 +22,14 @@ parse_template, to_ld_context, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters -from .handler import _parse_message_content, _to_openai_agent_items +from .handler import ( + _MODEL_SETTINGS_FORWARDED_KEYS, + _parse_message_content, + _to_openai_agent_items, +) try: from opentelemetry import trace @@ -176,9 +182,21 @@ async def _visit(node_key: str) -> None: agent_name = _sanitize_name(node.key) agent_name_to_key[agent_name] = node.key + node_model_settings_params = select_forwarded_parameters( + model_parameters(node.config), _MODEL_SETTINGS_FORWARDED_KEYS + ) agent = Agent( name=agent_name, model=node.config.get("model", {}).get("name", "gpt-4o"), + **( + { + "model_settings": agents_mod.ModelSettings( + **node_model_settings_params + ) + } + if node_model_settings_params + else {} + ), **({"instructions": instructions} if instructions else {}), **({"tools": tools} if tools else {}), **({"handoffs": child_handoffs} if child_handoffs else {}), @@ -262,7 +280,17 @@ async def on_agent_start(self, context: Any, agent: Any) -> None: root_prompt = _to_openai_agent_items(turns) try: - result = await Runner.run(root_agent, root_prompt, hooks=hooks) + # Only the root node's `max_turns` applies. The whole graph is one `Runner.run`, and + # the Agents SDK counts turns once for that run, across every handoff: `max_turns` is + # a `Runner.run` argument, and `Agent` has no per-agent turn limit to set from a + # child node's config. + root_max_turns = model_parameters(root.config).get("max_turns") + root_run_kwargs = ( + {"max_turns": root_max_turns} if root_max_turns is not None else {} + ) + result = await Runner.run( + root_agent, root_prompt, hooks=hooks, **root_run_kwargs + ) if span: span.set_status(SpanStatusCode.OK) except Exception as exc: diff --git a/packages/openai-agents/tests/test_handler.py b/packages/openai-agents/tests/test_handler.py index 75e20003..ab950f81 100644 --- a/packages/openai-agents/tests/test_handler.py +++ b/packages/openai-agents/tests/test_handler.py @@ -27,6 +27,7 @@ openai_agents, ) from launchdarkly_ai_openai_agents.utils import build_output_type +from tests.never_forwarded import NEVER_FORWARDED_BAG, find_leaks # --------------------------------------------------------------------------- # Fake `agents` SDK @@ -153,10 +154,16 @@ def run_streamed(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: return run_streamed +class FakeModelSettings: + def __init__(self, **kw: Any) -> None: + self.kwargs = kw + + def _fake_agents_module(run: Any = None, run_streamed: Any = None) -> Any: mod = SimpleNamespace() mod.FunctionTool = FakeFunctionTool mod.Agent = FakeAgent + mod.ModelSettings = FakeModelSettings class Runner: pass @@ -1422,6 +1429,148 @@ async def test_emits_no_content_by_default_on_the_streaming_path(self) -> None: # --------------------------------------------------------------------------- +class TestModelParametersForwarding: + async def test_snake_case_param_reaches_model_settings(self) -> None: + run_kwargs: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + run_kwargs["agent"] = agent + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + config = _make_config( + instructions="Be helpful.", + model={"name": "gpt-4o", "parameters": {"top_p": 0.5}}, + ) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(config, "q", {}, {}) + model_settings = run_kwargs["agent"].kwargs["model_settings"] + assert model_settings.kwargs["top_p"] == 0.5 + + async def test_max_turns_from_config_reaches_runner_run(self) -> None: + captured: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + captured["max_turns"] = kw.get("max_turns") + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + config = _make_config( + instructions="Be helpful.", + model={"name": "gpt-4o", "parameters": {"max_turns": 3}}, + ) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(config, "q", {}, {}) + assert captured["max_turns"] == 3 + + async def test_max_turns_from_config_reaches_runner_run_streamed(self) -> None: + captured: dict[str, Any] = {} + + def run_streamed(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + captured["max_turns"] = kw.get("max_turns") + return FakeStreamedResult( + agent, prompt, hooks, [{"output": _text_output("hi")}], "done" + ) + + agents_mod = _fake_agents_module(run_streamed=run_streamed) + config = _make_config( + instructions="Be helpful.", + model={"name": "gpt-4o", "parameters": {"max_turns": 4}}, + ) + with _patched_agents(agents_mod): + events = [] + gen = await create_openai_agent_handler().stream(config, "q", {}, {}) + async for event in gen: + events.append(event) + assert captured["max_turns"] == 4 + + async def test_max_turns_not_owned_by_model_settings(self) -> None: + run_kwargs: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + run_kwargs["agent"] = agent + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + config = _make_config( + instructions="Be helpful.", + model={"name": "gpt-4o", "parameters": {"max_turns": 3}}, + ) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(config, "q", {}, {}) + model_settings = run_kwargs["agent"].kwargs.get("model_settings") + assert model_settings is None or "max_turns" not in model_settings.kwargs + + async def test_call_unchanged_when_no_parameters_set(self) -> None: + run_kwargs: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + run_kwargs["agent"] = agent + run_kwargs["kw"] = kw + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(CONFIG, "q", {}, {}) + assert "model_settings" not in run_kwargs["agent"].kwargs + assert run_kwargs["kw"].get("max_turns") is None + + async def test_ui_keys_the_sdk_rejects_are_dropped_without_raising(self) -> None: + """Neither is a field of ``agents.ModelSettings``; forwarding one unfiltered raises + ``TypeError`` before this filter existed.""" + run_kwargs: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + run_kwargs["agent"] = agent + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + config = _make_config( + instructions="Be helpful.", + model={ + "name": "gpt-4o", + "parameters": { + "top_p": 0.5, + "stop_sequences": ["STOP"], + "seed": 42, + }, + }, + ) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(config, "q", {}, {}) + model_settings = run_kwargs["agent"].kwargs["model_settings"] + assert model_settings.kwargs["top_p"] == 0.5 + assert "stop_sequences" not in model_settings.kwargs + assert "seed" not in model_settings.kwargs + + async def test_transport_key_is_never_forwarded(self) -> None: + run_kwargs: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + run_kwargs["agent"] = agent + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + config = _make_config( + instructions="Be helpful.", + model={ + "name": "gpt-4o", + "parameters": {"top_p": 0.5, "extra_body": {"secret": "value"}}, + }, + ) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(config, "q", {}, {}) + model_settings = run_kwargs["agent"].kwargs["model_settings"] + assert model_settings.kwargs["top_p"] == 0.5 + assert "extra_body" not in model_settings.kwargs + + class TestOutputFormat: def test_absent_output_format_no_change(self) -> None: assert build_output_type(None) is None @@ -2106,3 +2255,57 @@ async def _drain() -> None: assert rec.root.attributes.get("launchdarkly.run.cancelled") is True assert "launchdarkly.stream.abandoned" not in rec.root.attributes + + +class TestNeverForwardedParameters: + """No credential, endpoint, request-injection, remote-tool, or host-process key in + ``model.parameters`` reaches ``ModelSettings`` or the ``Runner`` call.""" + + async def test_invoke_forwards_none_of_them(self) -> None: + captured: dict[str, Any] = {} + + async def run(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + captured["agent"] = agent + captured["run_kwargs"] = kw + await _drive_turns(hooks, agent, prompt, [{"output": _text_output("hi")}]) + return FakeRunResult("done") + + agents_mod = _fake_agents_module(run=run) + config = _make_config( + instructions="Be helpful.", + model={ + "name": "gpt-4o", + "parameters": {**NEVER_FORWARDED_BAG, "temperature": 0.1}, + }, + ) + with _patched_agents(agents_mod): + await create_openai_agent_handler()(config, "q", {}, {}) + model_settings = captured["agent"].kwargs["model_settings"] + assert model_settings.kwargs == {"temperature": 0.1} + assert not find_leaks(captured["run_kwargs"]) + + async def test_stream_forwards_none_of_them(self) -> None: + captured: dict[str, Any] = {} + + def run_streamed(agent: Any, prompt: str, hooks: Any = None, **kw: Any) -> Any: + captured["agent"] = agent + captured["run_kwargs"] = kw + return FakeStreamedResult( + agent, prompt, hooks, [{"output": _text_output("hi")}], "done" + ) + + agents_mod = _fake_agents_module(run_streamed=run_streamed) + config = _make_config( + instructions="Be helpful.", + model={ + "name": "gpt-4o", + "parameters": {**NEVER_FORWARDED_BAG, "temperature": 0.1}, + }, + ) + with _patched_agents(agents_mod): + gen = await create_openai_agent_handler().stream(config, "q", {}, {}) + async for _event in gen: + pass + model_settings = captured["agent"].kwargs["model_settings"] + assert model_settings.kwargs == {"temperature": 0.1} + assert not find_leaks(captured["run_kwargs"]) diff --git a/packages/openai-agents/tests/test_native_graph.py b/packages/openai-agents/tests/test_native_graph.py index 5b87afa4..80b0e567 100644 --- a/packages/openai-agents/tests/test_native_graph.py +++ b/packages/openai-agents/tests/test_native_graph.py @@ -16,6 +16,7 @@ to_openai_agents, ) from launchdarkly_ai_server import GraphDefinition, GraphEdge, GraphNode +from tests.never_forwarded import NEVER_FORWARDED_BAG, find_leaks # --------------------------------------------------------------------------- # Helpers @@ -766,6 +767,46 @@ async def _run_and_fire_hook(agent: Any, text: str, hooks: Any = None) -> Any: assert "$ld:ai:graph:handoff_success" in track_calls +class TestNativeGraphModelParameters: + @pytest.mark.asyncio + async def test_no_never_forwarded_key_reaches_any_agent_or_the_run(self) -> None: + run_result = _make_run_result("out") + agents_mock = _make_agents_mock(run_result) + agents_mock.ModelSettings = MagicMock(side_effect=lambda **kw: dict(kw)) + nodes = { + "root": { + "key": "root", + "config": { + "model": { + "name": "gpt-4o", + "parameters": { + **NEVER_FORWARDED_BAG, + "temperature": 0.1, + "max_turns": 6, + }, + }, + "instructions": "help", + }, + "meta": {"variationKey": "v1", "version": 1}, + "edges": [], + "is_terminal": True, + } + } + graph_def = _make_graph_def(nodes=nodes) + + with patch( + "importlib.import_module", + side_effect=lambda n: agents_mock if n == "agents" else __import__(n), + ): + await to_openai_agents(_make_def_promise(graph_def)).invoke("hi") + + (root_agent,) = agents_mock._created_agents + assert root_agent._kw["model_settings"] == {"temperature": 0.1} + run_kwargs = agents_mock.Runner.run.call_args.kwargs + assert run_kwargs["max_turns"] == 6 + assert not find_leaks({k: v for k, v in run_kwargs.items() if k != "hooks"}) + + class TestBuildNodeToolsSyncHandlers: """``_build_node_tools`` must accept sync handlers (native path skips wrap_tool_handlers).""" diff --git a/packages/openai-agents/tests/test_parameter_forwarding.py b/packages/openai-agents/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..c9020769 --- /dev/null +++ b/packages/openai-agents/tests/test_parameter_forwarding.py @@ -0,0 +1,43 @@ +""" +Drift test for the ``agents.ModelSettings`` parameter classification in ``handler.py``. + +``_MODEL_SETTINGS_FORWARDED_KEYS`` is a literal, hand-maintained list. This test reads +``ModelSettings``'s own dataclass fields and asserts every field it declares is classified in +exactly one of forwarded or excluded, so an SDK field nobody has classified yet fails loudly by +name, and so does a list entry that is not a real SDK field. ``ModelSettings`` has no handler-owned +fields: ``model`` and ``max_turns`` live outside it (on ``Agent``/``Runner.run``). +""" + +from __future__ import annotations + +import dataclasses + +from agents import ModelSettings + +from launchdarkly_ai_openai_agents.handler import ( + _MODEL_SETTINGS_EXCLUDED_KEYS as _EXCLUDED_KEYS, +) +from launchdarkly_ai_openai_agents.handler import _MODEL_SETTINGS_FORWARDED_KEYS + + +class TestModelSettingsAcceptsExactlyTheseFields: + def test_every_field_is_classified_exactly_once(self) -> None: + accepted = frozenset(f.name for f in dataclasses.fields(ModelSettings)) + classified = _MODEL_SETTINGS_FORWARDED_KEYS | _EXCLUDED_KEYS + + unclassified = accepted - classified + assert not unclassified, ( + f"ModelSettings now declares {sorted(unclassified)}, not classified as " + "forwarded or excluded in openai-agents handler.py" + ) + + overlap = _MODEL_SETTINGS_FORWARDED_KEYS & _EXCLUDED_KEYS + assert not overlap, f"fields classified more than once: {sorted(overlap)}" + + def test_every_classified_field_is_real(self) -> None: + accepted = frozenset(f.name for f in dataclasses.fields(ModelSettings)) + stale = (_MODEL_SETTINGS_FORWARDED_KEYS | _EXCLUDED_KEYS) - accepted + assert not stale, ( + f"{sorted(stale)} classified in openai-agents handler.py but " + "ModelSettings does not declare them" + ) diff --git a/packages/openai-messages/src/launchdarkly_ai_openai_messages/handler.py b/packages/openai-messages/src/launchdarkly_ai_openai_messages/handler.py index a404fdcb..d84a52ab 100644 --- a/packages/openai-messages/src/launchdarkly_ai_openai_messages/handler.py +++ b/packages/openai-messages/src/launchdarkly_ai_openai_messages/handler.py @@ -26,6 +26,8 @@ set_output_content_attributes, set_tool_call_content_attributes, ) +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from launchdarkly_ai_server.utils import model_parameters from .spans import ( fail_span, @@ -46,6 +48,97 @@ tool_arguments, ) +#: Every key ``AsyncResponses.create``/``.stream`` accept, classified by hand into exactly one of: +#: forwarded (below), handler-owned (``model``, ``input``, ``previous_response_id``, ``tools``, +#: ``text``, set by each call site itself), or excluded (below). +#: ``TestResponsesCreateAcceptsExactlyTheseKeys`` / ``TestResponsesStreamAcceptsExactlyTheseKeys`` +#: in this package's tests assert this classification stays exhaustive as the SDK's own signatures +#: change. The UI offers several keys the Responses API has never accepted: ``max_tokens``, +#: ``frequency_penalty``, ``presence_penalty``, ``seed``, ``n``, ``stop``, ``response_format``, +#: ``logit_bias``, ``logprobs``, ``max_completion_tokens``, ``audio``, ``modalities``, +#: ``prediction``. Of those, ``max_tokens``/``max_completion_tokens`` are renamed to +#: ``max_output_tokens`` (see :func:`_apply_max_output_tokens_rename`); the rest are dropped. +#: +#: The UI writes reasoning effort as ``reasoning: {"effort": ...}``, the Responses API's own shape, +#: so ``reasoning`` is forwarded as-is and no top-level ``reasoning_effort`` is read. +#: +#: Only model and run settings are forwarded, and the same list serves ``invoke`` and ``stream``, +#: so one config behaves the same whichever is called. +_RESPONSES_FORWARDED_KEYS = frozenset( + { + "context_management", + "include", + "instructions", + "max_output_tokens", + "max_tool_calls", + "metadata", + "moderation", + "parallel_tool_calls", + "prompt_cache_key", + "prompt_cache_retention", + "reasoning", + "safety_identifier", + "service_tier", + "store", + "temperature", + "tool_choice", + "top_logprobs", + "top_p", + "truncation", + "user", + } +) + +#: Accepted by the API but never forwarded, and why: +#: * ``stream``: the handler chooses blocking vs. streaming itself, not via a kwarg. +#: * ``stream_options``: only meaningful together with ``stream=True``, which the handler controls. +#: * ``background``: returns before the output exists, so the handler would get no result. +#: * ``conversation``: server-side conversation state conflicts with the input the handler builds. +#: * ``prompt``: server-side prompt template conflicts with the input the handler builds. +#: * ``response_id``, ``starting_after``: only ``.stream`` accepts them, to resume an existing +#: response rather than start a new one. That is request state, not a setting, and ``invoke`` has +#: no equivalent. +#: * ``text_format``: only ``.stream`` accepts it, and it is a Python type to parse into, which a +#: config cannot express. Structured output goes through ``text``, which the handler owns. +#: * ``timeout``, ``extra_headers``, ``extra_query``, ``extra_body``: client/connection +#: configuration (a request timeout, raw HTTP overrides), never a config-controlled setting. +#: +#: Named for the drift test and for review, not read at runtime: the forwarded list above already +#: leaves these out, so nothing needs to subtract them again. +_RESPONSES_EXCLUDED_KEYS = frozenset( + { + "stream", + "stream_options", + "background", + "conversation", + "prompt", + "response_id", + "starting_after", + "text_format", + "timeout", + "extra_headers", + "extra_query", + "extra_body", + } +) + + +def _apply_max_output_tokens_rename(params: dict[str, Any]) -> dict[str, Any]: + """Renames the Chat-Completions-era ``max_tokens``/``max_completion_tokens`` to the Responses + API's ``max_output_tokens``, the only one of the three the API actually accepts. + + Precedence when more than one is set: an explicit ``max_output_tokens`` wins outright, then + ``max_completion_tokens``, then ``max_tokens``. + """ + max_tokens = params.pop("max_tokens", None) + max_completion_tokens = params.pop("max_completion_tokens", None) + if "max_output_tokens" not in params: + if max_completion_tokens is not None: + params["max_output_tokens"] = max_completion_tokens + elif max_tokens is not None: + params["max_output_tokens"] = max_tokens + return params + def _build_tools(config_tools: dict[str, Any]) -> list[dict[str, Any]]: # Not filtered to the tools that have a registered handler, unlike the TypeScript SDK. That @@ -261,7 +354,12 @@ async def _call_impl( messages=root_messages, ) + extra_params = select_forwarded_parameters( + _apply_max_output_tokens_rename(model_parameters(config)), + _RESPONSES_FORWARDED_KEYS, + ) params: dict[str, Any] = { + **extra_params, "model": config["model"]["name"], "input": input_messages, } @@ -341,6 +439,7 @@ async def _call_impl( client, config, { + **extra_params, "model": config["model"]["name"], "previous_response_id": response.id, "input": tool_outputs, @@ -489,7 +588,12 @@ async def _stream_gen( tool_definitions=tool_definitions, ) + extra_params = select_forwarded_parameters( + _apply_max_output_tokens_rename(model_parameters(config)), + _RESPONSES_FORWARDED_KEYS, + ) stream_params: dict[str, Any] = { + **extra_params, "model": config["model"]["name"], "input": current_input, } diff --git a/packages/openai-messages/tests/test_handler.py b/packages/openai-messages/tests/test_handler.py index 4668976f..813edb55 100644 --- a/packages/openai-messages/tests/test_handler.py +++ b/packages/openai-messages/tests/test_handler.py @@ -12,8 +12,15 @@ from typing import Any, ClassVar from unittest.mock import AsyncMock, MagicMock, patch +import httpx import pytest +from tests.never_forwarded import ( + NEVER_FORWARDED_BAG, + NEVER_FORWARDED_KEYS, + find_leaks, +) + CONFIG = { "model": {"name": "gpt-4o"}, "provider": {"name": "OpenAI"}, @@ -1132,6 +1139,339 @@ async def test_rethrows_error(self, mock_openai: MagicMock) -> None: # --------------------------------------------------------------------------- +class TestModelParametersForwarding: + async def test_snake_case_param_reaches_provider( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"top_p": 0.5}}, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["top_p"] == 0.5 + + async def test_config_cannot_override_model_or_input( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "model": "not-the-real-model", + "input": "not-the-real-input", + }, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["model"] == CONFIG["model"]["name"] + assert kwargs["input"] != "not-the-real-input" + + async def test_call_unchanged_when_no_parameters_set( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + h = create_openai_messages_handler() + await h(CONFIG, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert set(kwargs.keys()) == {"model", "input"} + + async def test_streaming_forwards_snake_case_param( + self, mock_openai: MagicMock + ) -> None: + import launchdarkly_ai_openai_messages.spans as spans_mod + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + mock_openai.responses.stream = MagicMock( + return_value=_make_openai_stream_context(["hi"]) + ) + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"top_p": 0.3}}, + } + with patch.object(spans_mod, "_HAS_OTEL", False): + h = create_openai_messages_handler() + events = [e async for e in await h.stream(config, "q")] + assert events + stream_kwargs = mock_openai.responses.stream.call_args.kwargs + assert stream_kwargs["top_p"] == 0.3 + + async def test_ui_keys_the_sdk_rejects_are_dropped_without_raising( + self, mock_openai: MagicMock + ) -> None: + """None of these are ``responses.create`` parameters (they are Chat-Completions-era + keys); forwarding one unfiltered raises ``TypeError`` before this filter existed.""" + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "max_tokens": 100, + "frequency_penalty": 0.1, + "presence_penalty": 0.1, + "seed": 42, + "n": 1, + "stop": ["END"], + "response_format": {"type": "text"}, + "logit_bias": {"50256": -100}, + "logprobs": True, + "max_completion_tokens": 50, + "audio": {"voice": "alloy"}, + "modalities": ["text"], + "prediction": {"type": "content", "content": "x"}, + "top_p": 0.5, + }, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["top_p"] == 0.5 + for rejected in ( + "frequency_penalty", + "presence_penalty", + "seed", + "n", + "stop", + "response_format", + "logit_bias", + "logprobs", + "audio", + "modalities", + "prediction", + ): + assert rejected not in kwargs + + async def test_transport_key_is_never_forwarded( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": {"top_p": 0.5, "extra_body": {"secret": "value"}}, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["top_p"] == 0.5 + assert "extra_body" not in kwargs + + async def test_stream_key_is_never_forwarded(self, mock_openai: MagicMock) -> None: + """The handler picks blocking vs. streaming by which client method it calls; a config + setting ``stream`` must not reach ``responses.create``, and other handler-controlled + transport keys (``background``, ``conversation``, ``prompt``, ``stream_options``) are + likewise dropped.""" + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "stream": True, + "stream_options": {"include_usage": True}, + "background": True, + "conversation": "conv_123", + "prompt": {"id": "pmpt_123"}, + }, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + for excluded in ( + "stream", + "stream_options", + "background", + "conversation", + "prompt", + ): + assert excluded not in kwargs + + async def test_store_and_other_non_generation_keys_are_forwarded( + self, mock_openai: MagicMock + ) -> None: + """Only keys that would break the handler are excluded; everything else the API accepts + is forwarded, even keys that are not strictly generation settings.""" + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "store": False, + "user": "user-1", + "safety_identifier": "safe-1", + "prompt_cache_key": "cache-1", + "metadata": {"k": "v"}, + "service_tier": "auto", + "instructions": "be terse", + "moderation": "auto", + }, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["store"] is False + assert kwargs["user"] == "user-1" + assert kwargs["safety_identifier"] == "safe-1" + assert kwargs["prompt_cache_key"] == "cache-1" + assert kwargs["metadata"] == {"k": "v"} + assert kwargs["service_tier"] == "auto" + assert kwargs["moderation"] == "auto" + # `instructions` is a plain accepted Responses API parameter here, not one this handler + # sets itself (it builds the system prompt into `input`, not a top-level `instructions` + # field), so a config value forwards through like any other accepted key. + assert kwargs["instructions"] == "be terse" + + +class TestMaxOutputTokensRename: + async def test_max_tokens_renamed_to_max_output_tokens( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"max_tokens": 111}}, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["max_output_tokens"] == 111 + assert "max_tokens" not in kwargs + + async def test_max_completion_tokens_renamed_to_max_output_tokens( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": {"max_completion_tokens": 222}, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["max_output_tokens"] == 222 + assert "max_completion_tokens" not in kwargs + + async def test_explicit_max_output_tokens_wins_over_both( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": { + "max_output_tokens": 333, + "max_completion_tokens": 222, + "max_tokens": 111, + }, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["max_output_tokens"] == 333 + + async def test_max_completion_tokens_wins_over_max_tokens_when_both_set( + self, mock_openai: MagicMock + ) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": { + **CONFIG["model"], + "parameters": {"max_completion_tokens": 222, "max_tokens": 111}, + }, + } + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert kwargs["max_output_tokens"] == 222 + + +class TestModelParametersReachTheWire: + """Intercepts the real outgoing HTTP request with an httpx MockTransport, rather than + asserting only on a mock of our own call, so this proves the renamed key actually leaves the + process on the wire the real ``openai`` client builds. + """ + + async def test_max_output_tokens_reaches_the_wire_for_a_config_max_tokens( + self, + ) -> None: + import openai + + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + captured: dict[str, Any] = {} + + def _handler(request: httpx.Request) -> httpx.Response: + captured["body"] = json.loads(request.content) + return httpx.Response( + 200, + json={ + "id": "resp_1", + "object": "response", + "created_at": 1, + "status": "completed", + "model": "gpt-4o", + "output": [ + { + "type": "message", + "role": "assistant", + "content": [{"type": "output_text", "text": "hi"}], + } + ], + "usage": { + "input_tokens": 3, + "output_tokens": 2, + "total_tokens": 5, + }, + }, + ) + + transport = httpx.MockTransport(_handler) + real_client = openai.AsyncOpenAI( + api_key="test-key", http_client=httpx.AsyncClient(transport=transport) + ) + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": {"max_tokens": 256}}, + } + with patch("openai.AsyncOpenAI", return_value=real_client): + h = create_openai_messages_handler() + await h(config, "q", {}, {}) + + assert captured["body"]["max_output_tokens"] == 256 + assert "max_tokens" not in captured["body"] + + class TestOutputFormat: async def test_absent_output_format_no_change(self, mock_openai: MagicMock) -> None: from launchdarkly_ai_openai_messages import create_openai_messages_handler @@ -2180,3 +2520,76 @@ async def test_a_reasoning_item_with_no_summary_is_dropped( chat = rec.named("chat ")[0] assert chat.attributes["gen_ai.completion.0.content"] == "the real answer" assert "gen_ai.completion.1.content" not in chat.attributes + + +class TestNeverForwardedParameters: + """No credential, endpoint, request-injection, remote-tool, or host-process key in + ``model.parameters`` reaches ``responses.create`` or ``responses.stream``.""" + + async def test_invoke_forwards_none_of_them(self, mock_openai: MagicMock) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": dict(NEVER_FORWARDED_BAG)}, + } + await create_openai_messages_handler()(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert not find_leaks(kwargs) + assert not set(kwargs) & NEVER_FORWARDED_KEYS + + async def test_stream_forwards_none_of_them(self, mock_openai: MagicMock) -> None: + import launchdarkly_ai_openai_messages.spans as spans_mod + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + mock_openai.responses.stream = MagicMock( + return_value=_make_openai_stream_context(["hi"]) + ) + config = { + **CONFIG, + "model": {**CONFIG["model"], "parameters": dict(NEVER_FORWARDED_BAG)}, + } + with patch.object(spans_mod, "_HAS_OTEL", False): + h = create_openai_messages_handler() + [e async for e in await h.stream(config, "q")] + kwargs = mock_openai.responses.stream.call_args.kwargs + assert not find_leaks(kwargs) + assert not set(kwargs) & NEVER_FORWARDED_KEYS + + +class TestInvokeAndStreamForwardTheSameKeys: + """``response_id``, ``starting_after`` and ``text_format`` are accepted by + ``responses.stream`` only. Forwarding them there and not on ``create`` made one config behave + differently by call. They resume an existing response or name a Python type to parse into, + neither of which is a setting, so both paths drop them.""" + + _PARAMS: ClassVar[dict[str, Any]] = { + "response_id": "resp-other", + "starting_after": 3, + "text_format": "not-a-type", + "top_p": 0.4, + } + + async def test_invoke(self, mock_openai: MagicMock) -> None: + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + config = {**CONFIG, "model": {**CONFIG["model"], "parameters": self._PARAMS}} + await create_openai_messages_handler()(config, "q", {}, {}) + kwargs = mock_openai.responses.create.call_args.kwargs + assert not {"response_id", "starting_after", "text_format"} & set(kwargs) + assert kwargs["top_p"] == 0.4 + + async def test_stream(self, mock_openai: MagicMock) -> None: + import launchdarkly_ai_openai_messages.spans as spans_mod + from launchdarkly_ai_openai_messages import create_openai_messages_handler + + mock_openai.responses.stream = MagicMock( + return_value=_make_openai_stream_context(["hi"]) + ) + config = {**CONFIG, "model": {**CONFIG["model"], "parameters": self._PARAMS}} + with patch.object(spans_mod, "_HAS_OTEL", False): + h = create_openai_messages_handler() + [e async for e in await h.stream(config, "q")] + kwargs = mock_openai.responses.stream.call_args.kwargs + assert not {"response_id", "starting_after", "text_format"} & set(kwargs) + assert kwargs["top_p"] == 0.4 diff --git a/packages/openai-messages/tests/test_parameter_forwarding.py b/packages/openai-messages/tests/test_parameter_forwarding.py new file mode 100644 index 00000000..dc512c51 --- /dev/null +++ b/packages/openai-messages/tests/test_parameter_forwarding.py @@ -0,0 +1,92 @@ +""" +Drift test for the OpenAI Responses API parameter classification in ``handler.py``. + +``_RESPONSES_FORWARDED_KEYS`` is a literal, hand-maintained list, and the same list serves ``invoke`` and ``stream``. +This test reads ``AsyncResponses.create``/``.stream``'s own signatures and asserts every parameter either +accepts is classified in exactly one of forwarded, handler-owned, or excluded, so an SDK parameter +nobody has classified yet fails loudly by name. It also asserts every forwarded or handler-owned key +is accepted by both calls, so ``invoke`` and ``stream`` cannot drift apart again, and that every +excluded key is a real parameter of at least one of them. +""" + +from __future__ import annotations + +import inspect +from collections.abc import Callable + +from openai.resources.responses import AsyncResponses + +from launchdarkly_ai_openai_messages.handler import ( + _RESPONSES_EXCLUDED_KEYS, + _RESPONSES_FORWARDED_KEYS, +) + +#: Handler-owned: set by each call site itself, never taken from the config. +_OWNED_KEYS = frozenset({"model", "input", "previous_response_id", "tools", "text"}) + + +def _signature_keys(fn: Callable[..., object]) -> frozenset[str]: + keys: set[str] = set() + for name, param in inspect.signature(fn).parameters.items(): + if name == "self" or param.kind is inspect.Parameter.VAR_POSITIONAL: + continue + assert param.kind is not inspect.Parameter.VAR_KEYWORD, ( + f"{fn!r} now accepts **{name}; this test can no longer enumerate its accept-set" + ) + keys.add(name) + return frozenset(keys) + + +_CREATE_KEYS = _signature_keys(AsyncResponses.create) +_STREAM_KEYS = _signature_keys(AsyncResponses.stream) + + +class TestResponsesCreateAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + classified = _RESPONSES_FORWARDED_KEYS | _OWNED_KEYS | _RESPONSES_EXCLUDED_KEYS + + unclassified = _CREATE_KEYS - classified + assert not unclassified, ( + f"AsyncResponses.create now accepts {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in openai-messages handler.py" + ) + + overlap = ( + (_RESPONSES_FORWARDED_KEYS & _OWNED_KEYS) + | (_RESPONSES_FORWARDED_KEYS & _RESPONSES_EXCLUDED_KEYS) + | (_OWNED_KEYS & _RESPONSES_EXCLUDED_KEYS) + ) + assert not overlap, f"keys classified more than once: {sorted(overlap)}" + + def test_every_forwarded_or_owned_key_is_a_real_parameter(self) -> None: + stale = (_RESPONSES_FORWARDED_KEYS | _OWNED_KEYS) - _CREATE_KEYS + assert not stale, ( + f"{sorted(stale)} forwarded or handler-owned in openai-messages handler.py but " + "AsyncResponses.create does not accept them" + ) + + +class TestResponsesStreamAcceptsExactlyTheseKeys: + def test_every_accepted_key_is_classified_exactly_once(self) -> None: + classified = _RESPONSES_FORWARDED_KEYS | _OWNED_KEYS | _RESPONSES_EXCLUDED_KEYS + + unclassified = _STREAM_KEYS - classified + assert not unclassified, ( + f"AsyncResponses.stream now accepts {sorted(unclassified)}, not classified as " + "forwarded, handler-owned, or excluded in openai-messages handler.py" + ) + + def test_every_forwarded_or_owned_key_is_a_real_parameter(self) -> None: + stale = (_RESPONSES_FORWARDED_KEYS | _OWNED_KEYS) - _STREAM_KEYS + assert not stale, ( + f"{sorted(stale)} forwarded or handler-owned in openai-messages handler.py but " + "AsyncResponses.stream does not accept them" + ) + + +def test_every_excluded_key_is_a_real_parameter() -> None: + stale = _RESPONSES_EXCLUDED_KEYS - (_CREATE_KEYS | _STREAM_KEYS) + assert not stale, ( + f"{sorted(stale)} excluded in openai-messages handler.py but neither " + "AsyncResponses.create nor .stream accepts them" + ) diff --git a/tests/never_forwarded.py b/tests/never_forwarded.py new file mode 100644 index 00000000..24a34e8a --- /dev/null +++ b/tests/never_forwarded.py @@ -0,0 +1,122 @@ +"""The keys no handler may ever forward from ``model.parameters``, shared by every package's tests. + +Each handler forwards only an explicit allowlist of model and run settings and drops every other +key. This module is the other half of that rule: the keys that must never be on any allowlist, +whatever the provider SDK accepts. ``test_never_forwarded_parameters.py`` checks every handler's +forwarded lists against it, and each package's own tests check their real call sites with +:data:`NEVER_FORWARDED_BAG`. +""" + +from __future__ import annotations + +#: Credentials, endpoints and connection settings, raw request injection, remote tools, and the +#: Claude Agents host-process settings. +NEVER_FORWARDED_KEYS = frozenset( + { + # Credentials. + "api_key", + "openai_api_key", + "anthropic_api_key", + "bedrock_api_key", + "credentials", + "credentials_profile_name", + "auth_token", + "organization", + "aws_access_key_id", + "aws_secret_access_key", + "aws_session_token", + "aws_region", + # Endpoints and connection. + "base_url", + "openai_api_base", + "anthropic_api_url", + "endpoint", + "endpoint_url", + "region", + "region_name", + "inference_geo", + "client_options", + "client", + "async_client", + "http_client", + "http_async_client", + "default_headers", + "default_query", + "timeout", + "request_timeout", + "default_request_timeout", + "max_retries", + "retry", + "proxies", + "openai_proxy", + "anthropic_proxy", + # Request injection. + "headers", + "extra_headers", + "extra_body", + "extra_query", + "extra_args", + "model_kwargs", + "provider_data", + "additional_model_request_fields", + # Remote tools. + "mcp_servers", + # Claude Agents host-process settings. + "cli_path", + "env", + "cwd", + "add_dirs", + "permission_mode", + "settings", + "setting_sources", + "plugins", + "sandbox", + "resume", + "session_id", + "fork_session", + "continue_conversation", + "hooks", + "can_use_tool", + "stderr", + "session_store", + } +) + + +def leaked(key: str) -> str: + """A value no handler sets itself, so finding it anywhere in a provider call means *key* + leaked through from the config.""" + return f"leaked-from-config:{key}" + + +#: ``model.parameters`` holding every never-forwarded key, each set to its own :func:`leaked` +#: marker. +NEVER_FORWARDED_BAG = {key: leaked(key) for key in NEVER_FORWARDED_KEYS} + + +def find_leaks(value: object) -> set[str]: + """Every :func:`leaked` marker found anywhere inside *value* (dicts, lists, tuples, and object + attributes are searched).""" + found: set[str] = set() + seen: set[int] = set() + + def walk(v: object) -> None: + if id(v) in seen: + return + seen.add(id(v)) + if isinstance(v, str): + if v.startswith("leaked-from-config:"): + found.add(v.split(":", 1)[1]) + elif isinstance(v, dict): + for k, item in v.items(): + walk(k) + walk(item) + elif isinstance(v, (list, tuple, set, frozenset)): + for item in v: + walk(item) + elif hasattr(v, "__dict__") and not isinstance(v, type): + for item in vars(v).values(): + walk(item) + + walk(value) + return found diff --git a/tests/test_never_forwarded_parameters.py b/tests/test_never_forwarded_parameters.py new file mode 100644 index 00000000..c6f82e6c --- /dev/null +++ b/tests/test_never_forwarded_parameters.py @@ -0,0 +1,74 @@ +"""Cross-handler invariant: no handler forwards a never-forwarded key from ``model.parameters``. + +Each handler forwards only its own literal allowlist of model and run settings. This checks every +one of those lists, in all six packages, against the shared list of keys that must never be +forwarded (``never_forwarded.py``). Each package's own tests check its real call sites with the +same keys. +""" + +from __future__ import annotations + +import importlib + +import pytest + +from launchdarkly_ai_server.parameter_forwarding import select_forwarded_parameters +from tests.never_forwarded import ( + NEVER_FORWARDED_BAG, + NEVER_FORWARDED_KEYS, + find_leaks, + leaked, +) + +#: Handler module to the forwarded lists it defines. Listed rather than discovered so that a list +#: renamed or added without being checked here fails below by name. +FORWARDED_LISTS: dict[str, set[str]] = { + "launchdarkly_ai_claude_agents.handler": {"_CLAUDE_AGENT_OPTIONS_FORWARDED_KEYS"}, + "launchdarkly_ai_claude_messages.handler": {"_MESSAGES_FORWARDED_KEYS"}, + "launchdarkly_ai_openai_agents.handler": {"_MODEL_SETTINGS_FORWARDED_KEYS"}, + "launchdarkly_ai_openai_messages.handler": {"_RESPONSES_FORWARDED_KEYS"}, + "launchdarkly_ai_langchain_agents.handler": { + "_CHAT_OPENAI_FORWARDED_KEYS", + "_CHAT_ANTHROPIC_FORWARDED_KEYS", + "_CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS", + }, + "launchdarkly_ai_langchain_messages.handler": { + "_CHAT_OPENAI_FORWARDED_KEYS", + "_CHAT_ANTHROPIC_FORWARDED_KEYS", + "_CHAT_BEDROCK_CONVERSE_FORWARDED_KEYS", + }, +} + +_CASES = [ + (module, name) + for module, names in FORWARDED_LISTS.items() + for name in sorted(names) +] + + +@pytest.mark.parametrize("module", sorted(FORWARDED_LISTS)) +def test_every_forwarded_list_is_checked(module: str) -> None: + mod = importlib.import_module(module) + defined = {name for name in vars(mod) if name.endswith("_FORWARDED_KEYS")} + assert defined == FORWARDED_LISTS[module], ( + f"{module} defines forwarded lists {sorted(defined)}; " + f"this test checks {sorted(FORWARDED_LISTS[module])}" + ) + + +@pytest.mark.parametrize(("module", "name"), _CASES) +def test_no_forwarded_list_holds_a_never_forwarded_key(module: str, name: str) -> None: + forwarded: frozenset[str] = getattr(importlib.import_module(module), name) + assert not forwarded & NEVER_FORWARDED_KEYS, ( + f"{module}.{name} forwards {sorted(forwarded & NEVER_FORWARDED_KEYS)}" + ) + assert select_forwarded_parameters(NEVER_FORWARDED_BAG, forwarded) == {} + + +def test_find_leaks_finds_a_marker_at_any_depth() -> None: + class _Holder: + def __init__(self) -> None: + self.options = {"nested": [("x", leaked("cli_path"))]} + + assert find_leaks({"a": _Holder(), "b": leaked("env")}) == {"cli_path", "env"} + assert find_leaks({"a": "fine", "b": [1, 2]}) == set()