Skip to content

feat(orchestrator): wire memory-tree retrieval tools with periodic prefetch#1027

Merged
senamakel merged 3 commits into
tinyhumansai:mainfrom
sanil-23:feat/orchestrator-tree-retrieval
Apr 29, 2026
Merged

feat(orchestrator): wire memory-tree retrieval tools with periodic prefetch#1027
senamakel merged 3 commits into
tinyhumansai:mainfrom
sanil-23:feat/orchestrator-tree-retrieval

Conversation

@sanil-23
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 commented Apr 29, 2026

Summary

  • Six new LLM-callable Tools (memory_tree_{search_entities, query_topic, query_source, query_global, drill_down, fetch_leaves}) wrapping the existing Phase 4 retrieval RPCs so the orchestrator agent can answer questions about ingested email / chat / document memory.
  • TreeContextLoader injects the 7-day cross-source digest into the orchestrator's turn context on the first turn AND every REFRESH_INTERVAL (30 min) thereafter, so long-running sessions stay current with newly-ingested memory. Each injection rides on the user message (NOT the system prompt) to keep the KV-cache prefix stable.
  • Orchestrator system prompt grows two sections: tool-selection guidance (when to call which retrieval tool) and a citation-marker convention ([^N] + footnote with node_id and source_ref).
  • New tests/agent_retrieval_e2e.rs exercises the wrapper pipeline against a real ingest fixture, plus a static check that agent.toml exposes all six tool names.
  • about_app capability catalog gains intelligence.memory_tree_retrieval (Beta, LOCAL_RAW privacy).

Problem

Phase 4 (#710) shipped six JSON-RPC retrieval primitives over the hierarchical memory tree, but they're only reachable from external callers. The orchestrator agent — the user-facing chat front — has no way to call them, so it can't answer questions like "what emails did I get from Anthropic this week?" against the user's own ingested memory. The agent dispatches to the old episodic memory_recall only.

Solution

  • Thin Tool adapters in src/openhuman/tools/impl/memory/tree/. Each wrapper deserialises args into the matching *Request struct from retrieval/rpc.rs, loads config via config_rpc::load_config_with_timeout, calls the typed retrieval::* function (not _rpc variants — keeps tool output free of the CLI log envelope), and returns ToolResult::success(json).
  • Registration in tools/ops.rs::all_tools_with_runtime next to the existing memory tools, and in the orchestrator's agent.toml named=[…].
  • Eager + periodic refresh (TreeContextLoader): pure should_prefetch(last, now, interval) helper isolates the cadence decision from Instant::now() so it stays deterministically testable. Session struct gains last_tree_prefetch_at: Option<Instant>, bumped on every successful load (including empty digests, so empty workspaces don't get re-queried every turn). Failures are non-fatal — bare context returned on any error.
  • Citation convention in prompt.md: LLM emits [^N] markers + footnote block with node_id + source_ref from each RetrievalHit. UI rendering is deferred — markers stay as plaintext until that lands.
  • Top-down expansion is the cost-control story called out in the prompt: start with cheap query_* summaries; only call drill_down / fetch_leaves when the user wants details or a verbatim quote. Live testing observed exactly this pattern.

Verification

End-to-end smoke against the developer's populated .openhuman/workspace:

  • cargo run --bin openhuman-core -- call --method openhuman.local_ai_agent_chat --params '{"message":"Who is the most frequent email sender across my inbox? Only check via the memory tree retrieval tools."}'
  • Session JSONL trace shows all six tools dispatched in a single turn (21 total dispatches: 5 query_source, 4 search_entities, 4 drill_down, 3 query_topic, 3 query_global, 2 fetch_leaves). The LLM correctly reasoned about response fields (tree_scope, child_ids) and produced an accurate answer ranking notifications@github.com (27) > no-reply@otter.ai (5) by mention count.
  • cargo test --test agent_retrieval_e2e: 2/2 pass
  • cargo test --lib agent::tree_loader: 5/5 pass (4 new should_prefetch cases + the empty-workspace case)
  • cargo test --lib memory::tree::retrieval: 70/70 pass (no regressions)
  • cargo fmt --check + cargo clippy --all-targets -- -D warnings: zero warnings introduced in changed files.

Submission Checklist

  • Unit testscargo test --lib agent::tree_loader (5/5) covers the empty-workspace path and the full should_prefetch state machine. Per-tool wrappers are covered transitively by the integration test.
  • E2E / integrationtests/agent_retrieval_e2e.rs ingests a real email fixture and exercises the full tool-wrapper → typed retrieval → serialised result pipeline; static test asserts agent.toml exposes all six tool names.
  • Doc comments//! headers on every new module; // rationale comments on the KV-cache invariant in turn.rs and the typed-vs-_rpc choice in each tool wrapper.
  • Inline comments — Used sparingly per CLAUDE.md guidance; only the non-obvious bits (KV-cache invariant, graceful empty-tree no-op, timestamp-bump-on-empty rationale).

Impact

  • Runtime: desktop only (matches the rest of openhuman).
  • Performance: First-turn prefetch adds one query_global call (cached for 30 min); each refresh adds at most ~1.5 KB of context. No background work added.
  • Privacy: All retrieval is local against the user's own workspace; no new external calls. Capability catalog entry uses LOCAL_RAW privacy.
  • Migration: None — purely additive on existing schemas.

Related

  • Closes: (no upstream issue — feature work, see PR description for context)
  • Follow-up TODOs:
    • Citation markers ([^N]) are absent in current LLM output; needs prompt iteration with concrete anti-hallucination examples.
    • The orchestrator's full tool surface includes ~22 tools that aren't in named=[…] — pre-existing condition unrelated to this PR but worth investigating whether named is supposed to be authoritative.
    • query_global returns 0 against a freshly-ingested workspace because global_tree::digest::end_of_day_digest daily-digest job hasn't run; the loader handles this gracefully but the prefetch is a no-op until the digest pipeline fires.

Summary by CodeRabbit

  • New Features

    • Added Memory Tree Retrieval (chat) capability for ingested emails, chats, and docs.
    • New memory-tree tools: entity search, topic/source/global queries, drill-down, and leaf fetching; made available to the orchestrator.
    • Orchestration guidance and inline citation format for retrieved hits.
  • Behavior

    • Eager, rate-limited background prefetch of a compact memory-tree digest with session tracking.
  • Tests

    • End-to-end tests validating retrieval tools and orchestrator integration.

…efetch

Six Tool wrappers expose the existing Phase 4 memory-tree retrieval
RPCs to the orchestrator agent so chat can answer questions about
ingested email/chat/document memory:

- memory_tree_search_entities — resolve names to canonical entity ids
- memory_tree_query_topic — entity-scoped retrieval across every tree
- memory_tree_query_source — filter by source kind + time window
- memory_tree_query_global — cross-source daily digest
- memory_tree_drill_down — expand a summary node's children
- memory_tree_fetch_leaves — batch-hydrate raw chunks for citations

TreeContextLoader pre-loads the 7-day digest into the orchestrator's
turn context on session start AND every REFRESH_INTERVAL (30 min)
thereafter, so long-running conversations stay current with new
ingest. Each injection rides on the user message (NOT the system
prompt) to keep the KV-cache prefix stable. A pure should_prefetch
helper keeps the cadence decision deterministically testable.

The orchestrator system prompt grows two sections: tool-selection
guidance (when to call which retrieval tool) and a citation-marker
convention ([^N] + footnote with node_id and source_ref).

Tests:
- tests/agent_retrieval_e2e.rs (2 tests including a real
  ingest-pipeline round trip on alice/phoenix data) covers the full
  deserialise -> typed retrieval -> serialise wrapper pipeline.
- TreeContextLoader unit tests (5: empty-workspace + 4 covering the
  should_prefetch state machine).
- Static check asserting agent.toml exposes all six tool names.

End-to-end live verification on the developer's populated
.openhuman/workspace: a single agent_chat turn dispatched all six
tools (21 total dispatches: 5 query_source, 4 search_entities, 4
drill_down, 3 query_topic, 3 query_global, 2 fetch_leaves) and
produced a correct sender-frequency answer ranked by mention count.

about_app capability catalog gains intelligence.memory_tree_retrieval
(Beta, LOCAL_RAW privacy).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sanil-23 sanil-23 requested a review from a team April 29, 2026 16:12
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

📝 Walkthrough

Walkthrough

Adds a memory-tree retrieval feature: six new LLM-facing memory-tree tools and exports, orchestrator prompt/config updates to use them, eager tree-digest prefetching during agent turns with session timing, a TreeContextLoader implementation, capability catalog entry, and an end-to-end retrieval test.

Changes

Cohort / File(s) Summary
Capability Registration
src/openhuman/about_app/catalog.rs
Adds intelligence.memory_tree_retrieval (Beta, LOCAL_RAW) to static CAPABILITIES.
Orchestrator Config & Prompt
src/openhuman/agent/agents/orchestrator/agent.toml, src/openhuman/agent/agents/orchestrator/prompt.md
Exposes six memory_tree_* tools to the orchestrator and adds orchestration instructions for entity resolution, scoped queries, drill-down, leaf fetching, and citation formatting.
Agent Session State & Turn Logic
src/openhuman/agent/harness/session/types.rs, src/openhuman/agent/harness/session/builder.rs, src/openhuman/agent/harness/session/turn.rs
Adds last_tree_prefetch_at to session, initializes it, and implements conditional, non-fatal eager prefetching of tree context with refresh-interval debouncing and context concatenation on success.
Tree Loader Module
src/openhuman/agent/mod.rs, src/openhuman/agent/tree_loader.rs
Exports tree_loader; introduces TreeContextLoader with constants (7-day window, 30-min refresh), should_prefetch, async load querying global digest and formatting a compact context; includes tests.
Memory Tree Tools (impl & exports)
src/openhuman/tools/impl/memory/mod.rs, src/openhuman/tools/impl/memory/tree/mod.rs, src/openhuman/tools/impl/memory/tree/*.rs
Adds new memory/tree submodule, re-exports, and six Tool implementations: search_entities, query_topic, query_source, query_global, drill_down, fetch_leaves (parameter schemas, config loading, retrieval calls, logging, JSON result serialization).
Tool Registry
src/openhuman/tools/ops.rs
Registers the six new memory-tree tools in the runtime tool list.
End-to-End Test
tests/agent_retrieval_e2e.rs
New e2e test verifying orchestrator tool exposure, ingesting an EmailThread into memory-tree, resolving entity, topic/global query results (including citation metadata), and leaf hydration via tool wrappers.

Sequence Diagram

sequenceDiagram
    participant Agent as Agent::turn
    participant MemLoader as MemoryLoader
    participant TreeLoader as TreeContextLoader
    participant ConfigRPC as ConfigRPC (load_config_with_timeout)
    participant Retrieval as Retrieval Backend
    participant LLM as LLM Context

    Agent->>MemLoader: load_context()
    MemLoader-->>Agent: memory_context

    Agent->>TreeLoader: should_prefetch(last_tree_prefetch_at, now)?
    TreeLoader-->>Agent: boolean

    alt prefetch due
        Agent->>ConfigRPC: load_config_with_timeout()
        ConfigRPC-->>Agent: Config
        Agent->>TreeLoader: load(config)
        TreeLoader->>Retrieval: query_global(window_days=7)
        Retrieval-->>TreeLoader: hits
        TreeLoader-->>Agent: tree_ctx (formatted digest)
        Agent->>Agent: set last_tree_prefetch_at(now)
        alt tree_ctx non-empty
            Agent->>LLM: inject tree_ctx into context
        end
    else prefetch skipped
        Agent->>Agent: use existing context
    end

    Agent-->>LLM: proceed with turn using enriched context
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • senamakel

"🐰 I hopped through folders, found a tree,
Six little tools to fetch and see,
I prefetch crumbs and tuck them tight,
Citations sparkle in the night,
Agents remember — what a spree! 🥕"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: adding memory-tree retrieval tools with periodic prefetch capability to the orchestrator agent, which aligns with the substantial feature additions across the codebase.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Review rate limit: 4/5 reviews remaining, refill in 12 minutes.

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/openhuman/agent/agents/orchestrator/prompt.md (1)

127-132: ⚠️ Potential issue | 🟠 Major

Restrict verbatim quotes to raw leaf chunks, not generic retrieval hits.

Current wording allows quoting from any hit content. Topic/source/global hits can be summaries, so this can present paraphrases as verbatim quotes.

✏️ Proposed prompt fix
-Inline marker `[^N]` and a numbered footnote at the end carrying the node_id and source_ref from the RetrievalHit. Do not invent quotes — only quote text that appears verbatim in a hit's `content` field.
+Inline marker `[^N]` and a numbered footnote at the end carrying the node_id and source_ref from the RetrievalHit. Do not invent quotes. Only use verbatim quotes from `memory_tree_fetch_leaves` chunk `content` (raw leaf text), and cite that chunk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/agents/orchestrator/prompt.md` around lines 127 - 132,
The prompt currently allows verbatim quotes from any RetrievalHit `content`;
update the wording around the inline marker `[^N]` so that verbatim quotes are
only allowed when the hit is a raw leaf chunk (e.g., check hit.type == "leaf" or
hit.is_leaf / retrievalHit.kind == "leaf_chunk" in your system) and forbid
quoting from topic/source/global summary hits; explicitly state that for
non-leaf hits you must paraphrase or cite but not use verbatim quotation, and
ensure the example and the inline-footnote format `[^N]: node_id · source_ref`
remain unchanged.
🧹 Nitpick comments (1)
src/openhuman/agent/tree_loader.rs (1)

74-83: Align log prefixes with the repo’s standard tokens.

These logs currently use [memory_tree]; the guideline requires stable grep-friendly prefixes like [domain], [rpc], or [ui-flow]. Consider switching to one of those (and keep correlation fields in the message body as needed).

As per coding guidelines src/openhuman/**/*.rs: "Log ... with stable grep-friendly prefixes ([domain], [rpc], [ui-flow]) and correlation fields."

Also applies to: 88-89, 107-110

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/tree_loader.rs` around lines 74 - 83, Replace the
non-standard log prefix "[memory_tree]" with a repo-approved, grep-friendly
prefix (e.g. "[domain]") in the tree loader's logging calls; update the log
statements inside the async load path (the match on query_global in
tree_loader.load) and the other occurrences noted (the warnings and debug/info
around the query_global error handling and subsequent blocks) to use the chosen
prefix while preserving the existing correlation fields (window_days, error
details, etc.) in the message body so grepability and context remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/tools/impl/memory/tree/drill_down.rs`:
- Around line 33-35: The schema for the "max_depth" parameter currently allows 0
but the tool semantics require at least 1; update the JSON schema for
"max_depth" (the properties named "max_depth" in this file) to use "minimum": 1
instead of 0, and add a defensive check in the DrillDownTool's execute method to
reject or error when max_depth <= 0 (or specifically == 0) before proceeding;
reference the "max_depth" schema entries and the execute function in this file
to locate both the schema change and the runtime guard to enforce the new bound.

In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs`:
- Around line 29-30: The doc says chunk_ids are capped at 20 but the handler
forwards any length; modify the request-handling code in fetch_leaves.rs to
enforce that cap at the tool boundary by checking the chunk_ids vector (the
chunk_ids variable) before forwarding to the downstream call (the
hydrate/hydrate_leaves or similar client method). If chunk_ids.len() > 20,
return a clear error/BadRequest result (or trim to 20 if policy prefers) instead
of forwarding; ensure the check is placed in the public handler function (e.g.,
the function that receives the request and calls the client) so the documented
contract is always honored.

In `@src/openhuman/tools/impl/memory/tree/search_entities.rs`:
- Around line 47-48: The runtime default for the "limit" query param doesn't
match the documented default of 5; find where the code reads the limit (e.g. the
variable/field named limit, likely via params.limit.unwrap_or(0) inside the
search_entities handler/function) and change it to use unwrap_or(5) and clamp it
to 100 (e.g. take min(100) after defaulting) so the effective default and the
schema/description ("Max matches (default 5, clamped to 100).") align.

---

Outside diff comments:
In `@src/openhuman/agent/agents/orchestrator/prompt.md`:
- Around line 127-132: The prompt currently allows verbatim quotes from any
RetrievalHit `content`; update the wording around the inline marker `[^N]` so
that verbatim quotes are only allowed when the hit is a raw leaf chunk (e.g.,
check hit.type == "leaf" or hit.is_leaf / retrievalHit.kind == "leaf_chunk" in
your system) and forbid quoting from topic/source/global summary hits;
explicitly state that for non-leaf hits you must paraphrase or cite but not use
verbatim quotation, and ensure the example and the inline-footnote format `[^N]:
node_id · source_ref` remain unchanged.

---

Nitpick comments:
In `@src/openhuman/agent/tree_loader.rs`:
- Around line 74-83: Replace the non-standard log prefix "[memory_tree]" with a
repo-approved, grep-friendly prefix (e.g. "[domain]") in the tree loader's
logging calls; update the log statements inside the async load path (the match
on query_global in tree_loader.load) and the other occurrences noted (the
warnings and debug/info around the query_global error handling and subsequent
blocks) to use the chosen prefix while preserving the existing correlation
fields (window_days, error details, etc.) in the message body so grepability and
context remain intact.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 10419be4-7222-4a50-9897-99c87bc3a34f

📥 Commits

Reviewing files that changed from the base of the PR and between b11b8f3 and 9e10a4a.

📒 Files selected for processing (18)
  • src/openhuman/about_app/catalog.rs
  • src/openhuman/agent/agents/orchestrator/agent.toml
  • src/openhuman/agent/agents/orchestrator/prompt.md
  • src/openhuman/agent/harness/session/builder.rs
  • src/openhuman/agent/harness/session/turn.rs
  • src/openhuman/agent/harness/session/types.rs
  • src/openhuman/agent/mod.rs
  • src/openhuman/agent/tree_loader.rs
  • src/openhuman/tools/impl/memory/mod.rs
  • src/openhuman/tools/impl/memory/tree/drill_down.rs
  • src/openhuman/tools/impl/memory/tree/fetch_leaves.rs
  • src/openhuman/tools/impl/memory/tree/mod.rs
  • src/openhuman/tools/impl/memory/tree/query_global.rs
  • src/openhuman/tools/impl/memory/tree/query_source.rs
  • src/openhuman/tools/impl/memory/tree/query_topic.rs
  • src/openhuman/tools/impl/memory/tree/search_entities.rs
  • src/openhuman/tools/ops.rs
  • tests/agent_retrieval_e2e.rs

Comment thread src/openhuman/tools/impl/memory/tree/drill_down.rs
Comment thread src/openhuman/tools/impl/memory/tree/fetch_leaves.rs
Comment thread src/openhuman/tools/impl/memory/tree/search_entities.rs
- drill_down: schema `max_depth.minimum` 0 -> 1 to match the description
  ("one step or more"); reject `Some(0)` at runtime defensively.
- fetch_leaves: truncate `chunk_ids` to first 20 at the tool boundary
  to honour the documented "max 20 per call" cap, matching the silent-
  truncate behaviour the schema describes.
- search_entities: default `limit` was `unwrap_or(0)` which doesn't
  match the documented "default 5, clamped to 100" — fix to
  `unwrap_or(5).min(100)`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/openhuman/tools/impl/memory/tree/fetch_leaves.rs (1)

29-39: Encode the 20-id cap directly in JSON schema.

The description says “capped at 20,” but the schema doesn’t enforce it. Adding maxItems helps tool-call validation and reduces oversized calls from the model.

Suggested schema tweak
             "chunk_ids": {
                 "type": "array",
                 "items": {"type": "string"},
+                "maxItems": 20,
                 "description": "Chunk ids to hydrate. Capped at 20 per call."
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs` around lines 29 - 39,
The JSON schema for the request body (the json!({...}) literal that contains the
"chunk_ids" property) doesn't enforce the 20-item cap described; update the
"chunk_ids" property to include "maxItems": 20 (you can also add "minItems": 1
if desired) so the schema enforces the limit and prevents oversized calls from
the model.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs`:
- Around line 43-60: Replace the ad-hoc log prefixes `[tool][memory_tree]` with
the repo-standard prefix family (e.g., `[rpc][memory_tree]`) and include stable
correlation fields in each log call: when the function is invoked log the
requested_ids count (`req.chunk_ids.len()`), when truncating log `requested_ids`
and `truncated_to` (`take`), and when returning log `hits` (`hits.len()`);
update the three log::debug! calls in this file (the invocation, truncation
branch, and returning hits) to emit these grep-friendly fields so downstream
tooling can correlate requests.

---

Nitpick comments:
In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs`:
- Around line 29-39: The JSON schema for the request body (the json!({...})
literal that contains the "chunk_ids" property) doesn't enforce the 20-item cap
described; update the "chunk_ids" property to include "maxItems": 20 (you can
also add "minItems": 1 if desired) so the schema enforces the limit and prevents
oversized calls from the model.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 85a5d1e4-02f2-46a7-8f77-9031517ffdae

📥 Commits

Reviewing files that changed from the base of the PR and between 9e10a4a and 9fecd77.

📒 Files selected for processing (3)
  • src/openhuman/tools/impl/memory/tree/drill_down.rs
  • src/openhuman/tools/impl/memory/tree/fetch_leaves.rs
  • src/openhuman/tools/impl/memory/tree/search_entities.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/openhuman/tools/impl/memory/tree/drill_down.rs
  • src/openhuman/tools/impl/memory/tree/search_entities.rs

Comment thread src/openhuman/tools/impl/memory/tree/fetch_leaves.rs Outdated
…eaves logs

CodeRabbit follow-up: align log lines with CLAUDE.md convention
([domain] / [rpc] / [ui-flow] grep-friendly prefixes plus correlation
fields). Surfaces requested_ids / truncated_to / hits counts so a
fetch_leaves invocation can be traced end-to-end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/openhuman/tools/impl/memory/tree/fetch_leaves.rs (1)

32-39: Encode the 20-id cap directly in the JSON schema.

Line 35 documents the limit, but the schema doesn’t enforce it. Add maxItems so tool callers and validators get the constraint upfront.

Suggested diff
             "properties": {
                 "chunk_ids": {
                     "type": "array",
                     "items": {"type": "string"},
+                    "maxItems": MAX_CHUNK_IDS_PER_CALL,
                     "description": "Chunk ids to hydrate. Capped at 20 per call."
                 }
             },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs` around lines 32 - 39,
The JSON schema for the "chunk_ids" array in fetch_leaves.rs documents a 20-id
cap but doesn't enforce it; update the schema object that defines the
"chunk_ids" property to include "maxItems": 20 (i.e., add maxItems to the
"chunk_ids" property's JSON schema alongside "type" and "items") so callers and
validators will enforce the limit (you can leave the description as-is or adjust
it to match the enforced constraint).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs`:
- Around line 60-65: The call to retrieval::fetch_leaves and the
serde_json::to_string call return errors that are currently propagated with "?"
without any RPC-prefixed logs or contextual wrapping; add log::error calls with
a stable prefix like "[rpc][memory_tree]" and correlation fields from the
incoming request (e.g., any request_id/trace_id on req) and chunk info
(chunk_ids.len() and take) when these operations fail, and wrap the errors with
additional context before returning (e.g., use anyhow::Context or map_err to
attach "rpc memory_tree: fetch_leaves failed" and "rpc memory_tree: serialize
hits failed") so callers and logs include grep-friendly prefixes and useful
correlation details for retrieval::fetch_leaves and serde_json::to_string
failures.

---

Nitpick comments:
In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs`:
- Around line 32-39: The JSON schema for the "chunk_ids" array in
fetch_leaves.rs documents a 20-id cap but doesn't enforce it; update the schema
object that defines the "chunk_ids" property to include "maxItems": 20 (i.e.,
add maxItems to the "chunk_ids" property's JSON schema alongside "type" and
"items") so callers and validators will enforce the limit (you can leave the
description as-is or adjust it to match the enforced constraint).
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: d131ebb5-5a41-43d1-be04-bab6ab06377b

📥 Commits

Reviewing files that changed from the base of the PR and between 9fecd77 and 72b4458.

📒 Files selected for processing (1)
  • src/openhuman/tools/impl/memory/tree/fetch_leaves.rs

Comment on lines +60 to +65
let hits = retrieval::fetch_leaves(&cfg, &req.chunk_ids[..take]).await?;
log::debug!(
"[rpc][memory_tree] fetch_leaves completed hits={}",
hits.len()
);
let json = serde_json::to_string(&hits)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Log and wrap downstream failures with RPC-prefixed context.

Line 60 and Line 65 propagate errors without a grep-friendly error log/correlation fields. Add contextual logging before returning errors.

Suggested diff
-        let hits = retrieval::fetch_leaves(&cfg, &req.chunk_ids[..take]).await?;
+        let hits = retrieval::fetch_leaves(&cfg, &req.chunk_ids[..take])
+            .await
+            .map_err(|e| {
+                log::debug!(
+                    "[rpc][memory_tree] fetch_leaves failed requested_ids={} err={}",
+                    take,
+                    e
+                );
+                anyhow::anyhow!("memory_tree_fetch_leaves: retrieval failed: {e}")
+            })?;
         log::debug!(
             "[rpc][memory_tree] fetch_leaves completed hits={}",
             hits.len()
         );
-        let json = serde_json::to_string(&hits)?;
+        let json = serde_json::to_string(&hits).map_err(|e| {
+            log::debug!(
+                "[rpc][memory_tree] fetch_leaves serialize_failed hits={} err={}",
+                hits.len(),
+                e
+            );
+            anyhow::anyhow!("memory_tree_fetch_leaves: serialize failed: {e}")
+        })?;

As per coding guidelines: "Log entry/exit, branches, external calls, retries/timeouts, state transitions, errors with stable grep-friendly prefixes ([domain], [rpc], [ui-flow]) and correlation fields."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/impl/memory/tree/fetch_leaves.rs` around lines 60 - 65,
The call to retrieval::fetch_leaves and the serde_json::to_string call return
errors that are currently propagated with "?" without any RPC-prefixed logs or
contextual wrapping; add log::error calls with a stable prefix like
"[rpc][memory_tree]" and correlation fields from the incoming request (e.g., any
request_id/trace_id on req) and chunk info (chunk_ids.len() and take) when these
operations fail, and wrap the errors with additional context before returning
(e.g., use anyhow::Context or map_err to attach "rpc memory_tree: fetch_leaves
failed" and "rpc memory_tree: serialize hits failed") so callers and logs
include grep-friendly prefixes and useful correlation details for
retrieval::fetch_leaves and serde_json::to_string failures.

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.

2 participants