mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(memory): translate time_window_days for memory_tree query_global
## 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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2273?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: justin <justin80605@gmail.com> Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
This commit is contained in:
co-authored by
Steven Enamakel
parent
aaba2cadd1
commit
227f534e2c
@@ -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",
|
||||
}),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user