feat(recall): let callers supply the temporal window instead of parsing it - #3678
Conversation
…ng it
Recall derives the temporal arm's window by parsing dates out of the query
text. A caller that already knows the range it means — a date picker, an agent
that resolved "last quarter" itself — had no way to say so, and had to phrase
it in English and hope dateparser agreed.
Add `temporal_window: {start, end}` to RecallRequest. When set it is used
verbatim and the extraction is skipped entirely, which is also the point: that
work is pure CPU serialised through a single worker and costs up to ~1.3s on
document-sized query text, which is exactly what consolidation and reflect
recall with.
Naming and wording carry weight here, because the obvious reading of a date
range on a search API is "restrict results to this period" and that is not
what this does. The temporal arm is one of four retrieval arms: it surfaces
memories whose own dates fall in the window so fusion ranks them higher, and
the other three arms are untouched, so memories outside the window are still
returned. Every description — model docstring, OpenAPI field, MCP tool, both
wrappers, control plane, docs — says so explicitly.
It does not override `enable_temporal_retrieval`. That per-bank flag gates the
arm itself and stays the single switch for it, so a supplied window cannot
re-enable an arm a bank turned off.
Bounds are inclusive and naive datetimes are read as UTC at parse time, so
both ends are unambiguous before they reach a query; a reversed window is
rejected at the boundary rather than silently returning nothing.
The Rust struct literals in the CLI and the client's doctest have to name the
new field or progenitor's generated RecallRequest stops compiling.
r266-tech
left a comment
r266-tech
left a comment
There was a problem hiding this comment.
The public MemoryEngineInterface.recall_async contract in hindsight-api-slim/hindsight_api/engine/interface.py still lacks the new temporal_window parameter, while concrete MemoryEngine.recall_async accepts and forwards it. Extensions obtained through ExtensionContext are typed against this interface, so extension callers cannot use the explicit-window feature through the documented API without a type error, and third-party implementations are left out of contract parity.
Please add temporal_window with the matching type and default to MemoryEngineInterface.recall_async, document its semantics, and add an extension-facing type check or test that passes the field through the interface.
This comment targets PR #3678 at head 6e603e1091eac5b2a4fc72230f0db0881ed0b787, matching snapshot snapshot_6b4649b0b24142557b937a8c1f73cc5cc18ed37ea6fa059aba71335013dec64f. The head-binding check passed. git diff --check exited 0. The requested focused tests were not authority-approved and were not independently executed in this inspection.
|
The red checks split into one diff-owned failure and two unrelated failures:
This is for head |
The recall UI could not reach the window it now proxies. Adds a Time window row to the Recall Analyzer: two datetime inputs, a Clear button, and a hint that states what the window actually does — ranks memories dated in the range higher, does not hide the ones outside it — since "date range on a search form" reads as a filter otherwise. The two rules live in lib/temporal-window.ts rather than the component so they are testable: a window needs both ends (one alone is an incomplete range, not a half-open filter), and a reversed range is withheld and flagged inline with the Recall button disabled, instead of being sent for the API to reject with a 422. Comparing the raw `datetime-local` strings is exact — they are already YYYY-MM-DDTHH:mm, which sorts chronologically — so there is no Date parsing and no local-timezone reinterpretation between the input and the request. The value is sent with no offset, which the API reads as UTC, matching what query_timestamp already does from this same form; the hint says so. Strings added to all ten locales.
…e button Disabling the Recall button left the Enter-key handler on the query input calling runSearch() directly. With a reversed range that ran the search anyway and silently dropped the window, which is the failure the inline warning exists to prevent. Guard in runSearch so every entry point agrees, and toast the same message rather than doing nothing visible.
…ow-end check-cli-coverage caught that recall_memories gained a request-body field the CLI neither exposes nor exempts. The exemption list is for genuinely complex nested bodies — min_scores' four calibrated floats, tag_groups' boolean tree — and two datetimes is not that, so expose it rather than write it off. Flattened into two flags and recorded as such in the coverage manifest, the same shape `include` already uses. Both ends are required: one alone is an incomplete range, not a half-open filter, and running a recall without the window the caller asked for is worse than refusing. A reversed range is rejected before the request rather than sent for the API to 422, and a datetime with no offset is read as UTC, which is how the API reads one.
|
Confirmed the CLI coverage gap from my earlier triage is fixed at |
3 participants
Problem
Recall works out the temporal arm's window by parsing dates out of the query text. A caller that already knows the range it means — a date picker, an agent that resolved "last quarter" itself, a scheduled job over a fixed period — had no way to say so. It had to phrase the range in English and hope the parser agreed.
Fix
temporal_window: {start, end}onRecallRequest. When set it is used verbatim and the extraction is skipped entirely.{ "query": "what did we decide about pricing", "temporal_window": { "start": "2023-04-01T00:00:00Z", "end": "2023-06-30T23:59:59Z" } }Skipping the extraction is half the point. It is pure CPU deliberately serialised through a single worker, and costs up to ~1.3s on document-sized query text — which is exactly what consolidation and reflect recall with.
It ranks, it does not filter
The obvious reading of a date range on a search API is "restrict results to this period", and that is not what this does. The temporal arm is one of four retrieval arms: it surfaces memories whose own dates (
mentioned_at/occurred_start/occurred_end) fall in the window so fusion ranks them higher. The semantic, keyword and graph arms are untouched, so memories dated outside the window are still returned.Since that is the thing a caller is most likely to get wrong, every description says so explicitly and in the same terms — the model docstring, the OpenAPI field, both MCP recall tools, both wrapper clients, the control-plane type, and the docs page. Restricting results to a period would be a different feature, and is not in this PR.
Two smaller semantics, both settled at parse time rather than deep in a query:
Interaction with
enable_temporal_retrievalA supplied window does not override it. That per-bank flag gates the arm itself and stays the single switch for it, so a caller cannot re-enable an arm a bank deliberately turned off. Covered by a test.
query_timestampis unaffected — it still anchors recency scoring, so it remains useful alongside a window.Control plane
The Recall Analyzer gets a Time window row — two datetime inputs, a Clear button, and a hint that states what the window actually does, since "date range on a search form" reads as a filter otherwise.
The two rules live in
lib/temporal-window.tsrather than the component so they are testable: a window needs both ends (one alone is an incomplete range, not a half-open filter), and a reversed range is withheld and flagged inline with the Recall button disabled, rather than sent for the API to 422.Comparing the raw
datetime-localstrings is exact — they are alreadyYYYY-MM-DDTHH:mm, which sorts chronologically — so there is noDateparsing and no local-timezone reinterpretation between the input and the request. The value is sent with no offset, which the API reads as UTC, matching whatquery_timestampalready does from this same form; the hint says so. Strings added to all ten locales.Surfaces updated
Dataplane (
RecallRequest→ handler →recall_async→retrieve_all_fact_types_parallel), both MCPrecalltool definitions, regenerated OpenAPI + Go/Rust/Python/TypeScript clients, both hand-written wrappers (TStemporalWindow, Pythontemporal_window), the control-plane proxy route andlib/api.ts, and the docs (api/recall.mdx,mcp-server.md, regenerated docs skill).The Rust struct literals in
hindsight-cli(2) and the client doctest (1) had to name the new field — progenitor generatesRecallRequestat build time, so a struct literal that omits it stops compiling. Both crates verified withcargo check.Test
tests/test_recall_temporal_window.py(9): the supplied window reaches the store verbatim; it beats what the query text would have extracted; the extraction path is unchanged without it;enable_temporal_retrieval=falsestill wins; reversed/equal bounds; naive→UTC; and two HTTP-level tests for handler forwarding and the 422.min_scorespair:hindsight-clients/python/tests/test_recall_temporal_window.py, plus two cases intypescript/tests/main_operations.test.ts.npx vitest run(144, incl. 5 new for the window rules),npm run i18n:check,npx tsc --noEmit(5 errors = the pre-existing stale-SDK baseline, none in the changed files), plus a visual check of the rendered row../scripts/hooks/lint.sh,uv run ty check,./scripts/hooks/check-unused.sh,cargo check(cli + rust client),npx tsc --noEmit(TS client),npx jest -t temporal.