fix(recall): enforce expiry/supersession in top-K recall SQL (TAP-4586)#221
Merged
Conversation
Expired (valid_until in the past) and superseded (superseded_by set) rows previously won top-K by vector distance / FTS rank and were only dropped afterward by the Python is_temporally_valid filter, wasting the top-K budget so live rows could be crowded out. Push the live-row predicate into the KNN and FTS recall SQL: superseded_by IS NULL AND (valid_until IS NULL OR valid_until = '' OR valid_until::timestamptz > now()) valid_until is TEXT NOT NULL DEFAULT '' (migration 001), so the canonical live row is the empty string — the issue's literal `valid_until > now()` would have dropped every live row; adjusted to mirror MemoryEntry.is_temporally_valid. - Predicate uses now()/IS NULL only — no bound params, so existing param tuples are unchanged and tenant/RLS predicates (project_id/agent_id) are untouched. - Thread include_expired (default False) through build_search_sql / build_knn_search_sql, the sync+async backends, MemoryStore/QueryMixin, and the PrivateBackend protocol so the include_historical recall path still surfaces stale rows for the Python filter to decide. - Suppress the predicate when as_of is set: point-in-time queries resolve validity via the bi-temporal valid_at/invalid_at window and legitimately want the superseded version. - Python is_temporally_valid gate kept as defense-in-depth (unchanged). New requires_postgres test proves stale rows with a closer vector / higher FTS rank never occupy top-K when live rows exist, the include_expired + as_of escape hatches work, and cross-project rows never leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
158e1c9 to
6d41eb9
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.
What
Push the expiry/supersession predicate into the KNN + FTS recall SQL so expired and superseded rows no longer consume the top-K budget. Previously they were dropped only after the top-K cut by the Python
is_temporally_validfilter, so stale rows could crowd out live ones under budget pressure.Live-row predicate (added to
_postgres_private_sql.py):valid_untilisTEXT NOT NULL DEFAULT ''(migration 001), so a live row is the empty string — the issue's literalvalid_until IS NULL OR valid_until > now()would have dropped every live row. Adjusted to the canonical form that mirrorsMemoryEntry.is_temporally_valid.How
now()/IS NULLonly — no bound params, so existing positional param tuples are unchanged and tenant/RLS predicates (project_id/agent_id) are untouched.include_expired: bool = Falsethroughbuild_search_sql+ newbuild_knn_search_sql, the sync + async backends,MemoryStore/QueryMixin, and thePrivateBackendprotocol so theinclude_historicalrecall path still surfaces stale rows for the Python filter to decide.as_ofis set — point-in-time queries resolve validity via the bi-temporalvalid_at/invalid_atwindow and legitimately want the superseded version. (This fixed aTestTemporalFiltering::test_search_as_of_returns_old_versionregression the naive predicate introduced.)is_temporally_validgate kept as defense-in-depth (unchanged).Test
tests/integration/test_recall_temporal_sql.py(@pytest.mark.requires_postgres, ran — not skipped, against an isolated throwaway pgvector:pg17 container):valid_until='') and future-valid_untilrows still returned.include_expired=Trueescape hatch surfaces stale rows (KNN + FTS).as_ofpoint-in-time still returns the superseded version.Results: new test 10 passed; with the temporal unit tests 13 passed; existing recall integration 25 passed (no regression); affected-module unit sweep 512 passed / 26 skipped.
ruffclean;mypy --strict src/tapps_brain/→ no issues in 152 files.Closes TAP-4586.
🤖 Generated with Claude Code