fix: unify query_global params (#2252) and add MCP Accept headers (#2251)#2381
fix: unify query_global params (#2252) and add MCP Accept headers (#2251)#2381shguddn8591 wants to merge 13 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)
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 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}
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.
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.
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.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — fix: unify query_global params & MCP Accept headers
Nice fix for the window_days/time_window_days mismatch (#2252) — the serde alias is the right call. The MCP header work for #2251 mostly lands well, but there's one regression risk and an undisclosed dependency that need attention.
| Area | Files | Verdict |
|---|---|---|
| MCP client | client.rs |
Accept header fix is incomplete — see critical/major below |
| Memory RPC | rpc.rs |
Clean rename + alias, backward-compat preserved |
| Tests | ops_tests.rs |
Race fix looks correct (CodeRabbit-flagged, addressed) |
| Cargo.lock | Cargo.lock |
Undisclosed motosan-ai-oauth dependency |
| Cleanup | 6 files | Import narrowing, lint, visibility — unrelated but harmless |
| dependencies = [ | ||
| "base64 0.22.1", | ||
| "percent-encoding", | ||
| "rand 0.9.4", |
There was a problem hiding this comment.
[critical] Undisclosed dependency: motosan-ai-oauth 0.2.0
This crate (with reqwest, tokio, sha2, base64 deps) is added as a direct dependency of the openhuman package but isn't mentioned anywhere in the PR description, and no Cargo.toml change appears in the diff.
This is a network-capable OAuth library from an external registry. Even if it's benign, undisclosed dependencies are a supply-chain red flag. Please:
- Explain why this dependency was added and what it's used for
- Include the corresponding
Cargo.tomlchange in the PR - If this was pulled in by a rebase, note that in the PR description
There was a problem hiding this comment.
discover_authorization was sending Accept: application/json instead of MCP_HTTP_ACCEPT ("application/json, text/event-stream"). On strict servers this caused a 406 response, which the function treated as "no auth required" — silently breaking OAuth discovery.
The test discover_authorization_returns_none_when_not_401 was also masking the bug: the test server enforces has_streamable_http_accept, so it was returning 406, not 200 — the test passed for the wrong reason. Fixed in commit 32f8da1. All 17 mcp_client tests pass.
| .post(&self.endpoint) | ||
| .header(CONTENT_TYPE, "application/json") | ||
| .header(ACCEPT, MCP_HTTP_ACCEPT) | ||
| .header(ACCEPT, "application/json") |
There was a problem hiding this comment.
[major] send_initialize Accept header downgraded — potential #2251 regression
This changed from MCP_HTTP_ACCEPT ("application/json, text/event-stream") to just "application/json". But #2251 specifically shows that GitBooks returns HTTP 406 on the initialize request when the client doesn't accept both content types.
The fix correctly adds MCP_HTTP_ACCEPT to apply_standard_headers, but send_initialize builds its own request and doesn't go through that path. If GitBooks init hits this method, the 406 is back.
Suggestion: keep MCP_HTTP_ACCEPT here, or verify that GitBooks initialize goes through apply_standard_headers instead of this method.
There was a problem hiding this comment.
Fixed in 32f8da1. discover_authorization now uses MCP_HTTP_ACCEPT.
To clarify the method names: there is no send_initialize method. The initialize method (the standard session init path) builds its own request and already used MCP_HTTP_ACCEPT since 3f3b58d — it does not go through apply_standard_headers. discover_authorization was the only gap; that's now fixed. All 17 mcp_client tests pass.
| /// Providers the welcome agent cares about. Keep this list aligned | ||
| /// with the webview accounts system in `app/src-tauri/src/webview_accounts/`. | ||
| pub(crate) const PROVIDERS: &[Provider] = &[ | ||
| pub(self) const PROVIDERS: &[Provider] = &[ |
There was a problem hiding this comment.
[minor] pub(self) is equivalent to no visibility modifier (plain const is already private to the module). You can just drop the pub(self) entirely:
const PROVIDERS: &[Provider] = &[There was a problem hiding this comment.
Fixed in 898e14e. pub(self) is indeed redundant — dropped entirely as suggested.
discover_authorization was sending Accept: application/json, causing strict MCP servers (e.g. GitBooks) to return 406 instead of 401. The function treated any non-401 as "no auth required", so OAuth discovery silently failed on those servers. The test discover_authorization_returns_none_when_not_401 was passing for the wrong reason (406 masking as non-401). With this fix the test server returns 200 and the test validates the intended path. Fixes the regression introduced in the accept-header change from this same branch.
pub(self) is equivalent to no visibility modifier. Plain const is already private to the module, so the qualifier is misleading noise.
de-3.ts had subconscious.providerUnavailableTitle and providerSettings defined twice. de-5.ts had the entire settings.mcpServer and settings.developerMenu.mcpServer block duplicated at end of file. TypeScript TS1117 error blocked the E2E build.
Merge conflict resolution accidentally combined upstream's entry-based assert body with the branch's bare `fetched_at` variable reference. The variable is not declared in the upstream version; fix to use entry.fetched_at.elapsed() as upstream intended.
…enai_oauth commit
2 participants
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