fix(memory): handle mem_src scopes in retrieval#124
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMemory retrieval now supports ChangesMemory source scope matching
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready for review. This is the focused mem_src retrieval fix split out from #123 so OpenHuman source-scoped retrieval can depend on a small, independently reviewable TinyCortex change. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| src/memory/retrieval/fast.rs | Replaces direct HashSet containment checks with source_scope_allows, adds extract_mem_src_id; new mem_src code path is untested and has a case-sensitivity mismatch vs. source.rs |
| src/memory/retrieval/source.rs | Adds mem_src: prefix recognition inside scope_matches_kind so source trees with composite mem_src scopes are classified as document kind; logic is correct and narrowly scoped |
| src/memory/retrieval/source_tests.rs | Adds a single assertion for mem_src scope in scope_prefix_matching_known_platforms; covers the source.rs change but not the fast.rs change |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[fast_retrieve called\nwith source_scope] --> B{entity_ids\nempty?}
B -- yes --> C[dense path:\nquery_source no filter]
B -- no --> D{graph pairs\nfound?}
D -- no --> E[global_occurrence\ncalls dense]
D -- yes --> F[resolve_local\nfrom candidates]
C --> G[source_scope_allows\nfor each hit]
E --> G
F --> G
G --> H{scope.contains\ntree_scope?}
H -- yes --> I[Allow hit]
H -- no --> J{mem_src: prefix?}
J -- yes --> K[extract_mem_src_id]
K --> L{scope.contains\ncollection_id?}
L -- yes --> I
L -- no --> M[Reject hit]
J -- no --> M
Reviews (1): Last reviewed commit: "Handle mem_src scopes in source retrieva..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn source_scope_allows(scope: &HashSet<String>, tree_scope: &str) -> bool { | ||
| if scope.contains(tree_scope) { | ||
| return true; | ||
| } | ||
| extract_mem_src_id(tree_scope).is_some_and(|id| scope.contains(id)) | ||
| } | ||
|
|
||
| fn extract_mem_src_id(value: &str) -> Option<&str> { | ||
| let rest = value.strip_prefix("mem_src:")?; | ||
| let (id, _) = rest.split_once(':')?; | ||
| (!id.is_empty()).then_some(id) | ||
| } |
There was a problem hiding this comment.
mem_src path in fast retrieval
source_scope_allows and extract_mem_src_id are not exercised by any test in fast_tests.rs. The existing tests (dense_fallback_filters_scope_before_limit, resolve_local_hydrates_leaf_and_summary_and_skips_missing_or_out_of_scope) always use scopes like "slack:#allowed" that match via the direct scope.contains(tree_scope) branch, so they never reach the new extract_mem_src_id code path. A test seeding a tree with scope mem_src:src_vault_hob_local:path/to/note.md and a source_scope containing only "src_vault_hob_local" would confirm the core fix behaves correctly end-to-end through fast_retrieve.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| fn extract_mem_src_id(value: &str) -> Option<&str> { | ||
| let rest = value.strip_prefix("mem_src:")?; | ||
| let (id, _) = rest.split_once(':')?; | ||
| (!id.is_empty()).then_some(id) | ||
| } |
There was a problem hiding this comment.
scope_matches_kind
extract_mem_src_id uses a case-sensitive strip_prefix("mem_src:"), while the parallel function scope_matches_kind in source.rs normalises the scope via to_lowercase() before checking lower.starts_with("mem_src:"). A tree_scope stored with any capitalisation variant (e.g. Mem_Src:...) would be classified as document kind by scope_matches_kind but would silently fall through the fast-retrieval filter because extract_mem_src_id returns None. Lowercasing the prefix portion before the strip_prefix call would align both paths.
1 participant
Summary
mem_src:<source_id>:<item>scopes as document/source scopes during source retrievalmem_srccollection idsmem_srcdocument scope classificationWhy
OpenHuman stores reader-backed source trees with scopes such as
mem_src:src_vault_hob_local:path/to/note.md. Source-scoped retrieval can be asked for the collection id (src_vault_hob_local), so TinyCortex needs to recognize the collection id embedded inside the compositemem_srctree scope. Without this, host source retrieval can miss indexed summaries even when the source is correctly ingested.Validation
cargo +1.96.1 fmt --checkcargo +1.96.1 test scope_prefix_matching_known_platforms --libcargo +1.96.1 test source --libRelation to PR #123
This is a focused PR for only the
mem_srcretrieval fix. The same change was previously pushed onto the broader legacy chunk repair branch in #123, but keeping it separate makes the OpenHuman source-retrieval dependency easier to review and merge independently.Summary by CodeRabbit
mem_src:scopes are correctly recognized as documents.mem_src:scopes.