Adversarial review: two implementations, and one that could ask nothing

[adamjohnwright]

Follow-up review of #272. Three findings.

Two implementations of one rule

I wrote the expansion rule twice — once for the sync path, once for the async — in a file that already carries a note about those two drifting. Worse, the test written to catch that drift structurally cannot: test_sync_async_equivalence drives retrieve_documents directly, below the expansion step.

One _queries now serves both, with a test asserting the source keeps it that way — the behaviour is only equal while the code is shared, so that is the thing worth pinning.

A configuration that asks nothing at all

include_original=False — which is from_subdirectory's default — plus zero alternates produced an empty query list, and retrieval with no queries returns nothing. A retriever that finds nothing reads exactly like a question with no answer, which is the worst available way for a recall mechanism to fail.

_queries never returns empty now: it falls back to the original question and logs that it did.

My own test was vacuous

The "falls back loudly" test asserted only the fallback value, so it would have passed against a silent fallback. That is the same shape that bit the collection guards this morning — written by me a few hours after fixing those. It asserts the warning now.

Verified beyond the unit tests

answer-sweep 13/13 after the refactor, so the shared path retrieves what the two separate ones did.

584 passed, 1 skipped; mypy over 149 files, ruff clean.

🤖 Generated with Claude Code