fix(memory): connector-aware recall — re-embed sweep, signature/provider, gmail body, scout memory-first - #5258
fix(memory): connector-aware recall — re-embed sweep, signature/provider, gmail body, scout memory-first#5258yh928 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds connector-aware memory recall, recall-content shaping, and durable repair for deficient vector embeddings. It integrates recall and repair into agent context, memory tools, embedding updates, startup, login, and sync completion. ChangesConnector-aware memory recall
Durable vector re-embedding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentContext
participant auto_recall
participant Memory
AgentContext->>auto_recall: recall_with_connectors(query, limit)
auto_recall->>Memory: recall global memories
auto_recall->>Memory: recall selected connector namespaces
auto_recall-->>AgentContext: ranked memory entries
sequenceDiagram
participant SyncOrRuntime
participant vector_reembed
participant MemoryClient
participant UnifiedMemory
SyncOrRuntime->>vector_reembed: ensure_vector_reembed()
vector_reembed->>MemoryClient: sweep_pending_embeddings(budget)
MemoryClient->>UnifiedMemory: reembed_pending(budget)
UnifiedMemory-->>vector_reembed: repaired and failed counts
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
coderabbitai
Bot
added
bug
memory
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0211df28ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| src/openhuman/memory_store/namespace_store/reembed.rs | New durable re-embed sweep: scans vector_chunks for NULL/wrong-dim/stale-signature rows and re-embeds with backoff. Degraded-provider guard correctly refuses short vectors. Write-back is keyed on text to avoid stamping stale vectors after a concurrent re-ingest. |
| src/openhuman/memory_store/vector_reembed.rs | Background driver with an atomic in-flight flag and SweepGuard to clear the flag on panic. Correctly exits when no sweep is repairable rather than spinning. |
| src/openhuman/memory/auto_recall.rs | New connector-aware recall: fans out to global + busiest 4 skill-* namespaces concurrently, merges by score, deduplicates by (namespace, key), truncates to limit. Errors in any namespace are skipped rather than failing the turn. |
| src/openhuman/memory/recall_shaping.rs | New condensation pass: re-chunks recalled content, picks query-relevant chunks by keyword overlap, joins with ellipsis markers, hard-caps correctly subtracting ELLIPSIS_CHARS=3. |
| src/openhuman/memory/tools/recall.rs | namespace made optional; score fixed from raw 0-1 to real percentage; origin namespace shown for non-global hits; content condensed via condense_recall_content. |
| src/openhuman/memory_sync/composio/providers/gmail/sync_executor.rs | New ReshapingExecutor decorator restoring the Gmail MIME to slim-record reshape. Passes through error responses and respects raw_html flag via the same predicate as post_process. |
| src/openhuman/tinycortex/sync.rs | gmail_pipeline helper wraps ComposioClient in ReshapingExecutor before passing to GmailSyncPipeline::with_executor in both run_gmail_backfill and build_pipeline. |
| src/openhuman/tinycortex/config.rs | Carries embedding_provider into EmbeddingConfig so the tree's active signature matches the namespace store's embedder, preventing vector space divergence. |
| src/openhuman/agent/harness/memory_context.rs | Switches from recall_through_facade (global-only, Result) to recall_with_connectors (global + connectors, always Vec). Rest of context-building logic unchanged. |
| src/openhuman/channels/context.rs | Same global to connector fan-out change as memory_context.rs for the channels-side turn context path. |
Sequence Diagram
sequenceDiagram
participant Turn as Per-Turn Context
participant Tool as memory_recall Tool
participant AR as auto_recall
participant Facade as recall_through_facade
participant Mem as Memory Store
participant Sweep as vector_reembed sweep
Turn->>AR: "recall_with_connectors(query, limit=5)"
AR->>Facade: recall global (RecallOpts::default())
Facade-->>AR: "Vec<MemoryEntry> (<=5, global)"
AR->>Mem: namespace_summaries()
Mem-->>AR: [skill-gmail:12, skill-slack:3, ...]
AR->>Mem: "recall(skill-gmail, limit=5) concurrent"
AR->>Mem: "recall(skill-slack, limit=5) concurrent"
Mem-->>AR: hits per namespace
AR->>AR: rank_and_truncate(all hits, 5)
AR-->>Turn: "Vec<MemoryEntry> merged <=5"
Tool->>AR: recall_every_namespace(query, limit)
AR->>Facade: recall global
AR->>Mem: "recall ALL skill-* namespaces unbounded"
AR->>AR: rank_and_truncate(all hits, limit)
AR-->>Tool: "Vec<MemoryEntry>"
Note over Sweep: Triggered on startup/login/sync done/embedder switch
Sweep->>Mem: scan_chunks_needing_reembed(64)
Mem-->>Sweep: NULL-vec rows, wrong-dim rows, stale-sig rows
Sweep->>Mem: embed_with_backoff(texts)
Mem-->>Sweep: vectors
Sweep->>Sweep: "check vector.len() == active_dim"
Sweep->>Mem: write_chunk_embedding keyed on text
Reviews (2): Last reviewed commit: "fix(memory): repair signature mismatches..." | Re-trigger Greptile
![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/openhuman/memory/tools/recall.rs (1)
25-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTool
description()wasn't updated to match the new optional-namespace/search-everywhere behavior.The
namespaceparameter's own description was updated to say "OMIT THIS to search everywhere," but the top-level tool description at Line 26 still reads "Search memory for relevant facts in a namespace." Since models weigh both descriptions when choosing arguments, this stale top-level text risks perpetuating exactly the bug this PR fixes — the model defaulting to a namespace instead of omitting it.✏️ Align the tool description with the new default behavior
fn description(&self) -> &str { - "Search memory for relevant facts in a namespace. Returns scored results ranked by relevance." + "Search memory for relevant facts. Omit 'namespace' to search everywhere (global plus connected-app memories); name one only to narrow the search. Returns scored results ranked by relevance." }Also applies to: 39-59
🤖 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/openhuman/memory/tools/recall.rs` around lines 25 - 27, Update the tool description returned by description() and the related parameter documentation in the recall tool to state that namespace is optional: omitting it searches across all namespaces, while providing it restricts results to that namespace. Remove wording that implies searches always occur within a namespace.
🧹 Nitpick comments (1)
src/openhuman/memory/auto_recall.rs (1)
38-86: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGlobal recall and connector-namespace listing run sequentially, not concurrently.
recall_through_facade(...)is fully awaited beforeconnector_namespaces(mem)is even called, even though the two calls are independent. Since the module doc frames the fan-out cost as "roughly one concurrent recall," this ordering adds the namespace-listing latency (and the global recall latency) as a strict prefix to every turn instead of overlapping it.♻️ Run the global recall and namespace listing concurrently
- let mut entries = match crate::openhuman::tinyagents::retriever::recall_through_facade( - mem, - query, - limit, - RecallOpts::default(), - ) - .await - { + let (global_result, namespaces) = futures::future::join( + crate::openhuman::tinyagents::retriever::recall_through_facade( + mem, + query, + limit, + RecallOpts::default(), + ), + connector_namespaces(mem), + ) + .await; + let mut entries = match global_result { Ok(entries) => entries, Err(error) => { tracing::debug!("[memory::auto-recall] global recall failed: {error}"); Vec::new() } }; - - let namespaces = connector_namespaces(mem).await;🤖 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/openhuman/memory/auto_recall.rs` around lines 38 - 86, Update recall_with_connectors so recall_through_facade and connector_namespaces(mem) are started and awaited concurrently rather than awaiting global recall before listing namespaces. Preserve the existing error handling, connector fan-out, ranking, and truncation behavior after both results are available.
🤖 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/openhuman/memory_store/namespace_store/reembed.rs`:
- Around line 213-243: Update write_chunk_embedding to make its UPDATE
conditional on the candidate row remaining unchanged since scanning, using the
candidate’s existing freshness/version or original-row identifying fields in
addition to namespace and chunk_id. Inspect the affected-row count and return a
distinct skipped/stale result when no row was updated, so reembed_pending does
not increment report.reembedded for concurrently changed or deleted rows.
In `@src/openhuman/memory_sync/composio/providers/gmail/sync_executor.rs`:
- Around line 50-58: Update the response-processing flow around
apply_response_level_markdown and post_process so both transformations are
skipped when the action requests raw HTML, using the same raw-HTML predicate as
post_process. Preserve the existing transformations for non-raw-HTML responses,
and add a regression test verifying the raw-HTML payload remains unchanged.
---
Outside diff comments:
In `@src/openhuman/memory/tools/recall.rs`:
- Around line 25-27: Update the tool description returned by description() and
the related parameter documentation in the recall tool to state that namespace
is optional: omitting it searches across all namespaces, while providing it
restricts results to that namespace. Remove wording that implies searches always
occur within a namespace.
---
Nitpick comments:
In `@src/openhuman/memory/auto_recall.rs`:
- Around line 38-86: Update recall_with_connectors so recall_through_facade and
connector_namespaces(mem) are started and awaited concurrently rather than
awaiting global recall before listing namespaces. Preserve the existing error
handling, connector fan-out, ranking, and truncation behavior after both results
are available.
🪄 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: 6d929e16-fb39-4342-9dca-94efaf94e85f
📒 Files selected for processing (26)
src/core/runtime/services.rssrc/openhuman/agent/harness/memory_context.rssrc/openhuman/agent_registry/agents/context_scout/prompt.mdsrc/openhuman/agent_registry/agents/context_scout/prompt.rssrc/openhuman/channels/context.rssrc/openhuman/credentials/ops.rssrc/openhuman/embeddings/rpc.rssrc/openhuman/memory/auto_recall.rssrc/openhuman/memory/auto_recall_tests.rssrc/openhuman/memory/mod.rssrc/openhuman/memory/recall_shaping.rssrc/openhuman/memory/recall_shaping_tests.rssrc/openhuman/memory/sync.rssrc/openhuman/memory/tools/recall.rssrc/openhuman/memory_store/client.rssrc/openhuman/memory_store/mod.rssrc/openhuman/memory_store/namespace_store/mod.rssrc/openhuman/memory_store/namespace_store/reembed.rssrc/openhuman/memory_store/namespace_store/reembed_tests.rssrc/openhuman/memory_store/vector_reembed.rssrc/openhuman/memory_store/vector_reembed_tests.rssrc/openhuman/memory_sync/composio/providers/gmail/mod.rssrc/openhuman/memory_sync/composio/providers/gmail/sync_executor.rssrc/openhuman/memory_sync/composio/providers/gmail/sync_executor_tests.rssrc/openhuman/tinycortex/config.rssrc/openhuman/tinycortex/sync.rs
|
Pushed Signature mismatches in the scan (codex P2) — fixed, with one deliberate exclusion. You were right that this made the Write-back freshness (CodeRabbit) — fixed. The Tool-wide search (codex P2) — fixed.
Tests (against |
coderabbitai
Bot
added
feature
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/openhuman/memory/auto_recall.rs (1)
91-102: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound concurrent connector recalls.
recall_every_namespaceselects every connector namespace, andjoin_allstarts onemem.recalloperation per namespace. If more connector memories exist, these recalls run unbounded and can overload the memory backend or its connection pool. Process batches at a configured limit, or use bounded concurrency. Add a test that records the maximum in-flight recalls, as required for the changed behavior.🤖 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/openhuman/memory/auto_recall.rs` around lines 91 - 102, Update recall_every_namespace to replace unbounded join_all execution with bounded concurrency or configured-size batches when invoking mem.recall across namespaces. Preserve per-namespace result handling, and add a test that tracks concurrent recalls and verifies the configured maximum is never exceeded.Source: Coding guidelines
♻️ Duplicate comments (1)
src/openhuman/memory_store/namespace_store/reembed.rs (1)
235-273: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
write_chunk_embeddingstill reports a skipped write as a success.The
text = ?7guard correctly stops a stale vector from overwriting a re-ingested row. But whenupdated == 0, the function only logs and then falls through toOk(()). Inreembed_pending, the caller treatsOk(())asreport.reembedded += 1. So a row whose write was discarded because the text changed under the sweep is still counted, and reported, as repaired.This does not corrupt the row itself (it is left untouched and reappears on the next scan), but it makes
ReembedSweepReportand the"re-embedded {}/{} pending chunk(s)"log line inaccurate, and any caller that usesreport.reembeddedto judge sweep progress (e.g. deciding whether to schedule another immediate pass) is working from a wrong number.This is the same "0-row update still counted as
report.reembedded += 1" concern raised on this function in an earlier review round. The overwrite-prevention half of that comment is fixed here; this accounting half is not.🐛 Proposed fix: propagate the skip as a failure
if updated == 0 { // The row moved on (re-ingested, or deleted). Nothing to repair from // this pass; if it still needs an embedding the next scan picks up // its current text. tracing::debug!( namespace = %candidate.namespace, chunk_id = %candidate.chunk_id, "[memory][reembed] row changed under the sweep; vector discarded" ); + return Err(format!( + "write_chunk_embedding {}/{}: row changed under the sweep", + candidate.namespace, candidate.chunk_id + )); } Ok(())Please also confirm a test covers this exact path: a row whose
textchanges between scan and write should land inreport.failed, notreport.reembedded.🤖 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/openhuman/memory_store/namespace_store/reembed.rs` around lines 235 - 273, Update write_chunk_embedding so an updated count of zero propagates a failure instead of falling through to Ok(()), while retaining the existing debug log and stale-vector discard behavior. Ensure reembed_pending therefore increments report.failed rather than report.reembedded for rows whose text changes between scan and write, and add or update a test covering this exact path and report accounting.
🧹 Nitpick comments (1)
src/openhuman/memory_store/namespace_store/reembed.rs (1)
262-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent logging macro and prefix.
Every other log call in this file uses
log::(debug!,warn!,info!) with the[memory::reembed]prefix. This new call switches totracing::debug!with a different prefix format,[memory][reembed]. Use the same macro and prefix as the rest of the file for a consistent, greppable log stream for this subsystem.♻️ Proposed fix: align with the file's existing log convention
- tracing::debug!( - namespace = %candidate.namespace, - chunk_id = %candidate.chunk_id, - "[memory][reembed] row changed under the sweep; vector discarded" - ); + log::debug!( + "[memory::reembed] row changed under the sweep for {}/{}; vector discarded", + candidate.namespace, + candidate.chunk_id + );As per coding guidelines, "use
log/tracingatdebug/tracewith stable prefixes and correlation fields for new or changed flows."🤖 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/openhuman/memory_store/namespace_store/reembed.rs` around lines 262 - 271, Update the debug log in the updated == 0 branch to use the file’s existing log::debug! macro and the “[memory::reembed]” prefix, while preserving the namespace and chunk_id correlation fields and message context.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/openhuman/memory/auto_recall.rs`:
- Around line 91-102: Update recall_every_namespace to replace unbounded
join_all execution with bounded concurrency or configured-size batches when
invoking mem.recall across namespaces. Preserve per-namespace result handling,
and add a test that tracks concurrent recalls and verifies the configured
maximum is never exceeded.
---
Duplicate comments:
In `@src/openhuman/memory_store/namespace_store/reembed.rs`:
- Around line 235-273: Update write_chunk_embedding so an updated count of zero
propagates a failure instead of falling through to Ok(()), while retaining the
existing debug log and stale-vector discard behavior. Ensure reembed_pending
therefore increments report.failed rather than report.reembedded for rows whose
text changes between scan and write, and add or update a test covering this
exact path and report accounting.
---
Nitpick comments:
In `@src/openhuman/memory_store/namespace_store/reembed.rs`:
- Around line 262-271: Update the debug log in the updated == 0 branch to use
the file’s existing log::debug! macro and the “[memory::reembed]” prefix, while
preserving the namespace and chunk_id correlation fields and message context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08f87b04-335c-4efb-b25d-760e116ad511
📒 Files selected for processing (8)
src/openhuman/memory/auto_recall.rssrc/openhuman/memory/recall_shaping.rssrc/openhuman/memory/recall_shaping_tests.rssrc/openhuman/memory/tools/recall.rssrc/openhuman/memory_store/namespace_store/reembed.rssrc/openhuman/memory_sync/composio/providers/gmail/post_process.rssrc/openhuman/memory_sync/composio/providers/gmail/sync_executor.rssrc/openhuman/memory_sync/composio/providers/gmail/sync_executor_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/openhuman/memory/recall_shaping_tests.rs
- src/openhuman/memory/tools/recall.rs
- src/openhuman/memory/recall_shaping.rs
A chunk whose batch embed failed is persisted text-only, and a partial cloud failure writes degenerate short vectors; both are invisible to recall, which skips any chunk whose dimension differs from the live query embedding. On a live workspace that left thousands of connector chunks unsearchable with nothing in the system that would ever retry them. Deficient rows are their own durable work-list: the sweep re-derives the pending set from `vector_chunks` on every pass, so recovery survives a restart without a queue to keep in sync. Transient provider failures retry with jittered exponential backoff; unrecoverable ones (auth missing, refused input) end the pass without a tight loop and are retried by the next trigger — which is why logging in is one of them. A returned vector is only written when its width matches the active embedder. Stamping a short vector would satisfy a naive "has an embedding" check while still matching nothing, and — because the row stays pending — would make the sweep rewrite the same row forever. Triggers: startup, sync completion, login, and an embedder switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
a3af436 to
b1a4f93
Compare
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
coderabbitai
Bot
removed
the
feature
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/openhuman/memory/store/namespace_store/reembed.rs (1)
264-274: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA zero-row update is counted as a repair.
write_chunk_embeddingreturnsOk(())whenupdated == 0. Line 170 then incrementsreport.reembeddedfor a row that received no vector.Two consequences follow:
ReembedSweepReport.reembeddedoverstates the repairs. The[memory::reembed] re-embedded {}/{}log and the driver'ssweep repaired {repaired} chunk(s)log both report work that did not land.- The driver in
src/openhuman/memory/store/vector_reembed.rsbreaks its pass loop onreport.reembedded == 0. If a pass writes nothing because every candidate row changed under the sweep,reembeddedis still non-zero, so the loop continues. The next scan is deterministic (same predicate, sameORDER BY updated_at DESC, chunk_id ASC), so it re-selects the same rows and re-embeds them, up toMAX_BATCHES_PER_RUN(64) provider batch calls per trigger.Report the discarded write distinctly so the caller can count it as still-pending.
🔧 Proposed fix: report the discarded write
- ) -> Result<(), String> { + /// + /// Returns `Ok(false)` when the row changed under the sweep and the vector + /// was discarded — not a repair, and not a failure to retry immediately. + fn write_chunk_embedding( + &self, + candidate: &ReembedCandidate, + vector: &[f32], + signature: &str, + now: f64, + ) -> Result<bool, String> {if updated == 0 { // The row moved on (re-ingested, or deleted). Nothing to repair from // this pass; if it still needs an embedding the next scan picks up // its current text. tracing::debug!( namespace = %candidate.namespace, chunk_id = %candidate.chunk_id, "[memory][reembed] row changed under the sweep; vector discarded" ); + return Ok(false); } - Ok(()) + Ok(true) }Then, in
reembed_pending:match self.write_chunk_embedding(candidate, vector, &signature, now) { - Ok(()) => report.reembedded += 1, + Ok(true) => report.reembedded += 1, + // The row changed under the sweep. It is neither repaired nor a + // provider failure; the next scan re-derives its current state. + Ok(false) => report.stale += 1, Err(error) => {Add the field to the report and keep the driver's break condition meaningful:
pub(crate) struct ReembedSweepReport { /// Deficient rows the scan returned this pass (bounded by `budget`). pub scanned: usize, /// Rows given a fresh, usable vector. pub reembedded: usize, /// Rows still without a usable vector after this pass (they stay pending). pub failed: usize, + /// Rows whose text changed between the scan and the write-back, so the + /// computed vector was discarded. + pub stale: usize, }🤖 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/openhuman/memory/store/namespace_store/reembed.rs` around lines 264 - 274, Update write_chunk_embedding and reembed_pending so an updated == 0 write is reported as discarded rather than counted in report.reembedded. Add a distinct discarded-write field to ReembedSweepReport, increment it for rows changed during the sweep, and increment reembedded only when a vector is actually stored; preserve the driver’s zero-repair break behavior so discarded writes remain pending without causing another batch loop.
🧹 Nitpick comments (4)
src/openhuman/agent/registry/agents/context_scout/prompt.rs (1)
210-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete connector-recall contract.
The test accepts any
skill-text. It does not requireskill-gmail. It also does not verify the empty or insufficient-memory fallback. A later prompt regression can pass this test while removing the concrete namespace or blocking live delegation after an unsuccessful recall. Assert the concrete namespace and both live-fetch conditions after the two prompt surfaces use the same wording.🤖 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/openhuman/agent/registry/agents/context_scout/prompt.rs` around lines 210 - 230, Strengthen connected_integrations_block_points_the_scout_at_memory_first by requiring the rendered prompt to mention the concrete skill-gmail namespace and by asserting both fallback conditions: live fetching is allowed when memory_recall is empty and when it lacks sufficient information. Ensure the assertions match the shared wording used by both prompt surfaces, while retaining the memory-first and memory_recall checks.src/openhuman/memory/store/vector_reembed.rs (2)
45-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the two branches that skip the sweep.
Both early returns are silent. If a backlog does not drain, the log shows no reason: there is no record that the trigger found no runtime, or that the memory client was not ready, or that a sweep was already in flight. The domain logging guideline requires debug logging at branches with the stable
[memory::reembed]prefix.📋 Proposed fix: add branch logging
pub fn ensure_vector_reembed() { if tokio::runtime::Handle::try_current().is_err() { // No runtime to spawn onto (unit tests, sync CLI paths). The rows stay // pending, so the next trigger inside the service picks them up. + log::debug!("[memory::reembed] no tokio runtime — sweep not scheduled"); return; } let Some(client) = crate::openhuman::memory::global::client_if_ready() else { + log::debug!("[memory::reembed] memory client not ready — sweep not scheduled"); return; }; if SWEEP_RUNNING.swap(true, Ordering::SeqCst) { + log::debug!("[memory::reembed] a sweep is already in flight — trigger coalesced"); return; } tokio::spawn(async move { let _guard = SweepGuard; + log::debug!("[memory::reembed] sweep started (batch={SWEEP_BATCH}, max_passes={MAX_BATCHES_PER_RUN})"); let mut repaired = 0usize;Based on learnings and coding guidelines: "All Rust domain logic must include debug-level logging with stable grep-friendly prefixes (
[domain],[rpc]), correlation fields, entry/exit points, branches, external calls, retries/timeouts, and state transitions".🤖 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/openhuman/memory/store/vector_reembed.rs` around lines 45 - 56, Update ensure_vector_reembed to emit debug logs with the stable “[memory::reembed]” prefix before each early return for a missing Tokio runtime and an unavailable memory client, and also log when SWEEP_RUNNING indicates a sweep is already in progress. Keep the existing return behavior unchanged.Source: Coding guidelines
60-69: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe pass loop has no pacing between provider calls.
The loop issues up to 64 passes back to back. Each pass embeds up to 64 texts. One trigger can therefore drive about 4,096 embeds with no delay between provider calls.
The comment at lines 18-20 states the sweep "competes gently with live embedding traffic". The batch size bounds one call, but nothing spaces the calls apart, and nothing yields to live embedding traffic between passes. On a large backlog — the PR reports about 2,819 pending chunks — a boot trigger will saturate the embedder while a user is waiting on an interactive recall.
Add a short delay between passes.
♻️ Proposed refactor: space the passes apart
+/// Pause between passes so a large backlog yields the embedder to interactive +/// traffic instead of draining at full rate. +const PASS_INTERVAL: std::time::Duration = std::time::Duration::from_millis(500); +- for _ in 0..MAX_BATCHES_PER_RUN { + for pass in 0..MAX_BATCHES_PER_RUN { + if pass > 0 { + tokio::time::sleep(PASS_INTERVAL).await; + } let report = client.sweep_pending_embeddings(SWEEP_BATCH).await;🤖 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/openhuman/memory/store/vector_reembed.rs` around lines 60 - 69, Update the pass loop in the re-embedding sweep around sweep_pending_embeddings so successful passes are separated by a short asynchronous delay before the next provider call. Keep the existing MAX_BATCHES_PER_RUN limit and early exits for empty or unrepaired batches, and avoid delaying after the loop is about to terminate.src/openhuman/memory/store/namespace_store/reembed_tests.rs (1)
86-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the signature branch of the scan predicate.
The tests exercise
embedding IS NULLanddim <> active. They do not exercise the two signature rules that lines 57-67 ofreembed.rsdocument:
- A row whose
dimmatches but whosemodel_signaturediffers from the active embedder must be pending.- A row whose
model_signatureis NULL and whosedimmatches must NOT be pending.The second rule is the guard against re-embedding the whole legacy corpus. No test currently fails if that clause is removed.
insert_chunkalready writes a signature, so both cases need only a signature parameter.The write-back guard at
reembed.rsline 247 (AND text = ?7) is also untested. A test that mutates a candidate row'stextbetweenscan_chunks_needing_reembedandreembed_pendingwould pin the discard behavior.🧪 Proposed test for the signature rules
#[tokio::test] async fn scan_flags_signature_mismatch_but_not_untagged_legacy_rows() { let tmp = TempDir::new().unwrap(); let memory = UnifiedMemory::new(tmp.path(), Arc::new(DimStub::healthy(8)), None).unwrap(); // Right dimension, wrong embedding space — cosine across the two is // meaningless, so the row is pending. insert_chunk_with_signature( &memory, "skill-gmail", "other-model", 0, "colorado boulder", Some(8), Some("provider=stub;model=old-model;dims=8"), ); // Written before model tagging. Recall accepts it at a matching dimension, // so selecting it would re-embed the legacy corpus to change nothing. insert_chunk_with_signature( &memory, "skill-gmail", "legacy-untagged", 0, "denver colorado", Some(8), None, ); let pending = memory.scan_chunks_needing_reembed(100).unwrap(); let ids: HashSet<&str> = pending.iter().map(|c| c.document_id.as_str()).collect(); assert!( ids.contains("other-model"), "a signature from another embedding space must be pending" ); assert!( !ids.contains("legacy-untagged"), "a NULL signature at the active dimension must never be pending" ); assert_eq!(pending.len(), 1, "exactly the mismatched row is pending"); }🤖 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/openhuman/memory/store/namespace_store/reembed_tests.rs` around lines 86 - 135, Extend the re-embedding tests around scan_chunks_needing_reembed to cover matching-dimension rows with a mismatched model_signature as pending and NULL model_signature as not pending, using insert_chunk_with_signature and asserting the exact pending count. Also add coverage for the reembed_pending write-back guard by changing a candidate row’s text between scanning and reembedding, then verify that stale candidate is discarded.
🤖 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/openhuman/agent/registry/agents/context_scout/prompt.rs`:
- Around line 43-47: The live-delegation condition must match across both prompt
surfaces: update src/openhuman/agent/registry/agents/context_scout/prompt.rs
lines 43-47 to allow delegation when memory lacks the needed information or
freshness is required, and update
src/openhuman/agent/registry/agents/context_scout/prompt.md lines 33-41 to use
the identical condition. Keep the guidance consistent so an unsuccessful
memory_recall does not prevent live delegation.
In `@src/openhuman/inference/embeddings/rpc.rs`:
- Around line 366-372: The sig_changed branch in update_settings must ensure the
global memory client is rebound or recreated with the newly saved Config before
calling ensure_vector_reembed(), so UnifiedMemory.embedder and sweep signature
comparisons use the new provider/model/dimension. Update the relevant
memory::global initialization flow or pass the new configuration through the
vector_reembed path, while preserving the existing backfill call.
In `@src/openhuman/memory/sync/composio/providers/gmail/sync_executor.rs`:
- Around line 57-65: Gate both apply_response_level_markdown and post_process in
sync_executor.rs behind action == "GMAIL_FETCH_EMAILS"; leave non-fetch
responses unchanged. In sync_executor_tests.rs, add a non-fetch fixture
containing messages and markdown_formatted and assert the resulting payload is
preserved byte-for-byte.
- Around line 34-66: Add stable debug-level lifecycle logs in
GmailSyncExecutor::execute for entry, inner execution delegation and outcome,
unsuccessful-response and raw_html bypasses, response-level markdown
application, post-processing, and successful completion, using only safe
correlation fields and never logging arguments, message content, credentials, or
the full connection identifier. In src/openhuman/memory/tinycortex/sync.rs lines
544-548, add a stable prefixed debug event when constructing the Gmail pipeline;
both sites require direct changes.
---
Duplicate comments:
In `@src/openhuman/memory/store/namespace_store/reembed.rs`:
- Around line 264-274: Update write_chunk_embedding and reembed_pending so an
updated == 0 write is reported as discarded rather than counted in
report.reembedded. Add a distinct discarded-write field to ReembedSweepReport,
increment it for rows changed during the sweep, and increment reembedded only
when a vector is actually stored; preserve the driver’s zero-repair break
behavior so discarded writes remain pending without causing another batch loop.
---
Nitpick comments:
In `@src/openhuman/agent/registry/agents/context_scout/prompt.rs`:
- Around line 210-230: Strengthen
connected_integrations_block_points_the_scout_at_memory_first by requiring the
rendered prompt to mention the concrete skill-gmail namespace and by asserting
both fallback conditions: live fetching is allowed when memory_recall is empty
and when it lacks sufficient information. Ensure the assertions match the shared
wording used by both prompt surfaces, while retaining the memory-first and
memory_recall checks.
In `@src/openhuman/memory/store/namespace_store/reembed_tests.rs`:
- Around line 86-135: Extend the re-embedding tests around
scan_chunks_needing_reembed to cover matching-dimension rows with a mismatched
model_signature as pending and NULL model_signature as not pending, using
insert_chunk_with_signature and asserting the exact pending count. Also add
coverage for the reembed_pending write-back guard by changing a candidate row’s
text between scanning and reembedding, then verify that stale candidate is
discarded.
In `@src/openhuman/memory/store/vector_reembed.rs`:
- Around line 45-56: Update ensure_vector_reembed to emit debug logs with the
stable “[memory::reembed]” prefix before each early return for a missing Tokio
runtime and an unavailable memory client, and also log when SWEEP_RUNNING
indicates a sweep is already in progress. Keep the existing return behavior
unchanged.
- Around line 60-69: Update the pass loop in the re-embedding sweep around
sweep_pending_embeddings so successful passes are separated by a short
asynchronous delay before the next provider call. Keep the existing
MAX_BATCHES_PER_RUN limit and early exits for empty or unrepaired batches, and
avoid delaying after the loop is about to terminate.
🪄 Autofix
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: 8cee81c3-41af-444d-b732-b504edeabf76
📒 Files selected for processing (27)
src/core/runtime/services.rssrc/openhuman/agent/harness/memory_context.rssrc/openhuman/agent/registry/agents/context_scout/prompt.mdsrc/openhuman/agent/registry/agents/context_scout/prompt.rssrc/openhuman/channels/context.rssrc/openhuman/inference/embeddings/rpc.rssrc/openhuman/memory/auto_recall.rssrc/openhuman/memory/auto_recall_tests.rssrc/openhuman/memory/mod.rssrc/openhuman/memory/recall_shaping.rssrc/openhuman/memory/recall_shaping_tests.rssrc/openhuman/memory/store/client.rssrc/openhuman/memory/store/mod.rssrc/openhuman/memory/store/namespace_store/mod.rssrc/openhuman/memory/store/namespace_store/reembed.rssrc/openhuman/memory/store/namespace_store/reembed_tests.rssrc/openhuman/memory/store/vector_reembed.rssrc/openhuman/memory/store/vector_reembed_tests.rssrc/openhuman/memory/sync/composio/providers/gmail/mod.rssrc/openhuman/memory/sync/composio/providers/gmail/post_process.rssrc/openhuman/memory/sync/composio/providers/gmail/sync_executor.rssrc/openhuman/memory/sync/composio/providers/gmail/sync_executor_tests.rssrc/openhuman/memory/sync_events.rssrc/openhuman/memory/tinycortex/config.rssrc/openhuman/memory/tinycortex/sync.rssrc/openhuman/memory/tools/recall.rssrc/openhuman/security/credentials/ops.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/openhuman/channels/context.rs
- src/openhuman/memory/recall_shaping_tests.rs
- src/openhuman/memory/mod.rs
- src/openhuman/agent/harness/memory_context.rs
- src/core/runtime/services.rs
- src/openhuman/memory/auto_recall.rs
- src/openhuman/memory/auto_recall_tests.rs
- src/openhuman/memory/recall_shaping.rs
- src/openhuman/memory/tools/recall.rs
`RecallOpts::default()` resolves to `global`, so the automatic turn context never searched connector memories: a synced email could sit in `skill-gmail` and be unreachable from the very turn that asked about it. That scoping is not a decision — it is what an unfinished namespace migration left behind, and connector sync landed on top of it. Recall is namespace-scoped in the store, so widening the scope means fanning out: one recall per namespace, run concurrently, merged by score so a connector hit outranks a weak global one on merit rather than on which namespace it came from. The fan-out is bounded to the busiest few — each namespace costs a query embedding plus a scan — and a store that cannot enumerate namespaces degrades to exactly the previous global-only behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…scores `namespace` was a required parameter, so every recall began with a guess the model usually got wrong: the schema lists `global` first, the model picks it, and connector memories — where synced mail actually lives — are structurally unreachable. A live transcript shows the cost: asked about a university thread sitting in `skill-gmail`, the agent recalled `global`, got back five copies of its own earlier question, and went off to re-search Gmail live. Omitting the namespace now searches global plus connector namespaces and merges by score; naming one keeps the previous scoped behaviour. Results say which namespace a hit came from when it isn't the default, so a connector hit is distinguishable from a chat one. Scores are also rendered as percentages rather than a 0–1 value with a `%` appended — a 0.68 hit printed as "[1%]" told the model everything it found was worthless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Recall returns whole documents, and a document can be large — a synced email thread, or (from an older build) a subagent's entire system-prompt envelope saved as a "conversation". One 22 KB document filled the recall tool's whole result budget, hid every other hit, and pushed the agent to re-search the live source it had just been handed the answer from. A recalled entry is now condensed to the few chunks most relevant to the query — the same chunk unit the store already splits documents into — each trimmed, with a hard per-entry character cap as the backstop for documents stored before any of this (a single un-splittable blob is still bounded). Short facts, the common case, pass through untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…e fetch Asked to summarize the user's Gmail on a topic, the scout emitted its bundle on the first step with no tool calls at all and recommended a live `delegate_to_integrations_agent` fetch — even though those emails are already synced into the `skill-gmail` memory namespace. Re-fetching live is slower, costs a provider round-trip, and is the exact path that was prone to looping. The scout had `memory_recall` and a prompt that listed memory as one option, but nothing told it that connected integrations are mirrored into `skill-<toolkit>` memory — so the "Connected Integrations" block, which framed those platforms as things reachable via live delegation, pulled it straight to a live fetch. Both the archetype prompt and the rendered integrations block now state that a connector's history lives in memory under `skill-<toolkit>` and that `memory_recall` (namespace omitted searches those namespaces too, since the connector-aware recall change) should come first — a live delegation is for the very latest, not-yet-synced items, not a reflex because the platform is wired up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…h every namespace Four review findings, each a case where the sweep or the recall it feeds was narrower than what it promised: - **Signature mismatches are pending work.** Recall skips a chunk whose `model_signature` differs from the active embedder, but the scan selected only NULL/wrong-dimension rows — so a provider or model swap that keeps the dimension scheduled a sweep that repaired nothing, and those rows stayed unreachable until their document was rewritten. A NULL signature is still not pending: recall accepts those rows on dimension alone, so selecting them would re-embed the legacy corpus to change nothing. - **The write-back is keyed on the scanned text.** The embed between scan and write is a network round trip with retries; a document re-ingested in that window would be overwritten with a vector describing the old text, stamped with the ACTIVE dim and signature — indistinguishable from a healthy row and never re-selected. A row that moved on now keeps its own vector. - **`memory_recall` without a namespace searches every namespace.** Its schema says it searches everywhere and the model is told to omit the argument; it was reusing the per-turn helper, which caps at the four busiest connectors. The cap stays where its cost is paid every turn. - **`raw_html` gates both reshape steps.** `post_process` honoured it; the response-level markdown injected before it did not, so the payload a caller asked to keep raw was still rewritten. Also fixes `hard_cap`, which reserved one character for an ellipsis that appends three, returning text two characters over the cap on every truncated entry. Tests: reembed 8, recall_shaping 6, sync_executor 5, auto_recall 4, memory::tools::recall 8 — green against tinycortex#132 checked out locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
b1a4f93 to
539f02c
Compare
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Rebased onto current
Both need a tinycortex change first. Splitting them out is not a scope decision about whether they are right — it is that they cannot land from this repo alone, and holding the other six commits behind them buys nothing. I will raise them against tinycortex and reopen them here once the submodule carries the seam. What remains is the six commits that do compile: the durable base-vector re-embed sweep, connector-namespace auto-recall, recall condensation, real scores across namespaces, the context-scout prompt, and the signature/write-back repairs. Verified locally on the rebased head: |
…hanges The sweep this PR adds keys off the live embedder's signature and dimension, and `UnifiedMemory` resolves its embedder once, at construction. `memory::global::init` short-circuits when the workspace directory is unchanged — correct for its own callers, since an embedder swap changes how memory embeds, not where it lives. So `update_settings` saved the new provider/model/dimension and then started a sweep that still held the old embedder: it scanned for rows missing the OLD signature and wrote vectors back under it. Every row the switch had just made pending stayed pending until the next boot, which is the failure the sweep exists to prevent. Adds `global::rebind_current_workspace`, which rebuilds in place for the already-bound workspace, and calls it before the backfill and sweep. A rebuild failure is logged and both still run — that is no worse than today — and an unbound slot is a no-op, since the next `init` reads the saved config anyway. Tests: rebind installs a different `Arc` for the same workspace where `init` returns the bound one, and is a no-op with nothing bound. memory::global 8 pass. Reported by CodeRabbit on tinyhumansai#5258. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/openhuman/memory/global.rs`:
- Around line 124-156: Add structured [domain] debug telemetry to
rebind_current_workspace/rebind_in_slot covering entry, no-bound-client
branching, rebuild start, MemoryClient construction result, slot replacement,
and exit; include a non-PII workspace correlation field and preserve existing
error handling. In src/openhuman/memory/global.rs lines 124-156, update the
rebind lifecycle accordingly. In src/openhuman/inference/embeddings/rpc.rs lines
366-384, add structured [rpc] debug events for saved-signature transition,
rebind result, backfill scheduling, and sweep scheduling, while retaining the
rebuild-failure warning.
- Around line 148-155: Update rebind_in_slot to revalidate the slot under the
write lock before assigning the newly built GlobalMemoryClient, ensuring the
current binding still matches the snapped workspace. If another binding replaced
it, retry using that newer workspace; if the slot is empty, return Ok(None), and
add a concurrent regression test covering this interleaving.
In `@src/openhuman/memory/recall_shaping.rs`:
- Around line 40-100: Update condense_with to add debug-level tracing with a
stable “[domain]” prefix and a correlation field from the caller or tracing
context at entry and exit, plus bypass, empty-content, selection, dropped-chunk,
and hard-cap branches. Log only counts and character lengths; never include
query, content, or rendered memory text. Preserve the existing condensation
behavior while ensuring each requested branch emits the appropriate structured
debug event.
- Around line 79-84: Update the ranked selection logic in the recall-shaping
flow to retain only chunks with positive relevance scores whenever any such
matches exist, rather than filling max_chunks with zero-score entries. Preserve
the existing fallback selection behavior only when no positive-score chunk is
available, and keep the final index ordering unchanged.
🪄 Autofix
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: efbdb7b5-57fc-46a6-b9af-06cdd2b10aaa
📒 Files selected for processing (23)
src/core/runtime/services.rssrc/openhuman/agent/harness/memory_context.rssrc/openhuman/agent/registry/agents/context_scout/prompt.mdsrc/openhuman/agent/registry/agents/context_scout/prompt.rssrc/openhuman/channels/context.rssrc/openhuman/inference/embeddings/rpc.rssrc/openhuman/memory/auto_recall.rssrc/openhuman/memory/auto_recall_tests.rssrc/openhuman/memory/global.rssrc/openhuman/memory/mod.rssrc/openhuman/memory/recall_shaping.rssrc/openhuman/memory/recall_shaping_tests.rssrc/openhuman/memory/store/client.rssrc/openhuman/memory/store/mod.rssrc/openhuman/memory/store/namespace_store/mod.rssrc/openhuman/memory/store/namespace_store/reembed.rssrc/openhuman/memory/store/namespace_store/reembed_tests.rssrc/openhuman/memory/store/vector_reembed.rssrc/openhuman/memory/store/vector_reembed_tests.rssrc/openhuman/memory/sync/composio/providers/gmail/post_process.rssrc/openhuman/memory/sync_events.rssrc/openhuman/memory/tools/recall.rssrc/openhuman/security/credentials/ops.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- src/openhuman/memory/store/mod.rs
- src/openhuman/memory/store/namespace_store/mod.rs
- src/openhuman/memory/sync_events.rs
- src/openhuman/agent/registry/agents/context_scout/prompt.md
- src/openhuman/memory/store/client.rs
- src/openhuman/channels/context.rs
- src/openhuman/memory/tools/recall.rs
- src/openhuman/agent/registry/agents/context_scout/prompt.rs
- src/openhuman/memory/store/vector_reembed_tests.rs
- src/openhuman/memory/mod.rs
- src/openhuman/agent/harness/memory_context.rs
- src/core/runtime/services.rs
- src/openhuman/security/credentials/ops.rs
- src/openhuman/memory/sync/composio/providers/gmail/post_process.rs
- src/openhuman/memory/auto_recall_tests.rs
- src/openhuman/memory/store/vector_reembed.rs
- src/openhuman/memory/auto_recall.rs
- src/openhuman/memory/recall_shaping_tests.rs
- src/openhuman/memory/store/namespace_store/reembed.rs
- src/openhuman/memory/store/namespace_store/reembed_tests.rs
The rebind released the read lock to build its client and then installed unconditionally. An `init` for a different workspace — the post-login active-user switch `init_in_slot` exists to serve — can land in that window, and the rebind would then point the global at the workspace it *started* from, so every subsequent write went to the previous user's store. It now re-checks under the write lock and stands down if the binding moved, returning `Ok(None)`: the newer binding is the correct one, and the embedder change that triggered the rebind will be read by whatever built it. Split `rebind_in_slot_for` out so the recheck is exercised directly rather than through a race the test would have to win. memory::global 9 pass. Reported by CodeRabbit on tinyhumansai#5258. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
`condense_with` took the top `max_chunks` by relevance regardless of score, so with fewer matches than slots the leftovers were filled by whatever sorted next — which, once anything matches, is unrelated text. The model then read a passage the query never asked for beside one it did, inside a feature whose whole point is to hand back only the part that answers. Now: if any chunk matches, only matching chunks are kept. If none match, the head of the document is still the best available answer, so that fallback stays. Adds the debug-logging contract to both new domain paths — `condense_with` (entry, each exit branch, kept/dropped counts) and the rebind lifecycle (entry, nothing-bound, build-failed, installed). Counts and lengths only: the input here is recalled memory, so its content must not reach the log. Tests: unmatched sections no longer fill the remaining slots, and the no-match fallback still returns the head. recall_shaping 8, memory::global 9 pass. Reported by CodeRabbit on tinyhumansai#5258. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
Layered fixes to why connector-synced memories (a Gmail thread) were unretrievable, each confirmed against a live instance.
feat(memory)durable re-embed sweep — a chunk whose batch embed failed is stored text-only, and a partial cloud failure writes degenerate short vectors; both are invisible to recall (which drops any chunk whose dimension differs from the live query). Deficientvector_chunksrows are their own durable work-list: the sweep re-derives the pending set every pass, so recovery survives a restart without a queue. Transient failures retry with jittered exponential backoff; unrecoverable ones (auth missing) end the pass and are retried by the next trigger — which is why logging in is one. A returned vector is only written when its width matches the active embedder, so a degraded provider can't stamp a short vector that stays pending forever. Triggers: startup, sync completion, login, embedder switch.fix(memory-sync)restore the Gmail response reshape — the reshape that slims a verbose payload into one record per message lost its caller in the TinyCortex engine migration, so the sync had been storing raw provider JSON. Restored via aReshapingExecutorthat wraps the executor the pipeline fetches through.fix(tinycortex)carry the embedding provider into the tree config — so the tree's active signature names the same provider the namespace store's embedder does.feat(memory)auto-recall searches connector namespaces —RecallOpts::default()resolved toglobalonly, so the per-turn context never searched connector memories. It now fans out toglobalplus the busiestskill-*namespaces, merged by score, bounded so an unbounded connector store doesn't turn every turn into a full scan.fix(memory)memory_recall: search everywhere + real scores —namespacewas required, so the model guessedglobaland connector memories were unreachable; it is now optional (omitted → search everywhere). Scores render as real percentages (a 0.68 hit was printing as[1%]), and hits name their namespace.fix(memory)condense recalled documents — recall returns whole documents, and one large document (a synced thread, or an old subagent envelope) filled the entire tool result and buried every other hit. An entry is condensed to the few chunks most relevant to the query, each trimmed, with a hard per-entry char cap.fix(context-scout)recall connector memory before a live fetch — the scout emitted its bundle with no tool calls and recommended a live Gmail delegation even though the emails are inskill-gmail. Its prompt now states connector history lives in memory underskill-<toolkit>and to recall it first; a live delegation is for the very latest, not-yet-synced items.Tests
openhuman::memory(serial) 1276, plus memory_store / memory_sync / channels / credentials / embeddings / tinycortex suites — green. New coverage across the re-embed sweep (incl. degraded-provider guard), connector fan-out, recall search-all + scores, recall condensation, the reshaping executor, and the scout memory-first prompt.Validation (live)
Re-embed sweep repaired ~2819 pending chunks (pending → 0); a Colorado-email query now retrieves the real
skill-gmailthreads with canonicalised Korean bodies, condensed to relevant chunks with real percentages; and the context scout callsmemory_recallfirst instead of a live fetch.Summary by CodeRabbit
Closes #5300