fix: unify query_global params (#2252) and add MCP Accept headers (#2251)#2381
Open
shguddn8591 wants to merge 7 commits into
Open
fix: unify query_global params (#2252) and add MCP Accept headers (#2251)#2381shguddn8591 wants to merge 7 commits into
shguddn8591 wants to merge 7 commits into
Conversation
…ing Accept header to MCP client - Rename 'window_days' to 'time_window_days' in memory_tree retrieval RPC to align with agent prompts (fixes tinyhumansai#2252) - Add 'Accept: application/json' and 'Accept: text/event-stream' headers to McpHttpClient to resolve 406 errors with GitBooks MCP (fixes tinyhumansai#2251)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughStandardizes MCP client Accept headers (JSON vs SSE) and groups minor infra edits: removing a Clone derive, gating Unix trait imports, changing a module re-export, tightening a constant's visibility, silencing an unused_mut lint, renaming an unused parameter, reducing an import, and improving cache test determinism. ChangesMCP HTTP Accept header standardization
Minor derives, imports, re-exports, visibility, lint, and test tweaks
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/memory/tree/retrieval/rpc.rs (1)
85-96: ⚡ Quick winThe field rename from
window_daystotime_window_daysis correctly implemented; consider adding a compatibility alias if this API is public.No existing usage of the legacy
window_daysfield name was found in the codebase. However, if external clients will call this RPC interface, a#[serde(alias)]provides forward-compatible defense without impacting the current refactor:Suggested compatibility enhancement
#[derive(Debug, Serialize, Deserialize)] pub struct QueryGlobalRequest { + #[serde(alias = "window_days")] pub time_window_days: u32, }This is optional if the RPC is internal-only; required if external clients may depend on it.
🤖 Prompt for 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. In `@src/openhuman/memory/tree/retrieval/rpc.rs` around lines 85 - 96, The QueryGlobalRequest struct was renamed to use time_window_days; add a forward-compatible serde alias so external callers using the old window_days still work: update the QueryGlobalRequest definition (the struct used by query_global_rpc) to annotate the time_window_days field with #[serde(alias = "window_days")] (or equivalent) so deserialization accepts both names while leaving query_global_rpc and query_global unchanged.
🤖 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.
Nitpick comments:
In `@src/openhuman/memory/tree/retrieval/rpc.rs`:
- Around line 85-96: The QueryGlobalRequest struct was renamed to use
time_window_days; add a forward-compatible serde alias so external callers using
the old window_days still work: update the QueryGlobalRequest definition (the
struct used by query_global_rpc) to annotate the time_window_days field with
#[serde(alias = "window_days")] (or equivalent) so deserialization accepts both
names while leaving query_global_rpc and query_global unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ce827d0-e391-425d-8ce0-e716f4658629
📒 Files selected for processing (4)
src/openhuman/mcp_client/client.rssrc/openhuman/memory/tree/retrieval/rpc.rssrc/openhuman/memory/tree/retrieval/schemas.rssrc/openhuman/tools/impl/memory/tree/query_global.rs
- Introduced a new alias `window_days` for the existing `time_window_days` field in the `QueryGlobalRequest` struct to enhance compatibility with external data sources. - This change improves the flexibility of the API by allowing clients to use either field name when sending requests.
…ports - Adjusted the import of `PermissionsExt` to be conditional on both Unix and test configurations. - Removed unused exports from `model_requirements` in `local/mod.rs` to streamline the module. - Updated HTTP client headers in `client.rs` to use a constant for `ACCEPT` instead of a hardcoded string. - Cleaned up imports in `source_file.rs` to only include necessary types. - Minor adjustments in `pairing.rs` and `secrets.rs` for consistency in Unix imports. - Updated visibility of `PROVIDERS` in `ops.rs` to be self-contained.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/mcp_client/client.rs (1)
385-390: 💤 Low valueConsider eliminating redundant Accept header assignments.
Both
send_notification(line 389) andsend_jsonrpc_inner(line 440) set theAcceptheader before callingapply_standard_headers, which also sets it (line 489). While functionally correct (the last assignment wins), this pattern is redundant and could be streamlined by having onlyapply_standard_headersset theAcceptheader.♻️ Proposed refactor to remove duplication
Remove the
.header(ACCEPT, MCP_HTTP_ACCEPT)call fromsend_notification:let request = self .http .post(&self.endpoint) - .header(CONTENT_TYPE, "application/json") - .header(ACCEPT, MCP_HTTP_ACCEPT); + .header(CONTENT_TYPE, "application/json"); let request = self.apply_standard_headers(request, false, method, None, &[]);Remove the
.header(ACCEPT, MCP_HTTP_ACCEPT)call fromsend_jsonrpc_inner:let request = self .http .post(&self.endpoint) - .header(CONTENT_TYPE, "application/json") - .header(ACCEPT, MCP_HTTP_ACCEPT); + .header(CONTENT_TYPE, "application/json"); let request = if options.initialize {The
apply_standard_headersfunction already setsAcceptappropriately at line 489.Also applies to: 436-451, 489-489
🤖 Prompt for 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. In `@src/openhuman/mcp_client/client.rs` around lines 385 - 390, Remove the redundant Accept header assignments by deleting the explicit .header(ACCEPT, MCP_HTTP_ACCEPT) calls in send_notification and send_jsonrpc_inner and rely on apply_standard_headers to set Accept consistently; locate the two occurrences where request is built (in functions send_notification and send_jsonrpc_inner) and remove the MCP_HTTP_ACCEPT .header(...) invocation so that apply_standard_headers (the function that already sets Accept) is the single source of truth for that header.
🤖 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/memory/tree/tree_source/source_file.rs`:
- Line 35: The import was narrowed to only Tree which breaks tests that
reference TreeKind and TreeStatus; update the import in source_file.rs to bring
in the missing enum symbols (e.g., change the use crate::...::types::Tree; line
to import Tree, TreeKind and TreeStatus) so tests using TreeKind and TreeStatus
via super::* can resolve them; locate the use statement that currently
references types::Tree and expand it to types::{Tree, TreeKind, TreeStatus}.
---
Nitpick comments:
In `@src/openhuman/mcp_client/client.rs`:
- Around line 385-390: Remove the redundant Accept header assignments by
deleting the explicit .header(ACCEPT, MCP_HTTP_ACCEPT) calls in
send_notification and send_jsonrpc_inner and rely on apply_standard_headers to
set Accept consistently; locate the two occurrences where request is built (in
functions send_notification and send_jsonrpc_inner) and remove the
MCP_HTTP_ACCEPT .header(...) invocation so that apply_standard_headers (the
function that already sets Accept) is the single source of truth for that
header.
🪄 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: 371295f1-3912-4b65-94ff-a4142e35de93
📒 Files selected for processing (9)
src/core/auth.rssrc/openhuman/inference/local/mod.rssrc/openhuman/mcp_client/client.rssrc/openhuman/memory/tree/tree_source/source_file.rssrc/openhuman/security/pairing.rssrc/openhuman/security/secrets.rssrc/openhuman/voice/server.rssrc/openhuman/webview_accounts/ops.rssrc/openhuman/whatsapp_data/sqlite_retry.rs
💤 Files with no reviewable changes (1)
- src/openhuman/inference/local/mod.rs
✅ Files skipped from review due to trivial changes (4)
- src/openhuman/security/pairing.rs
- src/core/auth.rs
- src/openhuman/voice/server.rs
- src/openhuman/security/secrets.rs
- Updated the `runtime_snapshot_cache_hit_within_ttl` test to minimize lock hold time by reading cache fields under lock and asserting outside, reducing contention with other tests. - Simplified the `fetched_at` assignment to use a variable instead of calling `Instant::now()` directly. - Adjusted the `runtime_snapshot_cache_miss_after_ttl` test to directly assert on the `fetched_at` variable for clarity. - These changes enhance test reliability and maintainability, particularly in concurrent test scenarios.
- Added new dependencies: `encoding_rs` (v0.8.35), `motosan-ai-oauth` (v0.2.0), `system-configuration` (v0.7.0), and `system-configuration-sys` (v0.6.0) to the Cargo.lock file. - Updated existing dependencies to include `system-configuration` and `windows-registry`. - Minor formatting adjustments in `ops_tests.rs` for improved readability.
Contributor
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/app_state/ops_tests.rs`:
- Around line 203-217: The test currently writes to RUNTIME_SNAPSHOT_CACHE and
then opens a separate lock to read fetched_at and snapshot.autocomplete.phase,
allowing other tests to mutate the global between locks; fix this by holding the
RUNTIME_SNAPSHOT_CACHE.lock() once and, while the lock is held, read and clone
the needed values (e.g. entry.fetched_at and entry.snapshot.autocomplete.phase
or clone entry.snapshot) into local variables, then release the lock and perform
assertions on those locals so the write/read race is closed; reference
RUNTIME_SNAPSHOT_CACHE, CachedRuntimeSnapshot, fetched_at, dummy, and
snapshot.autocomplete.phase when making the change.
🪄 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: 5301489d-6efa-412a-9ce4-4c916a898e1d
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/openhuman/app_state/ops_tests.rs
…he tests - Updated the `runtime_snapshot_cache_hit_within_ttl` test to hold the lock during the entire write-then-read sequence, preventing concurrent tests from overwriting the cache. - This change improves test reliability and reduces contention issues in concurrent testing scenarios.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agent prompts.
Problem
expected window_days, causing runtime argument errors when the agent attempted to call the tool.
caused these servers to return 406 Not Acceptable, breaking the documentation search functionality.
Solution
implementation.
apply_standard_headers and specific method overrides. This ensures compatibility with strict HTTP servers.
Submission Checklist
suite)
Impact
Related
AI Authored PR Metadata
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behavior Changes
fails with 406.
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores