fix(memory): translate time_window_days for memory_tree query_global

[justinhsu1477]

Summary

Fix the schema/backend impedance mismatch reported in #2252. The consolidated memory_tree tool advertises time_window_days as the look-back field for both query_source and query_global, but the underlying QueryGlobalRequest deserializes from window_days. Any LLM call that followed the consolidated contract with mode = "query_global" failed with missing field 'window_days'.

Problem

// LLM sends, per the consolidated schema in
// src/openhuman/tools/impl/memory/tree/mod.rs:
{ "mode": "query_global", "time_window_days": 7 }

// Dispatch hands `args` straight through:
//   "query_global" => MemoryTreeQueryGlobalTool.execute(args).await,

// `MemoryTreeQueryGlobalTool` (src/openhuman/tools/impl/memory/tree/query_global.rs)
// deserializes into:
//   pub struct QueryGlobalRequest { pub window_days: u32 }
//
// -> serde error: missing field `window_days`

query_source natively uses time_window_days (its QuerySourceRequest matches the consolidated schema verbatim), so the bug is isolated to the query_global arm.

Solution

Translate time_window_days → window_days in the consolidated dispatch arm for query_global, mirroring the existing thin-adapter pattern used in src/openhuman/mcp_server/tools.rs (where tree.read_chunk maps MCP chunk_id → controller id).

• Schema stays as advertised — LLMs already calling the consolidated tool aren't asked to relearn a field name.
• Standalone MemoryTreeQueryGlobalTool (which advertises window_days natively) is unchanged.
• Explicit window_days always wins, so callers using the underlying contract directly aren't surprised when both fields are present.

Submission Checklist

• Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — four new tests cover: the rename (happy path), passthrough when window_days is already set, explicit window_days winning over a stale time_window_days, and the no-field case being untouched.
• Diff coverage ≥ 80% — cargo test --lib memory_tree_dispatcher_tests:: runs 9/9 locally; the new translator helper has a dedicated test per branch.
• Coverage matrix updated — N/A: behaviour-only fix on an existing consolidated tool path.
• All affected feature IDs from the matrix are listed in the PR description under ## Related
• No new external network dependencies introduced
• Manual smoke checklist — N/A: backend-only fix, not on a release-cut surface.
• Linked issue closed via Closes #NNN — Closes #2252 (commit message + this PR description).

Impact

• Runtime: agent-facing memory_tree consolidated tool only. The standalone memory_tree_query_global tool path is unchanged.
• Compatibility: backward compatible. Callers that were passing window_days (the only callers that worked before this fix) continue to work. Callers that were passing time_window_days (which previously errored) now succeed.
• Performance: negligible — one Map::contains_key + at most one remove + one insert per query_global call.
• Security: no policy change.

Related

• Closes Bug: consolidated memory_tree query_global schema advertises time_window_days but backend requires window_days #2252
• Touches: src/openhuman/tools/impl/memory/tree/mod.rs
• Backend reference: src/openhuman/memory/tree/retrieval/rpc.rs (QueryGlobalRequest, QuerySourceRequest)
• Pattern reference: src/openhuman/mcp_server/tools.rs — same thin-adapter translation idiom used for tree.read_chunk's chunk_id → id mapping.

Summary by CodeRabbit

• Bug Fixes

  • Fixed global memory query handling to properly process field parameters, improving compatibility and reliability of memory tree queries.

• Tests

  • Extended test coverage for memory query parameter handling, including edge cases and field precedence validation.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds a schema translation layer to the MemoryTreeTool that bridges the LLM-facing field name time_window_days to the backend-expected field name window_days before dispatching to the query_global path. A new helper function handles the translation, and comprehensive unit tests validate the translation logic across multiple scenarios.

Changes

Query Global Payload Translation

Layer / File(s)	Summary
Payload translation helper src/openhuman/tools/impl/memory/tree/mod.rs	translate_query_global_args function renames time_window_days to window_days only if window_days is missing, and preserves all other payload fields unchanged.
Dispatch integration and test coverage src/openhuman/tools/impl/memory/tree/mod.rs	The query_global dispatch branch calls translate_query_global_args before invoking MemoryTreeQueryGlobalTool.execute. Unit tests validate renaming, pass-through when window_days is already set, field precedence when both are present, and no-op behavior when neither field exists.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• #2252: The translator implementation directly addresses the schema mismatch by renaming time_window_days to window_days before the backend dispatch, as described in the original issue.

Poem

🐰 A schema name, once lost in the weeds,

Now translates from dreams to what the backend needs!

From time_window_days to window_days so clean,

The kindest translation I ever have seen! ✨

🚥 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 directly and specifically describes the main change: translating time_window_days to window_days for the memory_tree query_global functionality.
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.}

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (2)

src/openhuman/tools/impl/memory/tree/mod.rs (2)

135-159: ⚡ Quick win

Move new translation logic out of mod.rs into a sibling module.

This helper is operational code, so it should live in a sibling file (for example, dispatch_args.rs) with mod.rs staying mostly export/assembly focused.

As per coding guidelines: "src/openhuman/**/mod.rs: Keep domain mod.rs files light and export-focused. Put operational code in sibling files (ops.rs, store.rs, schedule.rs, types.rs, bus.rs)."

🤖 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/tools/impl/memory/tree/mod.rs` around lines 135 - 159, Extract
the helper function translate_query_global_args from mod.rs into a new sibling
module (e.g., dispatch_args.rs) and make it public (pub fn
translate_query_global_args) so callers within the crate can use it; then in
mod.rs replace the function body with a re-export (pub use
self::dispatch_args::translate_query_global_args) and add a mod dispatch_args;
declaration so mod.rs remains export/assembly-focused while operational logic
lives in the new file. Ensure imports/serde_json types resolve in the new file
and update any references to translate_query_global_args to the public path if
needed.

---

152-156: Translator behavior is safe from deserialization errors; minor test coverage gap remains.

The QueryGlobalRequest struct does not use #[serde(deny_unknown_fields)] — it uses default serde behavior which silently ignores unknown fields. Therefore, dual-field payloads containing both window_days and time_window_days will deserialize without error.

However, a behavioral inconsistency exists: the translator removes time_window_days only when window_days is absent (lines 152–156), but preserves it when both fields are present. The test at lines 253–265 verifies that window_days takes precedence but does not assert that time_window_days is cleaned from the object in the dual-field case. For consistency with the single-field test (line 249: assert!(translated.get("time_window_days").is_none())), consider removing time_window_days unconditionally after extracting its value, or add an assertion to the dual-field test to document the preservation 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/tools/impl/memory/tree/mod.rs` around lines 152 - 156, The
translator currently only removes "time_window_days" when "window_days" is
absent, leaving dual-field payloads with both keys; update the translation logic
in the function handling QueryGlobalRequest to always remove the
"time_window_days" key after reading it: call obj.remove("time_window_days")
unconditionally and, if there was a value and "window_days" is not already
present, insert that value as "window_days" (i.e., extract then conditionally
insert), so the original legacy key is always cleaned up; alternatively, if you
prefer to keep current behavior, add an assertion in the dual-field test to
document preservation, but prefer the unconditional removal approach for
consistency.

🤖 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.

Nitpick comments:
In `@src/openhuman/tools/impl/memory/tree/mod.rs`:
- Around line 135-159: Extract the helper function translate_query_global_args
from mod.rs into a new sibling module (e.g., dispatch_args.rs) and make it
public (pub fn translate_query_global_args) so callers within the crate can use
it; then in mod.rs replace the function body with a re-export (pub use
self::dispatch_args::translate_query_global_args) and add a mod dispatch_args;
declaration so mod.rs remains export/assembly-focused while operational logic
lives in the new file. Ensure imports/serde_json types resolve in the new file
and update any references to translate_query_global_args to the public path if
needed.
- Around line 152-156: The translator currently only removes "time_window_days"
when "window_days" is absent, leaving dual-field payloads with both keys; update
the translation logic in the function handling QueryGlobalRequest to always
remove the "time_window_days" key after reading it: call
obj.remove("time_window_days") unconditionally and, if there was a value and
"window_days" is not already present, insert that value as "window_days" (i.e.,
extract then conditionally insert), so the original legacy key is always cleaned
up; alternatively, if you prefer to keep current behavior, add an assertion in
the dual-field test to document preservation, but prefer the unconditional
removal approach for consistency.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cefaf2fb-ae3f-4c2c-8012-6e4aba68f3f2

📥 Commits

Reviewing files that changed from the base of the PR and between 34bcf34 and 1208088.

📒 Files selected for processing (1)

• src/openhuman/tools/impl/memory/tree/mod.rs

[senamakel]

nice catch on the window_days vs time_window_days mismatch @justinhsu1477, that kind of schema/backend drift is a pain to track down 🙌 always a treat seeing you back in the repo, thanks for keeping memory_tree honest!