docs(memory-tree): update stale comment on translate_query_global_args#2713
docs(memory-tree): update stale comment on translate_query_global_args#2713zahica1234 wants to merge 1 commit into
Conversation
The doc comment described QueryGlobalRequest as having `window_days: u32` as its primary field, which was the pre-fix state. The field was renamed to `time_window_days` with `#[serde(alias = "window_days")]` added so both names deserialize correctly. Update the comment to match the current struct shape and clarify that the translator routes through the alias path. Also add .specify/ and .claude/skills/ to .gitignore to exclude spec-kit project scaffolding from the tracked tree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR makes two independent maintenance updates: adding local AI assistant directories ( ChangesConfiguration and Documentation Updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
coderabbitai
Bot
added
the
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| # AI assistant progress tracking | ||
| .kimi/ | ||
| .specify/ | ||
| .claude/skills/ |
There was a problem hiding this comment.
🛑 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
*.encThe PR cannot be merged until this conflict is resolved on the branch (rebase onto / merge current main, keep all entries).
| /// wins so callers can opt into the underlying contract if they ever | ||
| /// want to. | ||
| /// `query_source` backend uses that exact name). `QueryGlobalRequest` in | ||
| /// `memory/tree/retrieval/rpc.rs` uses `time_window_days` as its primary |
There was a problem hiding this comment.
main. Since this branch was cut, main refactored the memory module: QueryGlobalRequest no longer lives at memory/tree/retrieval/rpc.rs — it's now memory_tree/retrieval/rpc.rs, and this whole file moved from tools/impl/memory/tree/mod.rs → memory/query/mod.rs. A docs PR whose entire purpose is to de-stale this comment will re-introduce a dead path the moment it merges. Please rebase onto current main and update the reference:
/// `query_source` backend uses that exact name). `QueryGlobalRequest` in
/// `memory_tree/retrieval/rpc.rs` uses `time_window_days` as its primary
| /// `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 |
There was a problem hiding this comment.
💡 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.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
PR #2713 — docs(memory-tree): update stale comment on translate_query_global_args
Cross-repo PR from
zahica1234:docs/fix-translate-query-global-comment→tinyhumansai/openhuman:main. State: OPEN. GitHub reportsmergeable: CONFLICTING.
Walkthrough
This is a small, well-intentioned docs-only change: it rewrites the doc comment on translate_query_global_args so it stops describing the old QueryGlobalRequest { window_days: u32 } shape and instead documents the current time_window_days primary field + #[serde(alias = "window_days")]. It also appends .specify/ and .claude/skills/ to .gitignore. The comment rewrite is technically accurate against the struct as it exists on the PR's base. However, the PR cannot merge as-is: there is an unresolved .gitignore conflict with main, and — more importantly — main has since landed a large memory-module refactor that moves the very file this PR edits and renames the path the new comment cites, so the "de-staled" comment lands stale again. This needs a rebase before it's worth merging.
Changes
| File | Summary |
|---|---|
.gitignore |
Adds .specify/ and .claude/skills/ ignore entries. Currently in merge conflict with main. |
src/openhuman/tools/impl/memory/tree/mod.rs |
Rewrites the doc comment on translate_query_global_args to match the current time_window_days (primary) + window_days (serde alias) shape. No logic change. |
Actionable comments (3)
🛑 Blockers
1. .gitignore:109-115 — unresolved merge conflict with main
GitHub reports this PR as mergeable: CONFLICTING, and merging main locally leaves conflict markers in .gitignore. Both branches appended ignore entries just below .kimi/:
<<<<<<< HEAD
.specify/
.claude/skills/
=======
.codex-tmp
*.enc
>>>>>>> main
The PR cannot be merged until this is resolved. The resolution is trivial (union — keep all four entries), but it must be done on the branch.
Suggested change (resolved block):
# AI assistant progress tracking
.kimi/
.specify/
.claude/skills/
.codex-tmp
*.enc⚠️ Major
2. src/openhuman/tools/impl/memory/tree/mod.rs:172 — the doc comment is already stale against current main
The whole point of this PR is to de-stale this comment, but main has landed a large memory refactor (PR #2556 and follow-ups) since the branch was cut. After merging current main:
- The edited file itself moves:
src/openhuman/tools/impl/memory/tree/mod.rs→src/openhuman/memory/query/mod.rs. - The path the new comment cites —
memory/tree/retrieval/rpc.rs— no longer exists;QueryGlobalRequestnow lives atsrc/openhuman/memory_tree/retrieval/rpc.rs.
So as written, the "fixed" comment points at a dead path the moment it merges. Please rebase onto current main and update the reference.
Suggested change:
// before
/// `query_source` backend uses that exact name). `QueryGlobalRequest` in
/// `memory/tree/retrieval/rpc.rs` uses `time_window_days` as its primary
// after
/// `query_source` backend uses that exact name). `QueryGlobalRequest` in
/// `memory_tree/retrieval/rpc.rs` uses `time_window_days` as its primary
💡 Refactor / suggestion
3. src/openhuman/tools/impl/memory/tree/mod.rs:171-184 — the translator is now redundant; consider deleting it instead of documenting why it's kept
The original comment explained the translation existed because the backend only accepted window_days and rejected time_window_days with missing field 'window_days'. That is no longer true: QueryGlobalRequest now has time_window_days as its primary field (with window_days as a serde alias), so the consolidated tool's time_window_days payload deserializes directly — no rename needed. The new comment even concedes the rename just routes "through the alias path, which is equivalent."
If the rename is equivalent to a no-op, the honest fix is to remove the indirection rather than write a paragraph justifying it. That also lets the query_global arm match every sibling arm (query_source, query_topic, …) which dispatch args unchanged.
Suggested change:
// in execute():
"query_global" => MemoryTreeQueryGlobalTool.execute(args).await,…and delete translate_query_global_args plus its memory_tree_dispatcher_tests rename cases. (Out of scope for a pure docs PR — flagging as a question for the author: is there a remaining caller path that still needs the rename, or is this safe to drop?)
Nitpicks (1)
src/openhuman/memory/query/mod.rs:310(post-merge location; pre-existing, not in this PR's diff) — while de-staling the function's doc comment, the sibling test comment intranslate_query_global_args_renames_time_window_days_to_window_daysis equally stale: it still claims "QueryGlobalRequestdeserializes fromwindow_days", which is the old shape. If you touch this area in the rebase, fix it too: the struct now deserializes fromtime_window_days(primary) and acceptswindow_daysvia the alias.
Questions for the author (1)
src/openhuman/tools/impl/memory/tree/mod.rs:178— Given the backend now acceptstime_window_daysnatively, istranslate_query_global_argsstill serving any purpose, or can it be deleted outright (see comment #3)?
Verified / looks good
- The rewritten comment is technically correct against the current struct:
QueryGlobalRequest { #[serde(alias = "window_days")] pub time_window_days: u32 }—time_window_daysprimary,window_daysalias. ✔ - The function body is unchanged and still correct: explicit
window_daysis preserved,time_window_daysis renamed only whenwindow_daysis absent. ✔ .gitignoreadditions (.specify/,.claude/skills/) are reasonable spec-kit/scaffolding ignores; consistent with the existing.kimi/entry. ✔- No i18n, security, or logging concerns — Rust doc comment + ignore entries only. ✔
🛑 Requesting changes — the .gitignore merge conflict is a hard merge blocker, and the docs change re-introduces a stale path reference once rebased onto current main.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
See the detailed review above. Blocking: (1) unresolved .gitignore merge conflict with main (mergeable: CONFLICTING); (2) the doc comment's path reference (memory/tree/retrieval/rpc.rs) is stale post-refactor — rebase onto current main and point it at memory_tree/retrieval/rpc.rs.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Looks good. Updated doc comment now accurately reflects the current field structure of QueryGlobalRequest — time_window_days as primary field with window_days as serde alias. Clean, focused change with no issues.
3 participants
Summary
translate_query_global_argsintools/impl/memory/tree/mod.rsQueryGlobalRequestas{ window_days: u32 }(pre-fix shape); field is nowtime_window_dayswith#[serde(alias = "window_days")].specify/and.claude/skills/to.gitignoreto exclude spec-kit scaffoldingTest plan
cargo fmt --checkpassesmod.rsandrpc.rscover the field alias behaviour🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation