mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
fix(auxiliary): apply review fixes to #36043 — guard named-custom routing, drop dead key assignment, tighten Palantir host match
Review follow-ups on the cherry-picked #36043 commit: 1. Guard the custom:<name> passthrough with a _get_named_custom_provider lookup. The PR unconditionally kept the full custom:<name> string, which broke config-less runtime custom providers (#34777 regression — entries that exist only in the live runtime, not config.yaml): the named arm found no entry and resolution fell through to Step 2. Now custom:<name> only takes the named arm when a config entry actually exists; otherwise it collapses to the anonymous-custom arm with the runtime endpoint, preserving pre-PR behavior. 2. Drop the dead 'explicit_api_key = runtime_api_key' assignment (and its misleading comment) in the named-entry branch. resolve_provider_client's named-custom arm derives the key exclusively from the entry's api_key/key_env and never reads explicit_api_key, so the assignment was a no-op. Wiring precedence in was not justified: for a named custom provider the runtime key IS the entry's key (set_runtime_main sources it from the same config), so deletion is the honest option. 3. Tighten the Palantir Bearer-auth check from a loose substring match ('palantirfoundry' in normalized) to a hostname match via base_url_host_matches(..., 'palantirfoundry.com'), so path segments or lookalike domains containing the string no longer trigger Bearer auth. Tests: named-custom anthropic_messages end-to-end routing (full name kept, AnthropicAuxiliaryClient at the original /anthropic URL, no /v1 rewrite) plus Palantir Bearer-auth positive and substring-false-positive cases.
This commit is contained in:
parent
367d3758d5
commit
adb647269a
4 changed files with 141 additions and 14 deletions
|
|
@ -534,8 +534,9 @@ def _requires_bearer_auth(base_url: str | None) -> bool:
|
|||
|
||||
Some third-party /anthropic endpoints implement Anthropic's Messages API but
|
||||
require Authorization: Bearer instead of Anthropic's native x-api-key header.
|
||||
MiniMax's global and China Anthropic-compatible endpoints, and Azure AI
|
||||
Foundry's Anthropic-style endpoint follow this pattern.
|
||||
MiniMax's global and China Anthropic-compatible endpoints, Azure AI
|
||||
Foundry's Anthropic-style endpoint, and Palantir Foundry's LLM proxy
|
||||
follow this pattern.
|
||||
"""
|
||||
normalized = _normalize_base_url_text(base_url)
|
||||
if not normalized:
|
||||
|
|
@ -544,7 +545,11 @@ def _requires_bearer_auth(base_url: str | None) -> bool:
|
|||
return (
|
||||
normalized.startswith(("https://api.minimax.io/anthropic", "https://api.minimaxi.com/anthropic"))
|
||||
or "azure.com" in normalized
|
||||
or "palantirfoundry" in normalized
|
||||
# Palantir Foundry LLM proxy (<org>.palantirfoundry.com/api/v2/llm/proxy/anthropic)
|
||||
# rejects x-api-key with 401 and requires Authorization: Bearer.
|
||||
# Hostname match (not substring) so e.g. evil.com/palantirfoundry
|
||||
# paths don't trigger Bearer auth.
|
||||
or base_url_host_matches(normalized, "palantirfoundry.com")
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -4392,17 +4392,32 @@ def _resolve_auto(
|
|||
explicit_api_key = runtime_api_key or None
|
||||
elif main_provider.startswith("custom:"):
|
||||
# Named custom provider (custom_providers / providers dict entry).
|
||||
# KEEP the full ``custom:<name>`` so resolve_provider_client lands in
|
||||
# the named-custom-provider arm — that arm honours the entry's
|
||||
# api_mode (e.g. anthropic_messages → AnthropicAuxiliaryClient,
|
||||
# avoiding the /anthropic→/v1 rewrite that 404s against proxies
|
||||
# like Palantir Foundry's Anthropic surface). Do NOT collapse to
|
||||
# plain "custom"; that path strips /anthropic and routes through
|
||||
# OpenAI chat.completions.
|
||||
resolved_provider = main_provider
|
||||
# base_url / api_key come from the named entry itself — leave the
|
||||
# explicit_* overrides unset so the named arm reads them from config.
|
||||
if runtime_api_key:
|
||||
_has_named_entry = False
|
||||
try:
|
||||
from hermes_cli.runtime_provider import _get_named_custom_provider
|
||||
_has_named_entry = _get_named_custom_provider(main_provider) is not None
|
||||
except ImportError:
|
||||
pass
|
||||
if _has_named_entry:
|
||||
# KEEP the full ``custom:<name>`` so resolve_provider_client
|
||||
# lands in the named-custom-provider arm — that arm honours the
|
||||
# entry's api_mode (e.g. anthropic_messages →
|
||||
# AnthropicAuxiliaryClient, avoiding the /anthropic→/v1 rewrite
|
||||
# that 404s against proxies like Palantir Foundry's Anthropic
|
||||
# surface). Do NOT collapse to plain "custom"; that path
|
||||
# strips /anthropic and routes through OpenAI chat.completions.
|
||||
# base_url and api_key come from the named entry itself, so
|
||||
# leave the explicit_* overrides unset.
|
||||
resolved_provider = main_provider
|
||||
explicit_base_url = None
|
||||
elif runtime_base_url:
|
||||
# Config-less named custom provider (#34777): the entry only
|
||||
# exists in the live runtime, so collapse to the anonymous
|
||||
# custom arm with the runtime endpoint + key.
|
||||
resolved_provider = "custom"
|
||||
explicit_base_url = runtime_base_url
|
||||
explicit_api_key = runtime_api_key or None
|
||||
elif runtime_api_key:
|
||||
explicit_api_key = runtime_api_key
|
||||
elif runtime_api_key:
|
||||
# Pin auxiliary to the same api_key as the active main chat session
|
||||
|
|
|
|||
|
|
@ -201,6 +201,44 @@ class TestBuildAnthropicClient:
|
|||
betas = kwargs["default_headers"]["anthropic-beta"]
|
||||
assert "context-1m-2025-08-07" in betas
|
||||
|
||||
def test_palantir_foundry_anthropic_endpoint_uses_bearer_auth(self):
|
||||
"""Palantir Foundry's LLM proxy requires Authorization: Bearer.
|
||||
|
||||
Regression test for PR #36043: Palantir's
|
||||
``<org>.palantirfoundry.com/api/v2/llm/proxy/anthropic`` endpoint
|
||||
rejects x-api-key with 401 — the SDK must be built with auth_token.
|
||||
"""
|
||||
with patch("agent.anthropic_adapter._anthropic_sdk") as mock_sdk:
|
||||
build_anthropic_client(
|
||||
"foundry-secret-123",
|
||||
base_url="https://acme.palantirfoundry.com/api/v2/llm/proxy/anthropic",
|
||||
)
|
||||
kwargs = mock_sdk.Anthropic.call_args[1]
|
||||
assert kwargs["auth_token"] == "foundry-secret-123"
|
||||
assert "api_key" not in kwargs
|
||||
|
||||
def test_palantir_bearer_auth_matches_hostname_not_substring(self):
|
||||
"""The palantirfoundry check must be a hostname match, not a loose
|
||||
substring match — a URL merely *containing* the string (path segment,
|
||||
lookalike domain) must not trigger Bearer auth."""
|
||||
from agent.anthropic_adapter import _requires_bearer_auth
|
||||
|
||||
# Real Foundry hosts (org subdomains) → Bearer.
|
||||
assert _requires_bearer_auth(
|
||||
"https://acme.palantirfoundry.com/api/v2/llm/proxy/anthropic"
|
||||
) is True
|
||||
assert _requires_bearer_auth("https://palantirfoundry.com/anthropic") is True
|
||||
# Substring false-positives → x-api-key (default).
|
||||
assert _requires_bearer_auth(
|
||||
"https://evil.example.com/palantirfoundry/anthropic"
|
||||
) is False
|
||||
assert _requires_bearer_auth(
|
||||
"https://palantirfoundry.com.evil.example/anthropic"
|
||||
) is False
|
||||
assert _requires_bearer_auth(
|
||||
"https://notpalantirfoundry.com/anthropic"
|
||||
) is False
|
||||
|
||||
def test_disables_sdk_retries_for_api_key(self):
|
||||
"""#26293: the SDK's default max_retries=2 ignores Retry-After and
|
||||
double-retries inside hermes's outer loop. We delegate retry entirely
|
||||
|
|
|
|||
|
|
@ -224,3 +224,72 @@ class TestResolveAutoCustomEndToEnd:
|
|||
assert base and base.rstrip("/") == "https://withcfg.example/v1"
|
||||
finally:
|
||||
mod.clear_runtime_main()
|
||||
|
||||
def test_named_custom_anthropic_messages_keeps_full_name_and_url(
|
||||
self, tmp_path, monkeypatch):
|
||||
"""PR #36043: a ``custom:<name>`` main provider whose config entry
|
||||
declares ``api_mode: anthropic_messages`` must reach the
|
||||
named-custom-provider arm of resolve_provider_client — NOT the
|
||||
anonymous-custom arm, whose ``_to_openai_base_url`` rewrite strips a
|
||||
trailing ``/anthropic`` into ``/v1`` and 404s against proxies like
|
||||
Palantir Foundry's Anthropic surface. The resulting client must be an
|
||||
AnthropicAuxiliaryClient pointed at the ORIGINAL /anthropic URL."""
|
||||
import agent.auxiliary_client as mod
|
||||
|
||||
for var in ("OPENROUTER_API_KEY", "NOUS_API_KEY", "OPENAI_API_KEY",
|
||||
"OPENAI_BASE_URL"):
|
||||
monkeypatch.delenv(var, raising=False)
|
||||
hermes_home = tmp_path / ".hermes"
|
||||
hermes_home.mkdir()
|
||||
proxy_base = "https://acme.palantirfoundry.com/api/v2/llm/proxy/anthropic"
|
||||
(hermes_home / "config.yaml").write_text(
|
||||
"model:\n"
|
||||
" default: claude-4-6-opus\n"
|
||||
" provider: 'custom:palantir'\n"
|
||||
" base_url: ''\n"
|
||||
"custom_providers:\n"
|
||||
" - name: palantir\n"
|
||||
f" base_url: '{proxy_base}'\n"
|
||||
" model: claude-4-6-opus\n"
|
||||
" api_key: foundry-token\n"
|
||||
" api_mode: anthropic_messages\n"
|
||||
)
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
mod.clear_runtime_main()
|
||||
try:
|
||||
# The live runtime carries the same base_url the main agent uses —
|
||||
# the regression collapsed the provider to bare "custom" whenever a
|
||||
# runtime base_url was present, which routed here through the
|
||||
# OpenAI-wire /anthropic→/v1 rewrite.
|
||||
mod.set_runtime_main(
|
||||
"custom:palantir",
|
||||
"claude-4-6-opus",
|
||||
base_url=proxy_base,
|
||||
api_key="foundry-token",
|
||||
api_mode="anthropic_messages",
|
||||
)
|
||||
client, resolved = mod.resolve_provider_client("auto", None)
|
||||
assert client is not None, (
|
||||
"custom:<name> with anthropic_messages entry resolved to None"
|
||||
)
|
||||
assert resolved == "claude-4-6-opus"
|
||||
assert client.__class__.__name__ == "AnthropicAuxiliaryClient", (
|
||||
f"expected AnthropicAuxiliaryClient, got {client.__class__.__name__}"
|
||||
" — the custom:<name> main provider was collapsed to the"
|
||||
" anonymous-custom OpenAI-wire arm (PR #36043 regression)"
|
||||
)
|
||||
# The original /anthropic URL must survive — no /v1 rewrite.
|
||||
assert getattr(client, "base_url", "").rstrip("/") == proxy_base
|
||||
|
||||
# Wiring check: _resolve_auto must hand the FULL custom:<name>
|
||||
# string to resolve_provider_client, with no explicit_base_url
|
||||
# override (the named arm reads base_url/api_key from config).
|
||||
with patch.object(mod, "resolve_provider_client") as mock_resolve:
|
||||
mock_resolve.return_value = (MagicMock(), "claude-4-6-opus")
|
||||
mod._resolve_auto(main_runtime=None)
|
||||
mock_resolve.assert_called_once()
|
||||
assert mock_resolve.call_args.args[0] == "custom:palantir"
|
||||
assert mock_resolve.call_args.kwargs["explicit_base_url"] is None
|
||||
finally:
|
||||
mod.clear_runtime_main()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue