fix(memory): accept time_window_days global alias#2255
Conversation
📝 WalkthroughWalkthroughThis PR makes ChangesMemory Tree Global Query Field Compatibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/openhuman/tools/impl/memory/tree/mod.rs`:
- Around line 176-182: The test memory_tree_schema_exposes_global_window_days
has a rustfmt violation in the chained call that constructs properties from
schema; split the chained calls on separate lines to satisfy rustfmt (break
after schema) so each method (.get, .and_then, .unwrap) appears on its own
indented line, updating the let properties = ... expression in that test
(referencing MemoryTreeTool.parameters_schema and the local variable properties)
to the multiline style.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45bbaee2-c2f9-42c4-af59-67365c2acfb5
📒 Files selected for processing (2)
src/openhuman/memory/tree/retrieval/rpc.rssrc/openhuman/tools/impl/memory/tree/mod.rs
M3gA-Mind
left a comment
There was a problem hiding this comment.
Clean, targeted fix for a real bug. The serde alias approach is the right call — back-compat, minimal churn, and the consolidated schema descriptions are now accurate. CodeRabbit's formatting nit was addressed in the latest commit.
| File | Change |
|---|---|
memory/tree/retrieval/rpc.rs |
#[serde(alias = "time_window_days")] on QueryGlobalRequest.window_days + alias regression test |
tools/impl/memory/tree/mod.rs |
Schema now exposes both window_days and time_window_days; time_window_days description updated to clarify mode scope |
One minor thing to be aware of (not blocking):
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct QueryGlobalRequest { | ||
| #[serde(alias = "time_window_days")] | ||
| pub window_days: u32, |
There was a problem hiding this comment.
[minor] If a caller passes both window_days and time_window_days in the same request, serde will return a "duplicate field window_days" error — the alias maps both names to the same slot, so supplying both is treated as a duplicate.
The consolidated schema now exposes both fields without noting they're mutually exclusive for query_global. In practice no LLM/caller should ever send both, and the descriptions make the intent clear, but it's a subtle footgun to keep in mind if you see confusing deserialization errors down the line.
graycyrus
left a comment
There was a problem hiding this comment.
Clean bug fix — looks good.
Summary: Adds #[serde(alias = "time_window_days")] to QueryGlobalRequest.window_days so the consolidated memory_tree schema's advertised field actually works for query_global. Schema updated to expose both fields with clear per-mode descriptions. Two focused regression tests cover the alias deserialization and schema exposure.
| File | What changed |
|---|---|
rpc.rs |
serde alias on window_days + alias deserialization test |
tree/mod.rs |
schema exposes window_days for query_global + schema test |
Backwards-compatible, well-tested, and directly addresses #2252. All CI green. No issues from my side — moving to approval queue.
Summary
QueryGlobalRequestto deserializetime_window_daysas an alias forwindow_days.memory_treetool schema to expose the canonicalwindow_daysfield forquery_globalwhile preserving the existingtime_window_dayscompatibility path.Problem
The consolidated
memory_treetool advertisedtime_window_daysforquery_global, but the backend deserializedquery_globalarguments into:So calls shaped like this failed with
missing field 'window_days':{ "mode": "query_global", "time_window_days": 7 }Solution
Accept
time_window_daysas a serde alias forwindow_days:and make the consolidated schema explicit about
window_daysforquery_global.Submission Checklist
Closes #2252.Impact
window_dayscontinue to work, and consolidated tool callers usingtime_window_daysnow work too.Related
Closes #2252
AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/memory-tree-window-aliasc3350aeValidation Run
pnpm --filter openhuman-app format:check— N/A: Rust-only change.pnpm typecheck— N/A: Rust-only change.cargo test --manifest-path Cargo.toml memory_tree --lib; blocked locally, see below.cargo fmt --check; blocked locally, see below.app/src-taurichanges.Validation Blocked
command:cargo fmt --checkerror:error: no such command: fmtimpact:Local environment has Rust 1.75 without rustfmt installed; formatting should be verified by CI.command:cargo test --manifest-path Cargo.toml memory_tree --liberror:failed to parse lock file ... Cargo.lock; lock file version 4 requires -Znext-lockfile-bumpimpact:Local cargo is 1.75 and cannot read the repository's lockfile format; CI/newer cargo should run the focused tests.Behavior Changes
memory_treeglobal queries accept bothwindow_daysand the consolidated-schema aliastime_window_days.window_days.Parity Contract
window_dayscallers continue to deserialize identically.Duplicate / Superseded PR Handling
Summary by CodeRabbit