Repair OpenHuman memory source retrieval paths#125
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
📝 WalkthroughWalkthroughChangesMemory compatibility and ownership
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 |
|
| 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 |
1 participant
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 when matching hits.mem_src:document scopes as document kind matches during scope filtering.