mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
6e206b191b
* refactor(api-slim): replace bare tuple contracts with named result types The project standard says "never use multi-item tuple return values -- not even for internal/private functions", but a diff-based review can only enforce it on new code. This sweeps the pre-existing violations where a positional contract was actually load-bearing, and adds a structural guard so they cannot come back. Six named types, chosen by how a mistake would surface rather than by count: RetainBatchResult retain pipeline (memory_ids, usage, processed_tokens) ExtractionResult fact extraction (facts, chunks, usage) TagClause tag SQL (sql, params, next_param_offset) GraphRetrieval graph retrieval (results, timings) FittedDeltaPrompt delta prompt fitting (doc, candidate, facts, truncated) ValidatedOperations delta ops (valid, skipped) PreparedFactEntities entity prep (fact_texts, fact_dates, entities_per_fact) Three defects this surfaced: * `_streaming_retain_batch` was annotated `tuple[list[list[str]], TokenUsage]` while returning three values, and `retain_batch` handed that straight back as its own 3-tuple. Nothing caught it: tuple arity is checked nowhere and `ty` has `invalid-return-type` disabled. It was harmless only because every caller happened to unpack three. * `_fit_structured_delta_prompt_parts` returns three `str` whose meaning is caller-dependent -- the refresh prompt puts the synthesis in slot 2 and new facts in slot 3, the retraction prompt puts still-supported facts in slot 2 and retracted ones in slot 3. Transposing those two type-checks and builds a grammatical prompt telling the model to strip content resting on still-valid facts and keep content resting on retracted ones. Now asserted directly by test_retraction_prompt_does_not_transpose_surviving_and_retracted, since no type can express it. * `_merge_processed_content_tokens` (the "None is contagious, bill full content" rule) was re-derived inline twice in memory_engine.py without the docstring explaining why `None + int` is a semantic and not a bug. Moved beside the field it governs; its docstring also cited `RetainResult.processed_content_tokens`, a field neither RetainResult has. Also deduplicates the memory_units link-expansion projection, which was spelled out ~20 times across both DB backends -- once per UNION ALL arm, again in the PG semantic arm's GROUP BY, again in each dialect's outer re-projection. Adding a column was a 20-edit change where a dropped column breaks union arity and a reordered one corrupts results silently, since the arms are read positionally. Behaviour-preserving, and checked rather than asserted: * 18/18 generated link-expansion SQL strings byte-identical (2 dialects x 3 builders x 3 window variants). * 930 tag-builder invocations and 380 Python-side filter evaluations identical to main, across every match mode, tag set, alias, offset and nesting shape. * 288 delta/retraction prompts identical to main, including at caps that force truncation. * Guard tests enumerate each family from source and assert no member returns a bare tuple; all are mutation-checked by reintroducing the original defect. Deliberately left alone: MemoryEngineInterface and mcp_tools dict returns (extension and MCP wire contracts), memories/base.graph_links_and_entities (an overridable store method), and tuples that are genuinely tuple-shaped -- a dict cache key and a sort key. Claude-Session: https://claude.ai/code/session_01UBcDzagMhuXsYZDp63i7aB * refactor(api-slim): write each duplicated policy once Four duplications found by auditing api-slim for logic that exists in more than one place. One of them was already producing malformed output; the other three are the conditions that let it happen unnoticed. Fixes a real bug: 35 MCP error payloads were not valid JSON The bank-id variants of the MCP tools declare `-> str` and return JSON text, and built their error branch by interpolation: return f'{{"error": "{e}"}}' which emits invalid JSON as soon as the message contains a double quote, a backslash or a newline. PostgreSQL quotes identifiers with double quotes, so an ordinary `relation "memory_units" does not exist` was already enough to hand the caller something it could not parse. 35 of the 38 sites were in the bank-id half of a duplicated tool; the session half returned a dict and was always correct -- which is exactly why no test caught it. All now go through `_error_json`, which serializes with `json.dumps`. Each MCP tool is written once instead of twice Every tool is registered twice, once taking an explicit `bank_id` and returning JSON text, once resolving the bank from the session and returning a dict. FastMCP builds each tool's schema from the literal signature and docstring, so those declarations genuinely have to exist twice -- but the logic did not, and all 36 pairs had diverging bodies. The shared flow (resolve the bank, reject a missing one, serialize, map OperationValidationError and everything else onto an error payload) is now `_run_tool`, and each registrar writes its one engine call once. 32 of 36 tools; the four exceptions are named in the guard test with their reasons -- recall/reflect use different pydantic serializers in the two copies (model_dump_json vs model_dump, which do not render datetimes identically), and retain/sync_retain resolve a bank per content item. `ValueError` handling is now explicit rather than incidental: tools treated it three different ways -- a fault, a rejected input logged beside OperationValidationError, or a normal negative answer with no log at all -- so the wrapper takes which one applies instead of averaging them. api/http.py: one error policy instead of 20 spellings 89 route handlers carried 20 distinct spellings of the same mapping: 72 byte-identical copies of the four-line catch-all, plus seven that logged the traceback without the message. All 79 now call `_internal_error`, which logs the route, the message and the traceback and returns a 500 carrying only the message. Two handlers keep their own: recall logs its handler duration, and retain maps MemoryDefenseAllBlockedError to a 422 and logs an input summary. Both are named in the guard test rather than pattern-matched. Providers: a capability that was half-declared openai_responses and github_copilot declared `cached_prefix` and `cached_prefix_message_count` on call/call_with_tools but never read them, and override none of the prompt-caching methods -- so `get_or_create_cached_prefix` returns None for them and callers can never pass one. Dead surface that read like a supported feature; removed, which also makes them consistent with the ten other providers that never declared it. Verification Behaviour is checked, not assumed. A harness registers every tool in both bank modes against a mock engine across three scenarios (success, not-found, and an exception whose message carries every JSON metacharacter) and captures each tool's parameter schema, output schema, description and returned payload: 468/468 entries identical before and after, including the four that caught a real regression mid-refactor -- routing recall and reflect through the shared wrapper changed their serializer, so they were reverted. New guard tests assert over each family rather than any one member, since the member that forgets is by construction the one without a test: that no tool builds error JSON by interpolation, that both copies of a tool declare the same parameters and share one implementation, that a provider declares a capability's parameters only if it implements the capability, and that no handler builds a 500 inline. Every one is mutation-checked by reintroducing the defect it describes.
136 lines
5.1 KiB
Python
136 lines
5.1 KiB
Python
"""Live integration test for the Gemini Batch API provider.
|
|
|
|
This makes REAL calls to the Gemini Batch API and runs the full retain fact
|
|
extraction pipeline end-to-end (translate -> upload JSONL -> batches.create ->
|
|
poll -> download -> normalize -> parse facts). It is the only test that
|
|
validates the one assumption the unit tests (which use a fake genai client)
|
|
cannot: that Gemini's real batch output JSONL shape matches what
|
|
``_normalize_output_line`` produces and what ``fact_extraction`` consumes. If
|
|
the shape is wrong, this returns zero facts.
|
|
|
|
This is an explicit opt-in test. It is gated on a dedicated flag rather than
|
|
just "a Gemini API key exists" because CI always has a Gemini key (Gemini is the
|
|
LLM-as-judge / core-LLM provider) — keying off the API key alone would let this
|
|
slow batch job run in the standard CI shard and blow the 300s pytest timeout. To
|
|
run it:
|
|
|
|
export HINDSIGHT_API_GEMINI_BATCH_LIVE_TEST=1
|
|
export GEMINI_API_KEY=... # or HINDSIGHT_API_GEMINI_API_KEY
|
|
# optional: override the model
|
|
export HINDSIGHT_API_GEMINI_TEST_MODEL=gemini-2.5-flash
|
|
uv run pytest tests/test_gemini_batch_integration.py -v -s
|
|
|
|
It is slow (typically minutes, but Gemini's batch queue can take far longer; the
|
|
SLA is up to 24h) and costs money, so it never runs in CI. No database is
|
|
required — it calls the extraction function directly with ``pool=None``.
|
|
"""
|
|
|
|
import logging
|
|
import os
|
|
from dataclasses import dataclass
|
|
from datetime import datetime, timezone
|
|
|
|
import pytest
|
|
from dotenv import load_dotenv
|
|
|
|
from hindsight_api.config import HindsightConfig, clear_config_cache
|
|
from hindsight_api.engine.llm_wrapper import LLMProvider
|
|
from hindsight_api.engine.retain.fact_extraction import (
|
|
RetainContent,
|
|
extract_facts_from_contents_batch_api,
|
|
)
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
load_dotenv()
|
|
|
|
_DEFAULT_TEST_MODEL = "gemini-2.5-flash"
|
|
|
|
|
|
@dataclass
|
|
class GeminiTestEnv:
|
|
api_key: str
|
|
model: str
|
|
|
|
|
|
@pytest.fixture
|
|
def gemini_env() -> GeminiTestEnv:
|
|
# Opt-in flag, NOT just key presence: CI always has GEMINI_API_KEY (judge /
|
|
# core-LLM provider), so gating on the key alone runs this slow batch job in
|
|
# the standard CI shard and times out. Require an explicit flag CI never sets.
|
|
if os.getenv("HINDSIGHT_API_GEMINI_BATCH_LIVE_TEST", "").lower() not in ("1", "true", "yes"):
|
|
pytest.skip("Set HINDSIGHT_API_GEMINI_BATCH_LIVE_TEST=1 (and GEMINI_API_KEY) to run the live Gemini batch test")
|
|
|
|
api_key = os.getenv("GEMINI_API_KEY") or os.getenv("HINDSIGHT_API_GEMINI_API_KEY")
|
|
if not api_key:
|
|
pytest.skip("Set GEMINI_API_KEY (or HINDSIGHT_API_GEMINI_API_KEY) to run the live Gemini batch test")
|
|
|
|
clear_config_cache()
|
|
|
|
return GeminiTestEnv(
|
|
api_key=api_key,
|
|
model=os.getenv("HINDSIGHT_API_GEMINI_TEST_MODEL", _DEFAULT_TEST_MODEL),
|
|
)
|
|
|
|
|
|
@pytest.mark.integration
|
|
@pytest.mark.slow
|
|
@pytest.mark.asyncio
|
|
async def test_real_gemini_batch_end_to_end(gemini_env):
|
|
config = HindsightConfig.from_env()
|
|
config.retain_batch_enabled = True
|
|
config.retain_batch_poll_interval_seconds = 30
|
|
config.retain_chunk_size = 4000
|
|
config.retain_extraction_mode = "concise"
|
|
config.retain_extract_causal_links = False
|
|
|
|
llm_config = LLMProvider(
|
|
provider="gemini",
|
|
api_key=gemini_env.api_key,
|
|
base_url="",
|
|
model=gemini_env.model,
|
|
reasoning_effort="low",
|
|
)
|
|
assert await llm_config._provider_impl.supports_batch_api() is True
|
|
|
|
contents = [
|
|
RetainContent(
|
|
content=(
|
|
"Alice is a senior software engineer at TechCorp. She specializes in "
|
|
"distributed systems and graduated from MIT in 2015."
|
|
),
|
|
event_date=datetime(2024, 1, 15, tzinfo=timezone.utc),
|
|
context="team member profile",
|
|
)
|
|
]
|
|
|
|
logger.info("Submitting a real Gemini batch (this can take several minutes)...")
|
|
extraction = await extract_facts_from_contents_batch_api(
|
|
contents=contents,
|
|
llm_config=llm_config,
|
|
config=config,
|
|
pool=None,
|
|
operation_id=None,
|
|
schema=None,
|
|
)
|
|
facts = extraction.facts
|
|
chunks = extraction.chunks
|
|
usage = extraction.usage
|
|
|
|
# The end-to-end proof: if the real output shape doesn't match the normalizer,
|
|
# the consumer extracts nothing and this is empty.
|
|
assert len(facts) > 0, (
|
|
"Gemini batch returned no facts. The live output JSONL shape likely "
|
|
"differs from what _normalize_output_line produces — inspect a raw output "
|
|
"line and adjust the normalizer."
|
|
)
|
|
assert any("Alice" in fact.fact_text for fact in facts)
|
|
# Token usage must be threaded from Gemini's usageMetadata into the batch
|
|
# result body — otherwise the consumer reports zero (the bug this guards).
|
|
assert usage.total_tokens > 0, "Gemini batch reported zero token usage — usageMetadata translation is broken"
|
|
assert usage.input_tokens > 0 and usage.output_tokens > 0
|
|
logger.info(
|
|
f"Extracted {len(facts)} facts; usage in={usage.input_tokens} out={usage.output_tokens} "
|
|
f"total={usage.total_tokens} tokens"
|
|
)
|