mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
Nicolò Boschi
5850b9c2f7
``call`` returned either the bare response or a ``(response, TokenUsage)`` tuple, chosen by a ``return_usage: bool`` argument. A boolean that changes the return type cannot be typed, which is why all fourteen implementations were annotated ``-> Any`` and neither shape was ever checked: callers that wanted the cost unpacked two names, callers that did not got the response directly, and nothing at the call site said which you were looking at. It now always returns ``LLMCallResult(content, usage)``, mirroring ``call_with_tools``, which has always returned a named ``LLMToolCallResult``. Usage is available unconditionally, so recording cost no longer means changing a call's return type to get it. Not a public break: ``LLMInterface`` is not an extension seam — extensions plug in at memory-defense, operation-validator, tenant, bank-tables, http and mcp. ``MemoryEngine.retain_batch_async`` keeps its own ``return_usage`` flag; that one is on the engine API extensions call, and is left for a deprecation cycle. Two live bugs this surfaced, both in consolidation: * ``_consolidate_batch_with_llm`` annotated the awaited value as ``_ConsolidationBatchResponse`` and read ``response.creates`` off it. * ``_maybe_dedup_observation`` passed the awaited value straight into ``_dedup_decision_from_response``. Neither is a tuple unpack, so a sweep for unpack sites could not see them, and both failed quietly: the error was caught, retried three times, and logged as "skipping batch (the caller will bisect it)". ``sanitize_llm_value`` needed no change but did need re-reading: it special-cased tuples *because* of ``return_usage`` (the #3729 UTF-8 boundary). It already descends into ``BaseModel``, so ``LLMCallResult.content`` stays covered — the two comments describing the old shape are updated rather than left to mislead. Also removes ``cached_prefix`` / ``cached_prefix_message_count`` from ``openai_responses`` and ``github_copilot``: both declared them, neither read them, and neither overrides any prompt-caching method, so ``get_or_create_cached_prefix`` returns None for them and a caller could never pass one. Dead surface that read like a supported feature; removing it also makes them consistent with the ten providers that never declared it. Test doubles are the other half of this change, and the reason for the guard. A double that returns the payload where production now expects the envelope is invisible to a caller-side sweep and to the type checker — it only shows up as an AttributeError at runtime, or, worse, as a pipeline that never reaches the failure a test was waiting for. ``tests/test_named_result_stubs.py`` therefore asserts over the whole suite rather than any one test, and recognises every way a double is actually installed here: * ``monkeypatch.setattr(module, "call", fn)`` * a method named after the function (``FakeGraphRetriever.retrieve``) * ``AsyncMock(return_value=...)`` and ``AsyncMock(side_effect=fn)`` * ``patch("dotted.path", new=fn)`` and ``patch.object(cls, "call", side_effect=fn)`` * ``<mock>.call.return_value = payload`` and requires the double to plausibly produce the named type — a constructor call, an await, or a mock factory — rather than merely "not a tuple". That last change alone found nine stale stubs across seven files. It also checks the other two directions: nothing may unpack a named result positionally, and nothing may read a non-``content``/``usage`` attribute off an awaited ``call``.
115 lines
4.2 KiB
Python
115 lines
4.2 KiB
Python
"""Regression tests for providers (e.g. OpenRouter) that return null content.
|
|
|
|
Some OpenRouter free-tier models (e.g. nvidia/nemotron-3-super-120b-a12b:free,
|
|
openai/gpt-oss-120b:free) occasionally respond with
|
|
``response.choices[0].message.content == None`` despite a valid finish_reason.
|
|
Without a guard, downstream string operations such as ``_strip_code_fences``
|
|
crash with ``TypeError: 'NoneType' object is not subscriptable``, and every
|
|
retry hits the same unhandled error so the entire retry budget is wasted.
|
|
|
|
See https://github.com/vectorize-io/hindsight/issues/1334.
|
|
"""
|
|
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
from pydantic import BaseModel
|
|
|
|
from hindsight_api.engine.providers.openai_compatible_llm import OpenAICompatibleLLM, ProviderResponseError
|
|
|
|
|
|
class _Response(BaseModel):
|
|
answer: str
|
|
|
|
|
|
def _make_llm() -> OpenAICompatibleLLM:
|
|
return OpenAICompatibleLLM(
|
|
provider="openrouter",
|
|
api_key="sk-test",
|
|
base_url="",
|
|
model="nvidia/nemotron-3-super-120b-a12b:free",
|
|
)
|
|
|
|
|
|
def _make_chat_response(content: str | None) -> MagicMock:
|
|
"""Build a mock that matches the shape expected by _first_choice_or_error.
|
|
|
|
Key fields that must be explicitly set (not left as auto-MagicMock):
|
|
- response.error = None (otherwise truthy MagicMock triggers error path)
|
|
- response.model_dump() (returns dict without 'error' key)
|
|
- choice.message.tool_calls/refusal (otherwise truthy MagicMock in error msg)
|
|
- usage.completion_tokens_details (otherwise reasoning-token math crashes, #2378)
|
|
- usage.cached_tokens / usage.prompt_tokens_details (otherwise the cached-
|
|
token extraction reads an auto-MagicMock and metrics' `cached_input_tokens
|
|
> 0` raises "'>' not supported between MagicMock and int" — only when the
|
|
metrics path runs, which makes it an intermittent xdist failure)
|
|
"""
|
|
choice = MagicMock()
|
|
choice.finish_reason = "stop"
|
|
choice.message.content = content
|
|
choice.message.tool_calls = None
|
|
choice.message.refusal = None
|
|
|
|
response = MagicMock()
|
|
response.error = None
|
|
response.model_dump.return_value = {}
|
|
response.usage.prompt_tokens = 10
|
|
response.usage.completion_tokens = 0 if content is None else 5
|
|
response.usage.total_tokens = 10 if content is None else 15
|
|
response.usage.completion_tokens_details = None
|
|
# Cover both cached-token extraction paths (response_usage.cached_tokens and
|
|
# usage.prompt_tokens_details.cached_tokens) so neither leaks a MagicMock.
|
|
response.usage.cached_tokens = 0
|
|
response.usage.prompt_tokens_details = None
|
|
response.choices = [choice]
|
|
return response
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_null_content_raises_after_retries_exhausted():
|
|
"""All retries return null content -> ProviderResponseError, not TypeError."""
|
|
llm = _make_llm()
|
|
|
|
with patch.object(llm._client.chat.completions, "create", new_callable=AsyncMock) as mock_create:
|
|
mock_create.return_value = _make_chat_response(None)
|
|
|
|
with pytest.raises(ProviderResponseError, match="empty message content"):
|
|
await llm.call(
|
|
messages=[{"role": "user", "content": "extract facts"}],
|
|
response_format=_Response,
|
|
max_retries=2,
|
|
initial_backoff=0.0,
|
|
max_backoff=0.0,
|
|
)
|
|
|
|
# 3 attempts = max_retries (2) + 1 initial
|
|
assert mock_create.call_count == 3
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_null_content_recovers_on_retry():
|
|
"""Provider returns null on first call, valid JSON on second -> request succeeds."""
|
|
llm = _make_llm()
|
|
|
|
responses = [
|
|
_make_chat_response(None),
|
|
_make_chat_response('{"answer": "ok"}'),
|
|
]
|
|
|
|
with patch.object(llm._client.chat.completions, "create", new_callable=AsyncMock) as mock_create:
|
|
mock_create.side_effect = responses
|
|
|
|
result = (
|
|
await llm.call(
|
|
messages=[{"role": "user", "content": "extract facts"}],
|
|
response_format=_Response,
|
|
max_retries=2,
|
|
initial_backoff=0.0,
|
|
max_backoff=0.0,
|
|
)
|
|
).content
|
|
|
|
assert isinstance(result, _Response)
|
|
assert result.answer == "ok"
|
|
assert mock_create.call_count == 2
|