mirror of
https://github.com/usestrix/strix.git
synced 2026-08-28 05:25:00 +00:00
Bind dedupe LLM credentials to the model instead of extra_args
A dedicated dedupe model (STRIX_DEDUPE_MODEL) carried its own
DEDUPE_LLM_API_KEY / DEDUPE_LLM_API_BASE through
ModelSettings.extra_args. LitellmModel already passes api_key= to
litellm.acompletion() explicitly and then splats extra_args into the
same call, so the key arrived twice and every dedupe request died
before it was sent:
TypeError: litellm.main.acompletion() got multiple values for
keyword argument 'api_key'
The OpenAI route was broken the same way: chat.completions.create()
has no api_key parameter at all.
Credentials now ride on the model rather than on the request.
StrixProvider takes optional api_key/api_base, forwarding them to
MultiProvider for the OpenAI route and binding them onto the
LitellmModel for the LiteLLM route, which is what those constructor
arguments are for. dedupe_model_provider() builds that provider, so a
dedicated dedupe model keeps its endpoint and key separate from the
main model's process-wide config exactly as before.
Fixes #1095
This commit is contained in:
parent
8ede419dcc
commit
62840537e5
4 changed files with 127 additions and 40 deletions
|
|
@ -445,12 +445,43 @@ def _response_usage(usage: Usage | None) -> ResponseUsage | None:
|
|||
)
|
||||
|
||||
|
||||
def _bind_route_credentials(model: Model, api_key: str | None, api_base: str | None) -> Model:
|
||||
"""Bind a route's own credential and endpoint to a LiteLLM model instance.
|
||||
|
||||
``LitellmModel`` reads both off itself and forwards them to
|
||||
``litellm.acompletion`` as explicit keyword arguments, so the same values
|
||||
passed through ``ModelSettings.extra_args`` would reach that call twice and
|
||||
raise ``got multiple values for keyword argument 'api_key'``. The OpenAI
|
||||
route is covered by ``MultiProvider``'s own client settings instead.
|
||||
"""
|
||||
if api_key is None and api_base is None:
|
||||
return model
|
||||
|
||||
from agents.extensions.models.litellm_model import LitellmModel
|
||||
|
||||
if isinstance(model, LitellmModel):
|
||||
if api_key is not None:
|
||||
model.api_key = api_key
|
||||
if api_base is not None:
|
||||
model.base_url = api_base
|
||||
return model
|
||||
|
||||
|
||||
class StrixProvider(MultiProvider):
|
||||
"""Route any non-OpenAI prefix through LiteLLM with the prefix preserved,
|
||||
so users type ``deepseek/deepseek-chat`` rather than
|
||||
``litellm/deepseek/deepseek-chat``.
|
||||
|
||||
``api_key``/``api_base`` scope a credential and endpoint to the models this
|
||||
provider hands out, for routes that must not inherit the process-wide LLM
|
||||
config. Omit them to keep using the global configuration.
|
||||
"""
|
||||
|
||||
def __init__(self, *, api_key: str | None = None, api_base: str | None = None) -> None:
|
||||
super().__init__(openai_api_key=api_key, openai_base_url=api_base)
|
||||
self._api_key = api_key
|
||||
self._api_base = api_base
|
||||
|
||||
def _resolve_prefixed_model(
|
||||
self,
|
||||
*,
|
||||
|
|
@ -482,7 +513,9 @@ class StrixProvider(MultiProvider):
|
|||
reasoning_effort=llm.reasoning_effort,
|
||||
)
|
||||
else:
|
||||
model = super().get_model(model_name)
|
||||
model = _bind_route_credentials(
|
||||
super().get_model(model_name), self._api_key, self._api_base
|
||||
)
|
||||
if llm.disable_streaming:
|
||||
model = _NonStreamingModel(model)
|
||||
# The wrapper emits its single event only once the whole request
|
||||
|
|
|
|||
|
|
@ -127,12 +127,10 @@ def _subscription_error_hint(exc: BaseException) -> str | None:
|
|||
|
||||
|
||||
async def warm_up_llm(show_model_warning: bool = True) -> None:
|
||||
from agents.model_settings import ModelSettings
|
||||
from agents.models.interface import ModelTracing
|
||||
|
||||
from strix.config.models import (
|
||||
RECOMMENDED_MODEL_NAMES,
|
||||
StrixProvider,
|
||||
configure_sdk_model_defaults,
|
||||
is_known_openai_bare_model,
|
||||
is_recommended_or_frontier_model,
|
||||
|
|
@ -209,15 +207,14 @@ async def warm_up_llm(show_model_warning: bool = True) -> None:
|
|||
logger.info("LLM warm-up succeeded for model %s", (llm.model or "").strip())
|
||||
|
||||
if settings.dedupe.model:
|
||||
from strix.report.dedupe import _dedupe_extra_args
|
||||
from strix.report.dedupe import dedupe_model_provider
|
||||
|
||||
dedupe_model = settings.dedupe.model.strip()
|
||||
raw_model = dedupe_model
|
||||
deduper = StrixProvider().get_model(dedupe_model)
|
||||
deduper_extra = _dedupe_extra_args(settings.dedupe)
|
||||
# A dedicated dedupe model may route to another provider, which must
|
||||
# never receive the main endpoint's headers; it has its own
|
||||
# DEDUPE_LLM_EXTRA_HEADERS.
|
||||
# never receive the main endpoint's credentials or headers; it has
|
||||
# its own DEDUPE_LLM_* settings, bound to the model by the provider.
|
||||
deduper = dedupe_model_provider(settings.dedupe).get_model(dedupe_model)
|
||||
deduper_settings = make_model_settings(
|
||||
None,
|
||||
model_name=dedupe_model,
|
||||
|
|
@ -226,9 +223,6 @@ async def warm_up_llm(show_model_warning: bool = True) -> None:
|
|||
extra_headers=settings.dedupe.extra_headers,
|
||||
has_tools=False,
|
||||
)
|
||||
if deduper_extra:
|
||||
merged = {**(deduper_settings.extra_args or {}), **deduper_extra}
|
||||
deduper_settings = deduper_settings.resolve(ModelSettings(extra_args=merged))
|
||||
await asyncio.wait_for(
|
||||
deduper.get_response(
|
||||
system_instructions="You are a helpful assistant.",
|
||||
|
|
|
|||
|
|
@ -7,7 +7,6 @@ import logging
|
|||
import re
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
from agents.model_settings import ModelSettings
|
||||
from agents.models.interface import ModelTracing
|
||||
from openai.types.responses import ResponseOutputMessage
|
||||
|
||||
|
|
@ -22,6 +21,7 @@ from strix.report.state import get_global_report_state
|
|||
|
||||
if TYPE_CHECKING:
|
||||
from agents.items import ModelResponse
|
||||
from agents.model_settings import ModelSettings
|
||||
|
||||
from strix.config.settings import DedupeSettings
|
||||
|
||||
|
|
@ -29,30 +29,36 @@ if TYPE_CHECKING:
|
|||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _dedupe_extra_args(dedupe: DedupeSettings) -> dict[str, str]:
|
||||
"""Per-call credential + endpoint for the dedupe model.
|
||||
def dedupe_model_provider(dedupe: DedupeSettings) -> StrixProvider:
|
||||
"""Provider carrying the dedupe model's own credential + endpoint.
|
||||
|
||||
Provider env vars and the global base URL are process-wide, so a
|
||||
shared-provider dedupe key or a distinct dedupe endpoint can't be installed
|
||||
globally without clobbering (or being clobbered by) the main model's
|
||||
config. Passing them per call keeps the two apart. Only applies when a
|
||||
dedicated dedupe model is configured.
|
||||
config. Binding them to the dedupe model keeps the two apart. They must
|
||||
ride on the model rather than on ``ModelSettings.extra_args``: LiteLLM
|
||||
already receives an explicit ``api_key`` on every call, so an ``extra_args``
|
||||
copy arrives as a duplicate keyword argument and the call fails before it
|
||||
is sent. Only applies when a dedicated dedupe model is configured.
|
||||
"""
|
||||
if not dedupe.model:
|
||||
return {}
|
||||
extra: dict[str, str] = {}
|
||||
if dedupe.api_key and dedupe.api_key.strip():
|
||||
extra["api_key"] = dedupe.api_key.strip()
|
||||
if dedupe.api_base and dedupe.api_base.strip():
|
||||
extra["api_base"] = dedupe.api_base.strip()
|
||||
return extra
|
||||
return StrixProvider()
|
||||
return StrixProvider(
|
||||
api_key=_stripped_or_none(dedupe.api_key),
|
||||
api_base=_stripped_or_none(dedupe.api_base),
|
||||
)
|
||||
|
||||
|
||||
def _stripped_or_none(value: str | None) -> str | None:
|
||||
stripped = (value or "").strip()
|
||||
return stripped or None
|
||||
|
||||
|
||||
def _dedupe_model_settings(
|
||||
dedupe: DedupeSettings, model_name: str, request_timeout: float | None
|
||||
) -> ModelSettings:
|
||||
llm = load_settings().llm
|
||||
settings = make_model_settings(
|
||||
return make_model_settings(
|
||||
dedupe.reasoning_effort,
|
||||
model_name=model_name,
|
||||
force_required_tool_choice=False,
|
||||
|
|
@ -64,10 +70,6 @@ def _dedupe_model_settings(
|
|||
extra_headers=dedupe.extra_headers if dedupe.model else llm.extra_headers,
|
||||
has_tools=False,
|
||||
)
|
||||
extra = _dedupe_extra_args(dedupe)
|
||||
if extra:
|
||||
settings = settings.resolve(ModelSettings(extra_args=extra))
|
||||
return settings
|
||||
|
||||
|
||||
DEDUPE_SYSTEM_PROMPT = """You are an expert vulnerability report deduplication judge.
|
||||
|
|
@ -371,7 +373,7 @@ async def check_duplicate(
|
|||
|
||||
configure_sdk_model_defaults(settings)
|
||||
resolved_model = model_name.strip()
|
||||
model = StrixProvider().get_model(resolved_model)
|
||||
model = dedupe_model_provider(dedupe).get_model(resolved_model)
|
||||
response = await model.get_response(
|
||||
system_instructions=DEDUPE_SYSTEM_PROMPT,
|
||||
input=user_msg,
|
||||
|
|
|
|||
|
|
@ -5,9 +5,13 @@ from __future__ import annotations
|
|||
import json
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from agents.extensions.models.litellm_model import LitellmModel
|
||||
from agents.model_settings import ModelSettings
|
||||
from agents.models.interface import ModelTracing
|
||||
|
||||
from strix.config import loader
|
||||
from strix.config.settings import DedupeSettings
|
||||
from strix.report.dedupe import _dedupe_model_settings
|
||||
from strix.report.dedupe import _dedupe_model_settings, dedupe_model_provider
|
||||
|
||||
|
||||
if TYPE_CHECKING:
|
||||
|
|
@ -16,12 +20,31 @@ if TYPE_CHECKING:
|
|||
import pytest
|
||||
|
||||
|
||||
def test_dedupe_key_sent_per_call_not_via_global_env() -> None:
|
||||
def _litellm_route(dedupe: DedupeSettings, model_name: str) -> LitellmModel:
|
||||
model = dedupe_model_provider(dedupe).get_model(model_name)
|
||||
while not isinstance(model, LitellmModel):
|
||||
model = model._inner # type: ignore[attr-defined]
|
||||
return model
|
||||
|
||||
|
||||
def test_dedupe_key_bound_to_model_not_via_global_env() -> None:
|
||||
dedupe = DedupeSettings(STRIX_DEDUPE_MODEL="deepseek/cheap", DEDUPE_LLM_API_KEY="dedupe-key")
|
||||
# The key rides on the dedupe model, so a shared-provider main key can't
|
||||
# clobber it (and vice versa) through the global provider env var.
|
||||
assert _litellm_route(dedupe, "deepseek/cheap").api_key == "dedupe-key"
|
||||
|
||||
|
||||
def test_dedupe_credentials_never_ride_on_extra_args() -> None:
|
||||
# LiteLLM already receives an explicit api_key on every call; a copy in
|
||||
# extra_args reaches acompletion() twice and fails the call outright.
|
||||
dedupe = DedupeSettings(
|
||||
STRIX_DEDUPE_MODEL="deepseek/cheap",
|
||||
DEDUPE_LLM_API_KEY="dedupe-key",
|
||||
DEDUPE_LLM_API_BASE="https://dedupe.example/v1",
|
||||
)
|
||||
settings = _dedupe_model_settings(dedupe, "deepseek/cheap", 300)
|
||||
# The key rides on the request, so a shared-provider main key can't clobber
|
||||
# it (and vice versa) through the global provider env var.
|
||||
assert (settings.extra_args or {})["api_key"] == "dedupe-key"
|
||||
assert "api_key" not in (settings.extra_args or {})
|
||||
assert "api_base" not in (settings.extra_args or {})
|
||||
|
||||
|
||||
def test_dedupe_settings_omit_api_key_when_unset() -> None:
|
||||
|
|
@ -29,19 +52,54 @@ def test_dedupe_settings_omit_api_key_when_unset() -> None:
|
|||
settings = _dedupe_model_settings(dedupe, "deepseek/cheap", 300)
|
||||
assert "api_key" not in (settings.extra_args or {})
|
||||
assert "api_base" not in (settings.extra_args or {})
|
||||
assert _litellm_route(dedupe, "deepseek/cheap").api_key is None
|
||||
|
||||
|
||||
def test_dedupe_endpoint_sent_per_call() -> None:
|
||||
def test_dedupe_endpoint_bound_to_model() -> None:
|
||||
dedupe = DedupeSettings(
|
||||
STRIX_DEDUPE_MODEL="openai/cheap",
|
||||
STRIX_DEDUPE_MODEL="deepseek/cheap",
|
||||
DEDUPE_LLM_API_KEY="dedupe-key",
|
||||
DEDUPE_LLM_API_BASE="https://dedupe.example/v1",
|
||||
)
|
||||
settings = _dedupe_model_settings(dedupe, "openai/cheap", 300)
|
||||
# A distinct dedupe endpoint rides on the request instead of the
|
||||
# A distinct dedupe endpoint rides on the dedupe model instead of the
|
||||
# process-wide base URL, so it can't clobber the main model's endpoint.
|
||||
assert (settings.extra_args or {})["api_base"] == "https://dedupe.example/v1"
|
||||
assert (settings.extra_args or {})["api_key"] == "dedupe-key"
|
||||
route = _litellm_route(dedupe, "deepseek/cheap")
|
||||
assert route.base_url == "https://dedupe.example/v1"
|
||||
assert route.api_key == "dedupe-key"
|
||||
|
||||
|
||||
def test_fallback_dedupe_model_keeps_global_credentials() -> None:
|
||||
# Without a dedicated dedupe model the main model's global config applies.
|
||||
route = _litellm_route(DedupeSettings(DEDUPE_LLM_API_KEY="dedupe-key"), "deepseek/cheap")
|
||||
assert route.api_key is None
|
||||
assert route.base_url is None
|
||||
|
||||
|
||||
async def test_dedupe_call_does_not_duplicate_litellm_api_key() -> None:
|
||||
"""Regression for #1095: the dedupe call reached litellm with two api_key values."""
|
||||
dedupe = DedupeSettings(
|
||||
STRIX_DEDUPE_MODEL="deepseek/cheap",
|
||||
DEDUPE_LLM_API_KEY="dedupe-key",
|
||||
DEDUPE_LLM_API_BASE="https://dedupe.example/v1",
|
||||
)
|
||||
model = dedupe_model_provider(dedupe).get_model("deepseek/cheap")
|
||||
settings = _dedupe_model_settings(dedupe, "deepseek/cheap", 300)
|
||||
settings = settings.resolve(
|
||||
ModelSettings(extra_args={**(settings.extra_args or {}), "mock_response": "OK"})
|
||||
)
|
||||
response = await model.get_response(
|
||||
system_instructions="You are a helpful assistant.",
|
||||
input="Reply with just 'OK'.",
|
||||
model_settings=settings,
|
||||
tools=[],
|
||||
output_schema=None,
|
||||
handoffs=[],
|
||||
tracing=ModelTracing.DISABLED,
|
||||
previous_response_id=None,
|
||||
conversation_id=None,
|
||||
prompt=None,
|
||||
)
|
||||
assert response.output
|
||||
|
||||
|
||||
def test_dedicated_dedupe_model_uses_own_headers_not_main() -> None:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue