fix(recall): fill the source_facts budget in rank order and flag truncation (#3221) - #3419
Merged
Merged
Conversation
nicoloboschi
force-pushed
the
fix-source-facts-rank-order-3221
branch
from
August 12, 2026 07:54
2abe02f to
5b89c68
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3221.
With
include.source_factsenabled, recall returns each observation's fullsource_fact_idsbut resolves them through asource_factsmap filled up to a token budget. All three problems reported in #3221 are real; this fixes them.1. The budget was spent in DB row order, not rank order
observation_idsis built in rank order fromtop_scored, but the follow-up read discarded that order:So whichever observation Postgres happened to return first spent the budget first, and a rank-1 result could lose its provenance to a rank-90 one — differently on two banks with identical content. The sibling enrichment paths in the same function already avoid this (chunks order by
array_position(...), entities preservetop_scoredorder); source facts were the one that didn't. Both branches (SQL store and store-owned rows) now keep observation-rank order, so truncation hits the tail of the result list and never the head.2. One oversized fact evicted everything behind it
The budget loop
breaked on the first fact that didn't fit, so a single long fact dropped every shorter fact after it. It now skips just that fact and keeps filling.3. Truncation was silent
results[].source_fact_idsstill advertises every source, so a caller could not tell a budget-truncated map from a dangling reference. The response now carriessource_facts_truncated: truewhen the budget dropped something (present only when source facts were requested).source_fact_idsis left complete on purpose — callers keep the full provenance list and the flag tells them why an ID may be unresolvable.Implementation
The budget fill moved out of the 700-line recall body into
engine/source_facts.py(select_source_facts_within_budget, returning aSourceFactSelectiondataclass) so the selection rules are directly testable. Behaviour is otherwise unchanged: same dedup, same per-observation cap semantics, same defaults.Tests
tests/test_source_facts_selection.py— 9 unit tests over the selection rules: rank-order fill, oversized-fact skip (both budget modes), dedup of shared sources, unresolvable ID is not reported as truncation.tests/test_source_facts_tokens.py::TestRecallSourceFactsRankOrder— end-to-end regression: budgets the top result's sources exactly and asserts it keeps them. Verified it fails without the fix, with exactly the reported symptom:Existing source-fact, recall-enrichment and observation tests pass unchanged.
Also updated
OpenAPI + generated Go/Python/TypeScript clients, the recall API docs, and the docs skill.