fix(sdk/recall): harden supersede counters, slot key contract, and resolver path#49
Merged
Merged
Conversation
…solver path
Addresses a review of the recall package's slot supersede + LLM update
resolver feature. Changes are correctness, escape-hatch, and
maintainability fixes; no public API removals.
Bugs:
- supersedeBySlot only increments supersede_total{channel="slot"}
after a successful Upsert (was over-counting candidates the loop
later skipped).
- normalizeSubject lowercases unknown tokens so case-only differences
("Alice" vs "alice") collapse onto the same slot_key, matching
normalizePredicate's behaviour.
- Add Request.WithTombstoned escape hatch and document MetaTombstone
as a reserved key so pre-existing user data under "tombstone" can
still be surfaced via Recall when needed.
Design:
- New WithoutSlotChannel option separates the deterministic slot path
from the noisy vector soft-merge so callers can disable one without
the other; WithoutSoftMerge now affects the vector channel only.
- slotEligible() rejects subject/predicate values containing the
slot delimiter "|" so ambiguous slot_keys are never written; such
facts gracefully degrade to the vector / resolver channels.
- Resolver invocation now opens its own span (memory.recall.resolver_batch)
with n_facts / n_candidates attributes for cost attribution.
- Centralise reserved metadata keys under sdk/recall/metadata.go and
sdk/retrieval/metadata.go; the SlotCollapse stage and recall package
now share retrieval.SlotKeyOf instead of duplicating the lookup.
Polish:
- LLMUpdateResolver sends a strict resolverActionsSchema alongside
WithJSONMode so providers that enforce JSON-schema (e.g. Azure
OpenAI strict) accept the call.
- Expand PredicateAliases coverage for high-frequency CN/EN surface
forms (lives_in / works_at / occupation / birthday / language /
spouse / child / parent / pet) and document the table as curated
rather than exhaustive; per-namespace overrides via
WithPredicateAlias still win.
- ResolveAction godoc clarifies that SourceID/TargetID are
informational/optional for ADD/NOOP and required for UPDATE/DELETE.
- Drop the now-stale references to docs/recall-update-resolution.md
(file does not exist in tree); link to in-code symbols instead.
Tests:
- TestTombstoneFilter_HidesByDefault_OptInReveals (memory_test.go)
- TestSlotChannel_KeptWhenWithoutSoftMerge,
TestSlotChannel_DisabledByWithoutSlotChannel,
TestSlotSupersede_MultipleStaleEntriesInOneSlot,
TestSlotEligibility_RejectsDelimiterInSubject,
TestSlotCollapse_EndToEnd_OnLegacyData (merger_test.go)
- TestNormalizeSubject_LowercasesUnknownTokens (aliases_test.go)
Made-with: Cursor
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.
Summary
Hardens the slot supersede + LLM update resolver feature in
sdk/recallafter a code review that surfaced counter accounting bugs, a missing escape hatch, and several maintainability issues.Bugs (correctness)
supersedeBySlotonly incrementssupersede_total{channel="slot"}after a successfulUpsert, matching the vector / resolver channels. Previously the metric was bumped bylen(resp.Items)before the loop's skip-list ran, over-reporting in dashboards.normalizeSubjectlowercases unknown tokens so case-only differences ("Alice"vs"alice") collapse onto the sameslot_key. The behaviour now mirrorsnormalizePredicateand is documented in godoc.Request.WithTombstonedso callers can surface tombstoned entries (debug resolver, stageAuditable.Rollback).MetaTombstoneis now documented as a reserved metadata key.Design / maintainability
sdk/recall/metadata.go(MetaSubject,MetaPredicate,MetaSupersededBy,MetaSupersededAt,MetaTombstone,MetaContentHash,MetaSourceLabel,MetaEntities) plussdk/retrieval/metadata.go(MetaSlotKey,SlotKeyOf). Pipeline'sSlotCollapseand the recall package now share one lookup helper instead of duplicating it.slotEligible(fact)rejects subject/predicate values containing the slot delimiter|so ambiguousslot_keys are never written; such facts gracefully degrade to the vector / resolver channels.WithoutSlotChannel()option separates the deterministic slot path from the noisy vector soft-merge.WithoutSoftMerge()now affects the vector channel only and the godoc spells the difference out.memory.recall.resolver_batchwithn_facts/n_candidatesattributes for cost attribution without enablingSearchDebug.WithUpdateResolvergodoc explains the ordering relationship with the slot/vector supersede channels and the resulting candidate-count behaviour.Polish
save.go; replaced with a "single source of truth" rationale that actually matches the code.ResolveActiongodoc clarifies thatSourceID/TargetIDare required forUPDATE/DELETEand informational/optional forADD/NOOP.PredicateAliasescoverage for high-frequency CN/EN surface forms across all v1 predicates (住/住所/老家/故乡/供职/就职/出生/爱人/伴侣,based_in/married_to/born_on/son/daughter, …); table is now documented as curated rather than exhaustive, withWithPredicateAliasoverrides retained as the per-namespace extension point.LLMUpdateResolversends a strictresolverActionsSchemaalongsideWithJSONModeso providers that enforce JSON-schema (Azure OpenAI strict) accept the call.docs/recall-update-resolution.md(file does not exist in tree); link to in-code symbols instead.New tests
TestTombstoneFilter_HidesByDefault_OptInRevealsmemory_test.goTestSlotChannel_KeptWhenWithoutSoftMergemerger_test.goTestSlotChannel_DisabledByWithoutSlotChannelmerger_test.goTestSlotSupersede_MultipleStaleEntriesInOneSlotmerger_test.goTestSlotEligibility_RejectsDelimiterInSubjectmerger_test.goTestSlotCollapse_EndToEnd_OnLegacyDatamerger_test.goTestNormalizeSubject_LowercasesUnknownTokensaliases_test.goTest plan
go build ./sdk/...go vet ./sdk/...go test ./sdk/recall/...go test ./sdk/... ./sdkx/... ./voice/...Made with Cursor