fix(copilot): send vision header for Copilot vision requests
Thread a vision-request flag through auxiliary provider resolution so Copilot clients can include Copilot-Vision-Request only for vision tasks. This preserves normal text requests while ensuring Copilot vision payloads reach the vision-capable route. Add regression coverage for Copilot vision routing and keep cached text and vision clients separate so a text client without the header is not reused for vision. Co-authored-by: dhabibi <9087935+dhabibi@users.noreply.github.com>
This commit is contained in:
committed by
Teknium
parent
512c610058
commit
8402ba150e
@@ -199,6 +199,7 @@ class TestResolveVisionMainFirst:
|
||||
mock_resolve.assert_called_once()
|
||||
assert mock_resolve.call_args.args[0] == "openrouter"
|
||||
assert mock_resolve.call_args.args[1] == "anthropic/claude-sonnet-4.6"
|
||||
assert mock_resolve.call_args.kwargs.get("is_vision") is True
|
||||
|
||||
def test_nous_main_vision_uses_paid_nous_vision_backend(self):
|
||||
"""Paid Nous main → aux vision uses the dedicated Nous vision backend."""
|
||||
@@ -266,6 +267,87 @@ class TestResolveVisionMainFirst:
|
||||
assert provider == "xiaomi"
|
||||
# Should use mimo-v2.5 (vision override), not mimo-v2-pro (text main)
|
||||
assert mock_resolve.call_args.args[1] == "mimo-v2.5"
|
||||
assert mock_resolve.call_args.kwargs.get("is_vision") is True
|
||||
|
||||
def test_copilot_vision_sets_vision_header(self, monkeypatch):
|
||||
"""Copilot vision requests include the header required for vision routing."""
|
||||
monkeypatch.setenv("COPILOT_GITHUB_TOKEN", "ghu_test-token")
|
||||
|
||||
captured = {}
|
||||
|
||||
def fake_headers(*, is_agent_turn=False, is_vision=False):
|
||||
captured["is_agent_turn"] = is_agent_turn
|
||||
captured["is_vision"] = is_vision
|
||||
return {"Copilot-Vision-Request": "true"} if is_vision else {}
|
||||
|
||||
with patch(
|
||||
"agent.auxiliary_client._read_main_provider", return_value="copilot",
|
||||
), patch(
|
||||
"agent.auxiliary_client._read_main_model", return_value="configured-copilot-model",
|
||||
), patch(
|
||||
"agent.auxiliary_client._resolve_task_provider_model",
|
||||
return_value=("auto", None, None, None, None),
|
||||
), patch(
|
||||
"agent.auxiliary_client.OpenAI",
|
||||
) as mock_openai, patch(
|
||||
"hermes_cli.auth.resolve_api_key_provider_credentials",
|
||||
return_value={
|
||||
"provider": "copilot",
|
||||
"api_key": "copilot-api-token",
|
||||
"base_url": "https://api.githubcopilot.com",
|
||||
},
|
||||
), patch(
|
||||
"hermes_cli.copilot_auth.copilot_request_headers",
|
||||
side_effect=fake_headers,
|
||||
):
|
||||
mock_client = MagicMock()
|
||||
mock_openai.return_value = mock_client
|
||||
|
||||
from agent.auxiliary_client import resolve_vision_provider_client
|
||||
|
||||
provider, client, model = resolve_vision_provider_client()
|
||||
|
||||
assert provider == "copilot"
|
||||
assert client is mock_client
|
||||
assert model == "configured-copilot-model"
|
||||
assert captured == {"is_agent_turn": True, "is_vision": True}
|
||||
assert mock_openai.call_args.kwargs["default_headers"]["Copilot-Vision-Request"] == "true"
|
||||
|
||||
def test_text_copilot_does_not_set_vision_header(self, monkeypatch):
|
||||
"""Text Copilot requests keep the vision-only header off."""
|
||||
monkeypatch.setenv("COPILOT_GITHUB_TOKEN", "ghu_test-token")
|
||||
|
||||
captured = {}
|
||||
|
||||
def fake_headers(*, is_agent_turn=False, is_vision=False):
|
||||
captured["is_agent_turn"] = is_agent_turn
|
||||
captured["is_vision"] = is_vision
|
||||
return {"Copilot-Vision-Request": "true"} if is_vision else {}
|
||||
|
||||
with patch(
|
||||
"agent.auxiliary_client.OpenAI",
|
||||
) as mock_openai, patch(
|
||||
"hermes_cli.auth.resolve_api_key_provider_credentials",
|
||||
return_value={
|
||||
"provider": "copilot",
|
||||
"api_key": "copilot-api-token",
|
||||
"base_url": "https://api.githubcopilot.com",
|
||||
},
|
||||
), patch(
|
||||
"hermes_cli.copilot_auth.copilot_request_headers",
|
||||
side_effect=fake_headers,
|
||||
):
|
||||
mock_client = MagicMock()
|
||||
mock_openai.return_value = mock_client
|
||||
|
||||
from agent.auxiliary_client import resolve_provider_client
|
||||
|
||||
client, model = resolve_provider_client("copilot", "gpt-5-mini")
|
||||
|
||||
assert client is mock_client
|
||||
assert model == "gpt-5-mini"
|
||||
assert captured == {"is_agent_turn": True, "is_vision": False}
|
||||
assert "default_headers" not in mock_openai.call_args.kwargs
|
||||
|
||||
def test_main_unavailable_vision_falls_through_to_aggregators(self):
|
||||
"""Main provider fails → fall back to OpenRouter/Nous strict backends."""
|
||||
@@ -312,7 +394,7 @@ class TestResolveVisionMainFirst:
|
||||
|
||||
# Explicit "nous" override → uses strict backend, NOT main model path
|
||||
assert provider == "nous"
|
||||
mock_strict.assert_called_once_with("nous")
|
||||
mock_strict.assert_called_once_with("nous", None)
|
||||
|
||||
|
||||
# ── Constant cleanup ────────────────────────────────────────────────────────
|
||||
|
||||
@@ -103,7 +103,7 @@ class TestCleanupStaleAsyncClients:
|
||||
mock_client._client = MagicMock()
|
||||
mock_client._client.is_closed = False
|
||||
|
||||
key = ("test_stale", True, "", "", "", ())
|
||||
key = ("test_stale", True, "", "", "", (), False)
|
||||
with _client_cache_lock:
|
||||
_client_cache[key] = (mock_client, "test-model", loop)
|
||||
|
||||
@@ -127,7 +127,7 @@ class TestCleanupStaleAsyncClients:
|
||||
loop = asyncio.new_event_loop() # NOT closed
|
||||
|
||||
mock_client = MagicMock()
|
||||
key = ("test_live", True, "", "", "", ())
|
||||
key = ("test_live", True, "", "", "", (), False)
|
||||
with _client_cache_lock:
|
||||
_client_cache[key] = (mock_client, "test-model", loop)
|
||||
|
||||
@@ -149,7 +149,7 @@ class TestCleanupStaleAsyncClients:
|
||||
)
|
||||
|
||||
mock_client = MagicMock()
|
||||
key = ("test_sync", False, "", "", "", ())
|
||||
key = ("test_sync", False, "", "", "", (), False)
|
||||
with _client_cache_lock:
|
||||
_client_cache[key] = (mock_client, "test-model", None)
|
||||
|
||||
@@ -182,7 +182,7 @@ class TestClientCacheBoundedGrowth:
|
||||
_get_cached_client,
|
||||
)
|
||||
|
||||
key = ("test_replace", True, "", "", "", ())
|
||||
key = ("test_replace", True, "", "", "", (), False)
|
||||
|
||||
# Simulate a stale entry from a closed loop
|
||||
old_loop = asyncio.new_event_loop()
|
||||
@@ -217,7 +217,7 @@ class TestClientCacheBoundedGrowth:
|
||||
_client_cache_lock,
|
||||
)
|
||||
|
||||
key = ("test_no_grow", True, "", "", "", ())
|
||||
key = ("test_no_grow", True, "", "", "", (), False)
|
||||
|
||||
loops = []
|
||||
try:
|
||||
@@ -269,7 +269,7 @@ class TestClientCacheBoundedGrowth:
|
||||
mock_client = MagicMock()
|
||||
mock_client._client = MagicMock()
|
||||
mock_client._client.is_closed = False
|
||||
key = (f"evict_test_{i}", False, "", "", "", ())
|
||||
key = (f"evict_test_{i}", False, "", "", "", (), False)
|
||||
with _client_cache_lock:
|
||||
# Inline the eviction logic (same as _get_cached_client)
|
||||
while len(_client_cache) >= _CLIENT_CACHE_MAX_SIZE:
|
||||
@@ -281,9 +281,9 @@ class TestClientCacheBoundedGrowth:
|
||||
assert len(_client_cache) <= _CLIENT_CACHE_MAX_SIZE, \
|
||||
f"Cache size {len(_client_cache)} exceeds max {_CLIENT_CACHE_MAX_SIZE}"
|
||||
# The earliest entries should have been evicted
|
||||
assert ("evict_test_0", False, "", "", "", ()) not in _client_cache
|
||||
assert ("evict_test_0", False, "", "", "", (), False) not in _client_cache
|
||||
# The latest entries should be present
|
||||
assert (f"evict_test_{_CLIENT_CACHE_MAX_SIZE + 4}", False, "", "", "", ()) in _client_cache
|
||||
assert (f"evict_test_{_CLIENT_CACHE_MAX_SIZE + 4}", False, "", "", "", (), False) in _client_cache
|
||||
finally:
|
||||
with _client_cache_lock:
|
||||
_client_cache.clear()
|
||||
|
||||
Reference in New Issue
Block a user