Skip to content

fix(memory): unblock legacy chunk reembedding#123

Open
hobertrand-hub wants to merge 2 commits into
tinyhumansai:mainfrom
hobertrand-hub:fix/openhuman-legacy-embedding-repair
Open

fix(memory): unblock legacy chunk reembedding#123
hobertrand-hub wants to merge 2 commits into
tinyhumansai:mainfrom
hobertrand-hub:fix/openhuman-legacy-embedding-repair

Conversation

@hobertrand-hub

@hobertrand-hub hobertrand-hub commented Jul 25, 2026

Copy link
Copy Markdown

Summary

  • fall back to legacy chunk content when content pointers/raw refs are absent
  • clear matching chunk reembed skip markers after a successful embedding write
  • ignore legacy body-read skip markers when detecting uncovered reembed work

Verification

  • cargo fmt --check
  • git diff --check
  • cargo test --features git-diff,sync,persona memory::chunks::store_embed_tests
  • cargo test --features git-diff,sync,persona high_level_chunk_reader_falls_back_to_legacy_content_column

Relation to source retrieval work

The mem_src source-retrieval fix has been split into focused PR #124. OpenHuman's current local integration branch can temporarily point at hobertrand-hub/tinycortex:integration/openhuman-memory-repair until the legacy and mem_src fixes are merged upstream and the submodule pointer is rebased onto upstream main.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@hobertrand-hub, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b06529f-a7f9-4b85-b626-78b565e711e6

📥 Commits

Reviewing files that changed from the base of the PR and between 953afeb and 700bf8b.

📒 Files selected for processing (8)
  • docs/openhuman-memory-migration.md
  • docs/openhuman-memory/README.md
  • docs/openhuman-memory/sources-registry-sync.md
  • docs/plan/05-openhuman-compat-matrix.md
  • src/memory/chunks/embeddings.rs
  • src/memory/chunks/store_embed_tests.rs
  • src/memory/store/content/read.rs
  • src/memory/store/content/read_tests.rs
📝 Walkthrough

Walkthrough

The change adds legacy chunk-content fallback handling, refines re-embedding skip-marker semantics, broadens mem_src retrieval matching, aligns TinyAgents dependency resolution and feature CI coverage, and updates OpenHuman ownership documentation.

Changes

Memory compatibility behavior

Layer / File(s) Summary
Legacy chunk content fallback
src/memory/store/content/*
Chunk body reads fall back to legacy content when staged pointers are missing or empty, with tests for successful fallback and empty-content errors.
Re-embedding skip-marker rules
src/memory/chunks/*
Embedding writes clear matching skip markers, while selected legacy body-read failures no longer suppress uncovered re-embedding work; related cases are tested.
mem_src retrieval scopes
src/memory/retrieval/*
Local and dense retrieval accept matching mem_src identifiers, and mem_src scopes are classified as documents with test coverage.

Dependency and build alignment

Layer / File(s) Summary
TinyAgents resolution and feature validation
.cargo/config.toml, Cargo.toml, vendor/tinyagents, .github/workflows/ci.yml
TinyAgents is pinned to version 2.1 with a local Cargo patch, the vendored revision advances, and CI adds persona feature coverage.

OpenHuman ownership documentation

Layer / File(s) Summary
Source reference normalization
docs/openhuman-memory-engine-spec.md, docs/openhuman-memory/README.md, docs/openhuman-memory/openhuman-code-reference.md
Developer-specific source paths are replaced with vendoring-host source-tree references.
Ownership boundary and migration updates
docs/openhuman-memory/*, docs/plan/05-openhuman-compat-matrix.md
Documentation assigns reusable provider and sync mechanics to TinyCortex while retaining live orchestration and host-owned responsibilities in OpenHuman.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: senamakel

Poem

A rabbit found old chunks tucked away,
And taught them to bloom in a newer display.
Skip-markers vanished when embeddings came through,
TinyAgents hopped to a pinned checkout too.
“Persona builds pass!” cried the hare with delight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main memory change: unblocking legacy chunk reembedding.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@docs/openhuman-memory-migration.md`:
- Around line 7-10: Unify the sync ownership terminology across
docs/openhuman-memory-migration.md:7-10, docs/openhuman-memory/README.md:13,
docs/openhuman-memory/sources-registry-sync.md:194-195, and
docs/plan/05-openhuman-compat-matrix.md:74-77: describe TinyCortex as owning
only reusable provider fetch, pagination, and canonicalization pipeline
mechanics behind injected traits, while OpenHuman owns the live sync runner,
credentials, scheduling, policy, callbacks, RPC, and product events. Update each
site to remove any implication that TinyCortex owns or does not own the complete
sync responsibility, preserving the single pipeline-versus-runner contract.
🪄 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: 977a9e60-21e2-4947-9c13-5926249b30b5

📥 Commits

Reviewing files that changed from the base of the PR and between 108b8e0 and 6d74085.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • .cargo/config.toml
  • .github/workflows/ci.yml
  • Cargo.toml
  • docs/openhuman-memory-engine-spec.md
  • docs/openhuman-memory-migration.md
  • docs/openhuman-memory/README.md
  • docs/openhuman-memory/openhuman-code-reference.md
  • docs/openhuman-memory/sources-registry-sync.md
  • docs/plan/05-openhuman-compat-matrix.md
  • src/memory/chunks/embeddings.rs
  • src/memory/chunks/store_embed_tests.rs
  • src/memory/store/content/read.rs
  • src/memory/store/content/read_tests.rs
  • vendor/tinyagents

Comment thread docs/openhuman-memory-migration.md Outdated
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR unblocks legacy chunk re-embedding by (1) falling back to the chunk.content column when no content pointer or raw refs exist, (2) deleting the matching skip marker after a successful embedding write, and (3) excluding the two legacy skip-reason strings from the terminal-skip filter so those chunks remain visible to the retry worker.

  • read.rs: read_chunk_body now delegates to read_legacy_chunk_preview instead of bailing immediately when a content pointer is absent or empty; the new helper emits a distinct "legacy chunk content empty for chunk X" error for the degenerate-data case, which is intentionally not excluded by the LIKE filters and correctly halts retries.
  • embeddings.rs: upsert_chunk_embedding_conn appends a DELETE FROM mem_tree_chunk_reembed_skipped after the embedding upsert, and has_uncovered_reembed_work now ignores both old-style "no content pointer" and "empty content pointer" markers so those chunks are retried with the new fallback path.
  • Tests: Four new embed tests and two new reader tests cover every branch of the LIKE filter and the legacy fallback, including the terminal case where chunk.content is empty.

Confidence Score: 5/5

Safe to merge — the three interlocking changes (legacy fallback, skip-marker clearing, LIKE filter widening) are mutually consistent and each path through the new logic is covered by a dedicated test.

The new read_legacy_chunk_preview emits a distinct error string for the degenerate empty-content case that is intentionally absent from the LIKE exclusion patterns, so retries halt correctly. Old markers that previously caused silent skips are now re-opened for retry, and a successful embed write removes the marker in the same connection. All four skip-reason branches are exercised by new tests, as is the legacy-content fallback path itself. No logic gaps or correctness issues were found.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
src/memory/chunks/embeddings.rs Adds DELETE of matching skip marker after a successful embedding upsert, and widens the NOT EXISTS subquery filter to exclude both legacy reason strings so those chunks remain visible to the retry loop
src/memory/store/content/read.rs Replaces hard errors for absent/empty content pointers with a fallback to read_legacy_chunk_preview; generates a distinct terminal error string for empty legacy content that is not matched by the LIKE exclusion patterns
src/memory/chunks/store_embed_tests.rs Adds four new tests covering skip-marker clearing, both excluded LIKE patterns, the terminal legacy-empty case, and the non-legacy skip marker case
src/memory/store/content/read_tests.rs Adds two integration tests: legacy content column fallback and exact terminal error string for empty legacy content
docs/openhuman-memory-migration.md Documentation-only ownership boundary description refresh
docs/openhuman-memory/README.md Documentation-only boundary wording update
docs/openhuman-memory/sources-registry-sync.md Documentation-only ownership wording update
docs/plan/05-openhuman-compat-matrix.md Documentation-only pipeline ownership clarification

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[read_chunk_body] --> B{raw refs present?}
    B -- yes --> C[read_chunk_body_from_raw]
    B -- no --> D{content pointer row exists?}
    D -- yes, non-empty path --> E[read_chunk_file]
    D -- no row --> F[read_legacy_chunk_preview]
    D -- empty path --> F
    F --> G{get_chunk returns Some?}
    G -- no --> H["bail: no content pointer or raw refs\n(LIKE-excluded → retry)"]
    G -- yes --> I{chunk.content empty?}
    I -- yes --> J["bail: legacy chunk content empty\n(NOT excluded → terminal)"]
    I -- no --> K[return chunk.content]
    E --> L[upsert embedding]
    K --> L
    L --> M[DELETE matching skip marker]
    M --> N[chunk no longer in reembed work queue]
Loading

Reviews (5): Last reviewed commit: "fix(memory): keep empty legacy chunks te..." | Re-trigger Greptile

Comment thread src/memory/chunks/store_embed_tests.rs
@hobertrand-hub hobertrand-hub changed the title fix(memory): unblock legacy chunk reembedding fix(memory): repair legacy chunks and mem_src retrieval Jul 26, 2026
@hobertrand-hub
hobertrand-hub force-pushed the fix/openhuman-legacy-embedding-repair branch from 953afeb to e681073 Compare July 26, 2026 23:10
@hobertrand-hub hobertrand-hub changed the title fix(memory): repair legacy chunks and mem_src retrieval fix(memory): unblock legacy chunk reembedding Jul 26, 2026
@hobertrand-hub
hobertrand-hub force-pushed the fix/openhuman-legacy-embedding-repair branch from e681073 to 700bf8b Compare July 26, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant