From 227f534e2c6d1b795e79dbc670ff0fecd97d1ecb Mon Sep 17 00:00:00 2001 From: Justin Date: Thu, 21 May 2026 07:49:42 +0800 Subject: [PATCH] fix(memory): translate time_window_days for memory_tree query_global MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 ```jsonc // 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 - [x] Tests added or updated (happy path + at least one failure / edge case) per [Testing Strategy](../gitbooks/developing/testing-strategy.md#failure-path-requirement) — 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. - [x] **Diff coverage ≥ 80%** — `cargo test --lib memory_tree_dispatcher_tests::` runs 9/9 locally; the new translator helper has a dedicated test per branch. - [x] Coverage matrix updated — `N/A: behaviour-only fix on an existing consolidated tool path.` - [x] All affected feature IDs from the matrix are listed in the PR description under `## Related` - [x] No new external network dependencies introduced - [x] Manual smoke checklist — `N/A: backend-only fix, not on a release-cut surface.` - [x] 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 #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. [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2273?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) Co-authored-by: justin Co-authored-by: Steven Enamakel --- src/openhuman/tools/impl/memory/tree/mod.rs | 93 ++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/src/openhuman/tools/impl/memory/tree/mod.rs b/src/openhuman/tools/impl/memory/tree/mod.rs index 623311a46..e503c66b7 100644 --- a/src/openhuman/tools/impl/memory/tree/mod.rs +++ b/src/openhuman/tools/impl/memory/tree/mod.rs @@ -145,7 +145,11 @@ impl Tool for MemoryTreeTool { "search_entities" => MemoryTreeSearchEntitiesTool.execute(args).await, "query_topic" => MemoryTreeQueryTopicTool.execute(args).await, "query_source" => MemoryTreeQuerySourceTool.execute(args).await, - "query_global" => MemoryTreeQueryGlobalTool.execute(args).await, + "query_global" => { + MemoryTreeQueryGlobalTool + .execute(translate_query_global_args(args)) + .await + } "drill_down" => MemoryTreeDrillDownTool.execute(args).await, "fetch_leaves" => MemoryTreeFetchLeavesTool.execute(args).await, "ingest_document" => MemoryTreeIngestDocumentTool.execute(args).await, @@ -159,6 +163,32 @@ impl Tool for MemoryTreeTool { } } +/// Rename the consolidated tool's `time_window_days` field to `window_days` +/// before dispatching to [`MemoryTreeQueryGlobalTool`]. +/// +/// The consolidated `parameters_schema()` exposes one shared +/// `time_window_days` field for both `query_source` and `query_global` (the +/// `query_source` backend uses that exact name). The `query_global` backend +/// — `QueryGlobalRequest { window_days: u32 }` in `memory/tree/retrieval/rpc.rs` +/// — was never aligned with that schema, so any call following the +/// consolidated contract failed with `missing field 'window_days'`. +/// +/// Translating in the dispatch keeps the LLM-facing schema stable and +/// leaves the standalone [`MemoryTreeQueryGlobalTool`] (which advertises +/// `window_days` natively) untouched. An explicit `window_days` always +/// wins so callers can opt into the underlying contract if they ever +/// want to. +fn translate_query_global_args(mut args: serde_json::Value) -> serde_json::Value { + if let Some(obj) = args.as_object_mut() { + if !obj.contains_key("window_days") { + if let Some(value) = obj.remove("time_window_days") { + obj.insert("window_days".to_string(), value); + } + } + } + args +} + #[cfg(test)] mod memory_tree_dispatcher_tests { use super::*; @@ -230,4 +260,65 @@ mod memory_tree_dispatcher_tests { let result = MemoryTreeTool.execute(json!({})).await; assert!(result.is_err()); } + + #[test] + fn translate_query_global_args_renames_time_window_days_to_window_days() { + // Per-issue #2252: the consolidated schema advertises `time_window_days` + // but `QueryGlobalRequest` deserializes from `window_days`. The + // translation closes that gap inside the dispatch. + let translated = translate_query_global_args(json!({ + "mode": "query_global", + "time_window_days": 7, + })); + assert_eq!( + translated, + json!({ + "mode": "query_global", + "window_days": 7, + }), + ); + } + + #[test] + fn translate_query_global_args_passes_through_window_days_when_already_set() { + // The standalone `MemoryTreeQueryGlobalTool` schema advertises + // `window_days` natively — callers using that path should reach the + // backend unchanged. + let translated = translate_query_global_args(json!({ + "mode": "query_global", + "window_days": 30, + })); + assert_eq!(translated["window_days"], 30); + assert!(translated.get("time_window_days").is_none()); + } + + #[test] + fn translate_query_global_args_prefers_explicit_window_days_over_time_window_days() { + // If a caller somehow supplies both, the underlying contract wins + // (`window_days` is what the deserializer reads). Without this, + // a future caller migrating to `window_days` while leaving a stale + // `time_window_days` in the payload would silently lose their + // explicit choice to the legacy alias. + let translated = translate_query_global_args(json!({ + "mode": "query_global", + "window_days": 30, + "time_window_days": 7, + })); + assert_eq!(translated["window_days"], 30); + } + + #[test] + fn translate_query_global_args_leaves_payload_untouched_when_neither_field_present() { + // The underlying tool surfaces its own missing-field error in this + // case; the translator should not invent a value. + let translated = translate_query_global_args(json!({ + "mode": "query_global", + })); + assert_eq!( + translated, + json!({ + "mode": "query_global", + }), + ); + } }