mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
test(system): make the open defects fail instead of documenting them
The suite was green while five filed issues sat unfixed, because it accommodated every one of them: comments saying "this returns a dict, see #4218" and then dict access; a skip on the import round trip; and — worst — a test asserting the *current* wrong answer for #4230, which would have failed the day someone fixed it and taught the next reader to delete it rather than read it. A comment is a code-review note, not a gate. These now assert the contract we want and fail until the product honours it: - #4217 the recall trace must report the caller's query_timestamp, not the moment the trace was built - #4218 list rows must be typed like their single-fetch siblings - #4221 every wrapper convenience method must have an async twin — written over the whole family, so the next one added without a twin also fails - #4230 a failed structured-output extraction must be distinguishable from an empty one - #4232 a bank template must round-trip through the SDK (the two skips removed) Deliberately not xfail: an expected-failure marker keeps the run green, so nothing forces the question, and it outlives the bug by months. They sit in one file because they are temporary — when an issue lands, its test moves into the story it belongs to and the file shrinks. When it is empty, delete it.
This commit is contained in:
Nicolò Boschi
@@ -85,23 +85,17 @@ async def test_no_extraction_call_happens_without_a_schema(client, llm, bank_wit
|
||||
assert llm.prompts_for("reflect_structured") == []
|
||||
|
||||
|
||||
async def test_an_extraction_that_fails_is_reported_as_nothing_extracted(client, llm, bank_with_facts):
|
||||
"""Current behaviour, pinned so a fix is visible — see #4230.
|
||||
async def test_a_failed_extraction_still_returns_the_prose_answer(client, llm, bank_with_facts):
|
||||
"""Degrading rather than failing the whole reflect is right: the answer is
|
||||
useful even when the machine-readable half is not available.
|
||||
|
||||
When the extraction call returns something unparseable the failure is
|
||||
swallowed: the caller gets 200, the prose answer, and `structured_output:
|
||||
null` with no indication that the half they asked for did not happen. That is
|
||||
indistinguishable from an answer that legitimately contained nothing to
|
||||
extract, so a caller cannot tell a retryable failure from a normal empty
|
||||
result.
|
||||
|
||||
Answering with the prose rather than failing the whole reflect is the right
|
||||
call; the missing piece is saying so. When #4230 lands, this test should need
|
||||
updating — which is the point of writing it down.
|
||||
That the failure is *reported* is a separate contract, and one the product
|
||||
does not honour yet — asserted in `test_99_open_defects.py` against #4230
|
||||
rather than pinned here, so a fix turns a red test green instead of breaking
|
||||
a green one.
|
||||
"""
|
||||
llm.on_step("reflect_structured").returns_text("absolutely not json")
|
||||
|
||||
response = await client.areflect(bank_id=bank_with_facts, query=QUERY, response_schema=SCHEMA)
|
||||
|
||||
assert response.text == ANSWER
|
||||
assert response.structured_output is None
|
||||
|
||||
@@ -79,18 +79,17 @@ async def test_a_template_leaves_untouched_settings_unset(client, configured_ban
|
||||
assert template.bank.retain_mission is None
|
||||
|
||||
|
||||
@pytest.mark.skip(reason="#4232 — import_bank_template cannot send the manifest from any SDK")
|
||||
async def test_a_template_applied_to_a_new_bank_reproduces_the_configuration(client, configured_bank, fresh_bank):
|
||||
"""The round trip that makes templates worth having: configure once, stamp
|
||||
out many.
|
||||
|
||||
Skipped rather than deleted, and skipped rather than rewritten against raw
|
||||
HTTP. The import handler reads its body off the raw request, so the spec
|
||||
declares no `requestBody` and every generated client's
|
||||
`import_bank_template` has no parameter to put the manifest in (#4232).
|
||||
Reaching around the client to make this pass would hide exactly the defect a
|
||||
suite driven through the published client exists to surface — the export
|
||||
half works and returns a typed manifest with nowhere to send it.
|
||||
Failing today, deliberately (#4232). The import handler reads its body off
|
||||
the raw request, so the spec declares no `requestBody` and every generated
|
||||
client's `import_bank_template` has no parameter to put the manifest in.
|
||||
|
||||
Not rewritten against raw HTTP: reaching around the client would hide exactly
|
||||
the defect a client-driven suite exists to surface — the export half works
|
||||
and hands you a typed manifest with nowhere to send it.
|
||||
"""
|
||||
template = await _templates(client).export_bank_template(configured_bank)
|
||||
|
||||
@@ -102,7 +101,6 @@ async def test_a_template_applied_to_a_new_bank_reproduces_the_configuration(cli
|
||||
assert config["disposition_skepticism"] == 5
|
||||
|
||||
|
||||
@pytest.mark.skip(reason="#4232 — import_bank_template cannot send the manifest from any SDK")
|
||||
async def test_a_template_carries_no_memories(client, llm, configured_bank, fresh_bank, settled):
|
||||
"""Configuration, not content. A template that dragged the source bank's
|
||||
memories along would leak one tenant's data into every bank stamped from it.
|
||||
|
||||
@@ -0,0 +1,188 @@
|
||||
"""Contracts the product does not currently honour.
|
||||
|
||||
Every test in this file **fails today**, on purpose. Each asserts the behaviour
|
||||
we want, names the issue tracking it, and goes green when that issue is fixed.
|
||||
|
||||
They live together rather than in their topical stories because they are
|
||||
temporary: when an issue lands, its test moves into the story it belongs to and
|
||||
this file shrinks. When the file is empty, delete it.
|
||||
|
||||
Two things this file is deliberately *not*:
|
||||
|
||||
- **Not `xfail`.** An expected-failure marker is a note that the suite has
|
||||
agreed to look away. It keeps the run green, so nothing forces the question,
|
||||
and the marker outlives the bug by months.
|
||||
- **Not a pin of current behaviour.** A test asserting today's wrong answer
|
||||
fails the day someone fixes it, which teaches people to delete tests instead
|
||||
of reading them.
|
||||
|
||||
So these are red, and the suite is red with them, until the product changes.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import inspect
|
||||
|
||||
import pytest
|
||||
from hindsight_client import Hindsight
|
||||
|
||||
from hindsight_system_tests.payloads import consolidation, extracted, fact
|
||||
|
||||
pytestmark = pytest.mark.asyncio
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #4217 — the recall trace reports wall-clock time, not the caller's anchor
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
async def test_the_trace_reports_the_anchor_the_query_actually_used(client, llm, bank_id, settled):
|
||||
"""A trace exists to explain a ranking, so it has to report the inputs that
|
||||
produced it.
|
||||
|
||||
`query_timestamp` moves the "now" that recency decays from, and it does reach
|
||||
the engine — story 07 asserts its effect. But `tracer.py:396` stamps the
|
||||
trace with `datetime.now(UTC)`, so someone debugging "why did my 2019 memory
|
||||
rank low when I asked as of 2020?" reads today's date and concludes their
|
||||
anchor was ignored. The field points away from the answer.
|
||||
"""
|
||||
llm.on_step("extract_facts").returns(
|
||||
extracted(fact("Alice moved to Berlin", who="Alice", entities=["Alice", "Berlin"]))
|
||||
)
|
||||
llm.on_step("consolidate").returns(consolidation())
|
||||
await client.aretain(bank_id=bank_id, content="Alice moved to Berlin.")
|
||||
await settled(bank_id)
|
||||
|
||||
response = await client.arecall(
|
||||
bank_id=bank_id, query="Where does Alice live?", query_timestamp="2020-01-01T00:00:00", trace=True
|
||||
)
|
||||
|
||||
assert response.trace["query"]["timestamp"].startswith("2020-01-01"), (
|
||||
"the trace reports when it was built, not the anchor the query ran against — #4217"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #4218 — list endpoints return untyped rows, unlike their single-fetch siblings
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
async def test_document_list_rows_are_typed_like_the_single_fetch(client, llm, bank_id, settled):
|
||||
"""`get_document` returns a model; `list_documents` returns dicts.
|
||||
|
||||
Same resource, two conventions, so a caller cannot learn one. The natural
|
||||
`listing.items[0].id` raises `AttributeError`, and a field renamed
|
||||
server-side is a compile error for one and a runtime `KeyError` for the
|
||||
other — found in production rather than at build time.
|
||||
"""
|
||||
llm.on_step("extract_facts").returns(
|
||||
extracted(fact("Alice moved to Berlin", who="Alice", entities=["Alice", "Berlin"]))
|
||||
)
|
||||
llm.on_step("consolidate").returns(consolidation())
|
||||
await client.aretain(bank_id=bank_id, content="Alice moved to Berlin.", document_id="d1")
|
||||
await settled(bank_id)
|
||||
|
||||
listing = await client.documents.list_documents(bank_id)
|
||||
|
||||
assert listing.items[0].id == "d1", "list rows are untyped dicts — #4218"
|
||||
|
||||
|
||||
async def test_memory_list_rows_are_typed(client, llm, bank_id, settled):
|
||||
"""The same defect on the endpoint most callers touch first."""
|
||||
llm.on_step("extract_facts").returns(
|
||||
extracted(fact("Alice moved to Berlin", who="Alice", entities=["Alice", "Berlin"]))
|
||||
)
|
||||
llm.on_step("consolidate").returns(consolidation())
|
||||
await client.aretain(bank_id=bank_id, content="Alice moved to Berlin.")
|
||||
await settled(bank_id)
|
||||
|
||||
memories = await client.memory.list_memories(bank_id, limit=10)
|
||||
|
||||
assert memories.items[0].text, "memory list rows are untyped dicts — #4218"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #4221 — most wrapper methods have no async variant, despite the docstring
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# `close`/`aclose` manage the client's own connection pool rather than calling
|
||||
# the API. Both halves are excluded, not just `close` — excluding one orphans the
|
||||
# other, which then reads as a method missing *its* async twin.
|
||||
_NOT_API_CALLS = {"close", "aclose"}
|
||||
|
||||
|
||||
def _convenience_methods() -> list[str]:
|
||||
"""Public methods on the wrapper that are not already the async half of a pair."""
|
||||
names = [
|
||||
name
|
||||
for name, _ in inspect.getmembers(Hindsight, callable)
|
||||
if not name.startswith("_") and name not in _NOT_API_CALLS
|
||||
]
|
||||
async_variants = {name for name in names if name.startswith("a") and name[1:] in names}
|
||||
return sorted(name for name in names if name not in async_variants)
|
||||
|
||||
|
||||
async def test_every_convenience_method_has_an_async_variant():
|
||||
"""The wrapper's own docstring says so:
|
||||
|
||||
"Every convenience method has an async counterpart prefixed with `a`
|
||||
... **Prefer the async variants** whenever you are inside an async
|
||||
context (`async def`, event loops, frameworks like
|
||||
FastAPI/LangGraph/CrewAI)."
|
||||
|
||||
27 do not. And they are not merely inconvenient there — they route through
|
||||
`loop.run_until_complete`, which raises inside a running loop. So from
|
||||
exactly the frameworks that sentence names, mental models, knowledge pages,
|
||||
directives and bank config are unreachable through the wrapper.
|
||||
|
||||
Written over the whole family rather than as a list of the 27, so the *next*
|
||||
method added without a twin fails here too — the structural-guard shape from
|
||||
the code-review skill (§9a).
|
||||
"""
|
||||
missing = [name for name in _convenience_methods() if not hasattr(Hindsight, f"a{name}")]
|
||||
|
||||
assert missing == [], f"{len(missing)} convenience methods have no async variant — #4221: {missing}"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #4230 — a failed structured-output extraction is silent
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
SCHEMA = {
|
||||
"type": "object",
|
||||
"properties": {"city": {"type": "string"}},
|
||||
"required": ["city"],
|
||||
"additionalProperties": False,
|
||||
}
|
||||
|
||||
|
||||
async def test_a_failed_structured_extraction_is_distinguishable_from_an_empty_one(client, llm, bank_id, settled):
|
||||
"""`structured_output: null` currently means three different things.
|
||||
|
||||
The extraction call errored, or returned unparseable text, or the answer
|
||||
genuinely had nothing matching the schema. The first two are retryable and
|
||||
worth alerting on; the third is normal. Collapsing them means a caller either
|
||||
retries every empty result or none, and an operator watching for "structured
|
||||
output stopped working" has no signal — the request succeeded.
|
||||
|
||||
Returning the prose answer rather than failing the whole reflect is right.
|
||||
Saying nothing about the half that did not happen is not.
|
||||
"""
|
||||
from hindsight_system_tests import reflect_loop
|
||||
|
||||
llm.on_step("extract_facts").returns(
|
||||
extracted(fact("Alice moved to Berlin", who="Alice", entities=["Alice", "Berlin"]))
|
||||
)
|
||||
llm.on_step("consolidate").returns(consolidation())
|
||||
await client.aretain(bank_id=bank_id, content="Alice moved to Berlin.")
|
||||
await settled(bank_id)
|
||||
|
||||
reflect_loop(llm, answer="Alice lives in Berlin.")
|
||||
llm.on_step("reflect_structured").returns_text("absolutely not json")
|
||||
|
||||
response = await client.areflect(bank_id=bank_id, query="Where does Alice live?", response_schema=SCHEMA)
|
||||
|
||||
assert response.structured_output is None
|
||||
assert getattr(response, "structured_output_error", None), (
|
||||
"a failed extraction is reported exactly like an empty one — #4230"
|
||||
)
|
||||
Reference in New Issue
Block a user