Repair OpenHuman memory source retrieval paths - #125
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes clarify TinyCortex and OpenHuman ownership boundaries. They add legacy chunk-content fallback, improve re-embedding tombstone handling, and broaden retrieval matching for ChangesMemory compatibility and ownership
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 |
|
| Filename | Overview |
|---|---|
| src/memory/store/content/read.rs | Changed read_chunk_body to fall back to a new read_legacy_chunk_preview helper when no content pointer exists, restoring usable text from the inline content column for legacy chunk rows. Added three public constants that anchor the reembed-backfill skip-reason contract. |
| src/memory/chunks/embeddings.rs | upsert_chunk_embedding_conn now deletes matching skip tombstones on successful embedding, and has_uncovered_reembed_work excludes chunks whose only skip reason matches the two legacy-content-pointer retryable prefixes. |
| src/memory/retrieval/fast.rs | Replaced raw HashSet::contains scope checks with source_scope_allows, and added three helper functions to handle mem_src: prefix extraction and case-insensitive matching. |
| src/memory/retrieval/source.rs | Added mem_src: → document kind classification arm in scope_matches_kind with case-insensitive lowercased guard. |
| src/memory/chunks/store_embed_tests.rs | Added four new targeted tests covering skip-marker cleanup on embedding upsert, retryable skip reason transparency for has_uncovered_reembed_work, and terminal empty-content behavior. |
| src/memory/retrieval/fast_tests.rs | Added mem_src scope filter test covering bare-ID, collection-prefix, and exact tree-scope forms, plus negative case. |
| src/memory/retrieval/source_tests.rs | Extended scope_prefix_matching_known_platforms test with mem_src: cases in both lowercase and mixed case. |
| src/memory/store/content/read_tests.rs | Added two tests verifying legacy-content-column fallback reads and the terminal error path for empty inline content. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["read_chunk_body(chunk_id)"] --> B{raw refs present?}
B -- yes --> C["read_chunk_body_from_raw"]
B -- no --> D{content pointer exists?}
D -- yes --> E["resolve path + read file\n(repair checksum if stale)"]
D -- no --> F["read_legacy_chunk_preview"]
F --> G{get_chunk returns Some?}
G -- no --> H["bail: LEGACY_NO_CONTENT_POINTER_REASON_PREFIX\n(retryable skip reason)"]
G -- yes --> I{chunk.content empty?}
I -- yes --> J["bail: LEGACY_EMPTY_CHUNK_CONTENT_REASON_PREFIX\n(terminal skip reason)"]
I -- no --> K["return chunk.content (legacy inline column)"]
subgraph reembed_backfill
L["has_uncovered_reembed_work"] -->|"chunk has no embedding AND no terminal skip row"| M["chunk in worklist"]
N["upsert_chunk_embedding_conn"] -->|success| O["DELETE matching skip tombstone"]
end
K --> N
E --> N
H -->|"wraps to body read failed: no content pointer"| P["mark_chunk_reembed_skipped (retryable)"]
J -->|"wraps to body read failed: legacy chunk content empty"| Q["mark_chunk_reembed_skipped (terminal)"]
Reviews (3): Last reviewed commit: "Document legacy reembed skip reasons" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/memory/store/content/read.rs (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFragile string contract between legacy bail! messages and SQL
LIKEclassification.
read_legacy_chunk_previewinread.rsauthors the exact error text thathas_uncovered_reembed_workinembeddings.rspattern-matches viaLIKEto decide whether a skip record is retryable (legacy) or terminal. This coupling is undocumented and only enforced by tests — an innocuous wording tweak in one file silently changes re-embed retry semantics in the other, including a pattern in embeddings.rs that intentionally targets only pre-existing (pre-fix) skip rows no longer produced by current code.
src/memory/store/content/read.rs#L159-168: add a doc comment onread_legacy_chunk_previewnoting that its bail! message text is matched verbatim byLIKEpatterns inembeddings.rs::has_uncovered_reembed_work, or extract the strings into shared constants used by both sites.src/memory/chunks/embeddings.rs#L393-395: add a comment on the twoNOT LIKEclauses explaining that the second pattern targets legacy (pre-fix) skip rows only and is no longer generated by currentread.rscode, to avoid future removal as "dead."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/content/read.rs` at line 1, Document the error-message contract in read_legacy_chunk_preview, noting that its bail! text is matched by LIKE patterns in embeddings.rs::has_uncovered_reembed_work. Also comment the two NOT LIKE clauses there, clarifying that the second preserves handling for legacy pre-fix skip rows no longer produced by current read.rs code; prefer shared constants only if already appropriate.
🧹 Nitpick comments (1)
src/memory/store/content/read.rs (1)
140-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLikely unreachable
rel_path.is_empty()branch.
get_chunk_content_pointersalready filters out empty paths before returningSome: "Ok(row.and_then(|(p, s)| p.zip(s).filter(|(path, _)| !path.is_empty())))". Given that contract, onceSome((rel_path, expected_sha256))is destructured,rel_pathcannot be empty, so this check appears to be dead code.If this is intentional defense against a future contract change, a short comment noting that would help; otherwise consider removing it to avoid confusing readers about when the legacy-preview path triggers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/content/read.rs` around lines 140 - 145, Remove the redundant rel_path.is_empty() check in the read content flow after get_chunk_content_pointers returns Some, since that function already filters empty paths. Keep the existing read_legacy_chunk_preview fallback for the None case and preserve normal processing for valid pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/chunks/embeddings.rs`:
- Around line 393-395: Add a concise comment beside the legacy “empty content
pointer and no raw refs” LIKE filter explaining that it preserves retries for
pre-existing skip rows from older read.rs behavior, even though current code no
longer emits that message. Leave both retry filters and their matching patterns
unchanged.
In `@src/memory/store/content/read.rs`:
- Around line 159-168: The legacy chunk bail messages in
read_legacy_chunk_preview are coupled to hardcoded SQL LIKE prefixes in
has_uncovered_reembed_work. Extract shared reason-prefix constants and use them
both when constructing these errors and in the embedding query, preserving the
existing message text and matching behavior.
---
Outside diff comments:
In `@src/memory/store/content/read.rs`:
- Line 1: Document the error-message contract in read_legacy_chunk_preview,
noting that its bail! text is matched by LIKE patterns in
embeddings.rs::has_uncovered_reembed_work. Also comment the two NOT LIKE clauses
there, clarifying that the second preserves handling for legacy pre-fix skip
rows no longer produced by current read.rs code; prefer shared constants only if
already appropriate.
---
Nitpick comments:
In `@src/memory/store/content/read.rs`:
- Around line 140-145: Remove the redundant rel_path.is_empty() check in the
read content flow after get_chunk_content_pointers returns Some, since that
function already filters empty paths. Keep the existing
read_legacy_chunk_preview fallback for the None case and preserve normal
processing for valid pointers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6280690f-7089-4d73-a594-a00a77433bf2
📒 Files selected for processing (11)
docs/openhuman-memory-migration.mddocs/openhuman-memory/README.mddocs/openhuman-memory/sources-registry-sync.mddocs/plan/05-openhuman-compat-matrix.mdsrc/memory/chunks/embeddings.rssrc/memory/chunks/store_embed_tests.rssrc/memory/retrieval/fast.rssrc/memory/retrieval/source.rssrc/memory/retrieval/source_tests.rssrc/memory/store/content/read.rssrc/memory/store/content/read_tests.rs
|
Addressed the Greptile P2 notes in follow-up commit
Validation after the follow-up:
|
|
Addressed the CodeRabbit maintainability comments in follow-up commit
Validation after this follow-up:
|
|
@senamakel PR #125 is ready from my side for maintainer squash-merge. Current visible status:
Please squash-merge into |
|
@senamakel quick follow-up on PR #125. Checks are green and CodeRabbit approved, but GitHub still reports I’m keeping the OpenHuman host branch parked until this lands upstream. |
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
b9e4f56 to
d92de6d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/memory/chunks/embeddings.rs (1)
487-511: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
src/memory/chunks/embeddings.rsbefore merge.This file reaches at least 530 lines. This exceeds the 500-line limit.
Move the signature-aware embedding query functions, including
get_chunk_embeddings_for_signature_batch, to a focused sibling module. Keep related tests in that module’s<name>_tests.rssibling.As per coding guidelines,
src/**/*.rsfiles must stay below 500 lines and split behavior into focused modules before that point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/chunks/embeddings.rs` around lines 487 - 511, Split the signature-aware embedding query functions, including get_chunk_embeddings_for_signature_batch, out of embeddings.rs into a focused sibling Rust module, preserving their public API and behavior. Move the related tests into the corresponding <name>_tests.rs sibling, update module declarations and imports, and ensure embeddings.rs and the new module remain below 500 lines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/memory/chunks/embeddings.rs`:
- Around line 487-511: Split the signature-aware embedding query functions,
including get_chunk_embeddings_for_signature_batch, out of embeddings.rs into a
focused sibling Rust module, preserving their public API and behavior. Move the
related tests into the corresponding <name>_tests.rs sibling, update module
declarations and imports, and ensure embeddings.rs and the new module remain
below 500 lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93a52b66-1a4e-4329-8899-fd7bd9f20de1
📒 Files selected for processing (4)
src/memory/chunks/embeddings.rssrc/memory/chunks/store_embed_tests.rssrc/memory/store/content/read.rssrc/memory/store/content/read_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/memory/chunks/store_embed_tests.rs
- src/memory/store/content/read_tests.rs
signature_variants' doc comment linked to signature_in_clause, which is pub(crate); rustdoc rejects that link with -D rustdoc::private_intra_doc_links and the CI Document step fails. The error predates this PR (current main's own push run fails the same way); keep the reference as plain code text. Co-authored-by: Medulla <medulla@tinyhumans.ai>
…gs_query Embeddings.rs had grown to 541 lines, over the repo's 500-line ceiling (as flagged in review). Move the read-side, signature-aware query functions — get_chunk_embedding_for_signature, get_chunk_embedding, get_chunk_embeddings_for_signature_batch, get_chunk_embeddings_batch and has_uncovered_reembed_work (plus the embedding_from_blob decoder and the MAX_EMBEDDING_BATCH cap) — into a focused embeddings_query sibling. embeddings.rs keeps the writers, upserts, and re-embed tombstones; both remain under 500 lines and the public chunks re-export surface is unchanged, so no caller or test file needs edits. Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Rebased onto rewritten The merge was blocked by a conflict GitHub couldn't even diff (
Three additional notes:
All required checks are green and every review thread is resolved. No functional behavior of the PR changed. |
Two drain tests (drain_terminates_on_a_deferred_job, drain_tolerates_retired_kind_rows) flaked in CI under the parallel all-features suite. The worker routes LLM-bound jobs to a process-wide gate keyed by queue.llm_permits, so every test config leaving it at the default of 1 shares a single slot; the worker gate-semantics tests can briefly hold the sole permit, and the drain's job then gets deferred with "llm concurrency gate busy", ending the drain before its handler ran. Give the drain tests their own gate by raising llm_permits in their test config (gates are keyed by permit count). Assertions are unchanged — the tests now simply never contend for the shared slot. No production code touched. Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Flake root-caused and fixed (not just re-run). The flaky
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0180 · 104,506 in / 59,741 out · 79,409 cached (76%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
critique: $0.0123 · 44,449 in / 41,405 out · 34,360 cached (77%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
security: $0.0031 · 28,795 in / 10,824 out · 22,393 cached (78%) · z-ai/glm-5.2
tests: $0.0013 · 15,502 in / 4,195 out · 11,564 cached (75%) · z-ai/glm-5.2
description: $0.0012 · 15,760 in / 3,317 out · 11,092 cached (70%) · z-ai/glm-5.2
| if !prefix.eq_ignore_ascii_case("mem_src:") { | ||
| return false; | ||
| } | ||
| let rest = &value[8..]; |
There was a problem hiding this comment.
Restrict mem_src scope matching to collection prefixes, not full paths
The new mem_src_scope_selects_id function treats any mem_src:-prefixed entry in the caller's scope set as a source-level selector, regardless of whether that entry includes a path component. When a caller's scope contains a path-specific grant such as mem_src:src-folder-9:some/path, the function extracts only src-folder-9 and matches it against every hit whose tree_scope starts with mem_src:src-folder-9:. The comment above the function states that "the exact tree scope" is one of the three valid selector forms, implying that a mem_src:<source_id>:<path> scope entry should match only that exact tree. But because mem_src_scope_selects_id strips the path and compares only the source id, a path-specific scope entry silently becomes a source-wide grant, letting the caller access memory from any path under the same source.
[RULE] authentication and authorisation changes ·
What this change touches16 files, +578 -260 across 7 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src/memory/chunks<br/>5 files +390 -227<br/>1 finding"]:::flagged
n1["src/memory/retrieval<br/>4 files +82 -2<br/>4 findings"]:::flagged
n2["src/memory/store/content<br/>2 files +67 -7"]:::changed
n3["docs<br/>1 file +17 -15"]:::changed
n4["docs/openhuman-memory<br/>2 files +8 -4"]:::changed
n5["src/memory/queue<br/>1 file +9 -1"]:::changed
n6["docs/plan<br/>1 file +5 -4"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p1
There was a problem hiding this comment.
The previously-blocking findings are resolved. Clearing the changes request.
$0.0163 · 113,988 in / 61,106 out · 89,813 cached (79%) · z-ai/glm-5.2
critique: $0.0075 · 51,439 in / 28,442 out · 40,955 cached (80%) · z-ai/glm-5.2
security: $0.0046 · 30,931 in / 17,408 out · 25,039 cached (81%) · z-ai/glm-5.2
tests: $0.0024 · 15,680 in / 8,795 out · 11,197 cached (71%) · z-ai/glm-5.2
description: $0.0018 · 15,938 in / 6,461 out · 12,622 cached (79%) · z-ai/glm-5.2
| // `mem_src:<source_id>:<path>`, so kind filtering must classify the whole | ||
| // tree family as documents while exact source-id callers still use the | ||
| // source_id path above. | ||
| if kind_prefix == SourceKind::Document.as_str() && lower.starts_with("mem_src:") { |
There was a problem hiding this comment.
Restrict mem_src scope matching to collection prefixes, not full paths
The added check classifies every mem_src:<source_id>:<path> string as a Document source for kind-filtering purposes:
if kind_prefix == SourceKind::Document.as_str() && lower.starts_with("mem_src:") {
return true;
}This is the same broad match the prior finding flagged. The comment describes the format as mem_src:<source_id>:<path>, but the predicate accepts any string beginning with mem_src:, including arbitrary <source_id> and <path> components. If non-Document sources can also produce mem_src:-prefixed entries (or if callers rely on this function to reject paths outside a collection scope), this will over-classify them as documents. The match should be tightened to the collection-prefix shape — e.g. requiring the mem_src:<source_id>: form and not arbitrary trailing path segments — or the function should delegate to a source-id lookup that already exists, as the comment alludes to ("exact source-id callers still use the source_id path above").
**[RULE] ** ·
| assert!(scope_matches_kind("gmail:alice", "email")); | ||
| assert!(scope_matches_kind("notion:page123", "document")); | ||
| assert!(scope_matches_kind("linear:conn-1:issue-abc", "document")); | ||
| assert!(scope_matches_kind( |
There was a problem hiding this comment.
Restrict mem_src scope matching to collection prefixes, not full paths
The earlier finding still stands. This diff adds test assertions that explicitly confirm and codify the behavior the finding identified as problematic:
existing_code: assert!(scope_matches_kind(
"mem_src:src-folder-9:Slides_Notes/example.md",
"document"
));
assert!(scope_matches_kind(
"Mem_Src:src-folder-9:Slides_Notes/example.md",
"document"
));
These assertions lock in full-path matching (Slides_Notes/example.md) for mem_src scopes rather than restricting the match to the collection prefix (mem_src:src-folder-9). If a scope is intended to authorize access at the collection level, allowing the full file path to be part of the scope identifier means the scope string becomes unpredictable and effectively unbounded — an attacker who can influence the path portion of a stored memory can craft a scope that bypasses collection-level access control. The tests should instead assert that the prefix form matches and that arbitrary path suffixes do not broaden or change the kind match.
[RULE] Restrict mem_src scope matching to collection prefixes, not full paths ·
| for (id, _) in ordered { | ||
| if let Some(mut hit) = by_id.remove(&id) { | ||
| if source_scope.is_some_and(|scope| !scope.contains(&hit.tree_scope)) { | ||
| if source_scope.is_some_and(|scope| !source_scope_allows(scope, &hit.tree_scope)) { |
There was a problem hiding this comment.
Prior finding fixed: mem_src scope matching now uses collection prefixes
Prior finding: Restrict mem_src scope matching to collection prefixes, not full paths
Fixed. The new source_scope_allows extracts the source id from per-file mem_src:<source_id>:<path> tree scopes and matches against bare ids, collection prefixes (mem_src:<source_id>), and exact scopes. The test mem_src_scope_filter_accepts_bare_id_collection_prefix_and_exact_scope confirms all three paths. Dropping this finding.
[RULE] prior-finding ·
| (!id.is_empty()).then_some(id) | ||
| } | ||
|
|
||
| fn mem_src_scope_selects_id(value: &str, expected_id: &str) -> bool { |
There was a problem hiding this comment.
Treat full mem_src paths as exact scopes, not collection prefixes
New finding
mem_src_scope_selects_id extracts the source id from any mem_src:-prefixed allowed scope value and discards the path component. When a caller puts a full per-file path like mem_src:src-folder-9:file_a.md in the scope set, it matches all files under src-folder-9, not just file_a.md.
The exact-match guard (scope.contains(tree_scope)) handles the case where the allowed scope equals the hit's tree scope, but a caller scoping to one file of a multi-file source will see results from sibling files.
No test covers a full-path scope value that differs from the tree scope. The test mem_src_scope_filter_accepts_bare_id_collection_prefix_and_exact_scope only tests the three documented input shapes and a non-matching source. A negative test — a full mem_src:<id>:<other_path> scope should not match a different file under the same source — would fail against the current code.
mem_src_scope_selects_id should distinguish mem_src:<id> (collection prefix, match all files) from mem_src:<id>:<path> (specific file, match only via exact match) by checking whether rest contains a colon and returning false when it does.
[RULE] behaviour-test ·
tinysweeper
Bot
added
priority: p2
2 participants
Summary
This is the TinyCortex dependency PR for the OpenHuman source-retrieval repair. It keeps the patch narrow to memory/source retrieval behavior needed by OpenHuman before the host branch is exported.
Changes included:
mem_src:<source_id>:...source scopes during source-scoped retrievalWhy
OpenHuman source-note queries depend on TinyCortex being able to retrieve document-tree scopes and recover usable text from legacy chunk rows. Without these fixes, host-side routing can classify the request correctly but source retrieval still misses or stalls on legacy/indexed source data.
Validation
cargo +1.96.1 test store_embed --lib -- --nocapturecargo +1.96.1 test source --lib -- --nocapturecargo +1.96.1 test content::read --lib -- --nocapturecargo +1.96.1 test --libFull lib result:
1238 passed; 0 failed.Summary by CodeRabbit
mem_src:-based scopes and selector forms.mem_src:document scopes as document matches during filtering.