Skip to content

docs(memory-tree): update stale comment on translate_query_global_args #2713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zahica1234 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

docs(memory-tree): update stale comment on translate_query_global_args #2713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,7 @@ test-map.md

# AI assistant progress tracking
.kimi/
.specify/
.claude/skills/
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 May 27, 2026

Choose a reason for hiding this comment

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

🛑 Blocker — merge conflict with main. GitHub reports this PR as mergeable: CONFLICTING. Both this branch and main appended ignore entries just below .kimi/, and they collide here. The fix is a trivial union of the four entries:

# AI assistant progress tracking
.kimi/
.specify/
.claude/skills/
.codex-tmp
*.enc

The PR cannot be merged until this conflict is resolved on the branch (rebase onto / merge current main, keep all entries).

.codex-tmp
*.enc
17 changes: 7 additions & 10 deletions src/openhuman/memory/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,13 @@ impl Tool for MemoryTreeTool {
///
/// 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.
/// `query_source` backend uses that exact name). `QueryGlobalRequest` in
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 May 27, 2026

Choose a reason for hiding this comment

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

💡 Is this translator still needed, or is the cleaner fix to delete it? The original comment justified the rename because the backend only accepted window_days (failing with missing field 'window_days'). That's no longer true: QueryGlobalRequest now has time_window_days as its primary field with #[serde(alias = "window_days")], so the consolidated tool's time_window_days payload already deserializes directly. The new comment even concedes the rename just routes "through the alias path, which is equivalent" — i.e. effectively a no-op.

Rather than spend a paragraph documenting why a redundant rename is retained, consider removing the indirection so this arm matches its siblings (query_source, query_topic, … dispatch args unchanged):

"query_global" => MemoryTreeQueryGlobalTool.execute(args).await,

…and drop translate_query_global_args plus its rename test cases. If there is a remaining caller path that still needs the rename, a one-line note on that would be more useful than the current justification.

/// `memory_tree/retrieval/rpc.rs` uses `time_window_days` as its primary
/// field name and accepts `window_days` via `#[serde(alias = "window_days")]`.
/// The translation here keeps the LLM-facing consolidated schema stable
/// (callers always send `time_window_days`) while routing through the alias
/// path, which is equivalent. An explicit `window_days` in the payload
/// always wins — the translator is a no-op for callers that already use it.
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") {
Expand Down
Loading