Add Unified Search tools with cross-app search support#43
Merged
oleksandr-nc merged 2 commits intomainfrom Apr 12, 2026
Merged
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
+ Coverage 96.16% 96.22% +0.06%
==========================================
Files 26 27 +1
Lines 2658 2701 +43
==========================================
+ Hits 2556 2599 +43
Misses 102 102
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
(AI) Ready for review. All CI green. Two read-only Unified Search tools that search across all 18 Nextcloud app providers with cursor pagination and provider-specific filters. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/integration/test_search.py (2)
96-99:test_search_with_limitdoes not strongly validate limit enforcement.Line 99 passes even when results are 0/1, so truncation behavior is not proven. Seed >2 matching fixtures and assert exactly 2 results.
Suggested test tightening
async def test_search_with_limit(self, nc_mcp: McpTestHelper) -> None: + await nc_mcp.create_test_dir() + for i in range(3): + await nc_mcp.upload_test_file(f"mcp-test-suite/pagtest-limit-{i}.txt", "pagtest-limit") - result = await nc_mcp.call("unified_search", provider="files", term="pagtest", limit=2) + result = await nc_mcp.call("unified_search", provider="files", term="pagtest-limit", limit=2) data = json.loads(result) - assert len(data["entries"]) <= 2 + assert len(data["entries"]) == 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_search.py` around lines 96 - 99, test_search_with_limit doesn't prove truncation because it allows 0/1 results; update the test_search_with_limit to ensure there are more than two matching fixtures before calling nc_mcp.call("unified_search", provider="files", term="pagtest", limit=2) (e.g., create or seed at least 3 entries that match "pagtest" using the existing test helper), then assert that json.loads(result)["entries"] has length exactly 2 to validate the limit is enforced.
122-130: Add negative-path filter tests.Current coverage only validates valid JSON filters. Add cases for invalid JSON and reserved keys (
limit,term,cursor) so request-shape constraints are regression-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_search.py` around lines 122 - 130, Add negative-path unit tests for the unified_search call to ensure filter validation: extend or add tests alongside test_search_with_filter that call nc_mcp.call("unified_search", ...) with filters set to invalid JSON (e.g., malformed string) and with filters containing reserved keys "limit", "term", and "cursor"; assert that the response indicates a validation error (HTTP error code or error payload consistent with the service) rather than success, and ensure the tests reference the same test harness (test_search_with_filter, nc_mcp.call) so they run in the same suite and protect against regressions in request-shape handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nc_mcp_server/tools/search.py`:
- Around line 103-105: The current code directly does params.update(extra) after
parsing filters, allowing callers to override reserved pagination/query keys
(like "limit", "term", "cursor") and bypass the earlier clamping logic; change
the update to either whitelist allowed filter keys or strip reserved keys from
extra before merging (i.e., remove "limit", "term", "cursor" from extra or only
merge keys not in that reserved set) so that the existing clamp/pagination
enforcement on params remains in effect; update the logic around the filters
parsing in search.py where `extra = json.loads(filters)` and
`params.update(extra)` is used.
- Line 108: The code at the call to client.ocs_get is interpolating raw provider
into the path (SEARCH_API/{provider}/search); sanitize by URL-encoding the
provider before building the endpoint so characters like "/" cannot alter the
path. Use a standard encoder (e.g., urllib.parse.quote with an appropriate safe
parameter) to create provider_encoded and replace
f"{SEARCH_API}/{provider}/search" with
f"{SEARCH_API}/{provider_encoded}/search"; add the necessary import for the
encoder and ensure any tests or callers still pass encoded values where needed.
---
Nitpick comments:
In `@tests/integration/test_search.py`:
- Around line 96-99: test_search_with_limit doesn't prove truncation because it
allows 0/1 results; update the test_search_with_limit to ensure there are more
than two matching fixtures before calling nc_mcp.call("unified_search",
provider="files", term="pagtest", limit=2) (e.g., create or seed at least 3
entries that match "pagtest" using the existing test helper), then assert that
json.loads(result)["entries"] has length exactly 2 to validate the limit is
enforced.
- Around line 122-130: Add negative-path unit tests for the unified_search call
to ensure filter validation: extend or add tests alongside
test_search_with_filter that call nc_mcp.call("unified_search", ...) with
filters set to invalid JSON (e.g., malformed string) and with filters containing
reserved keys "limit", "term", and "cursor"; assert that the response indicates
a validation error (HTTP error code or error payload consistent with the
service) rather than success, and ensure the tests reference the same test
harness (test_search_with_filter, nc_mcp.call) so they run in the same suite and
protect against regressions in request-shape handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b88448b5-5326-4c1f-bbe6-bc5f7ddd5d27
📒 Files selected for processing (5)
PROGRESS.mdsrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/search.pytests/integration/test_search.pytests/integration/test_server.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/nc_mcp_server/tools/search.py (2)
103-107:⚠️ Potential issue | 🟠 MajorValidate
filtersis a JSON object before merging.At Line 104, valid JSON that is not an object (e.g.,
[],1,"x") can make Line 105/Line 107 fail with runtime errors instead of a clean user-facing validation error.Suggested fix
@@ if filters: - extra = json.loads(filters) + try: + extra = json.loads(filters) + except json.JSONDecodeError as exc: + raise ValueError("filters must be valid JSON") from exc + if not isinstance(extra, dict): + raise ValueError("filters must be a JSON object") for key in ("term", "limit", "cursor"): extra.pop(key, None) params.update(extra)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/search.py` around lines 103 - 107, The code parses filters into `extra` and assumes it's a JSON object; if `filters` is valid JSON but not an object (e.g., list, number, string) the subsequent `extra.pop(...)` and `params.update(extra)` will raise runtime errors — change the logic in the block that handles `filters` (the `filters` variable, `extra` and `params`) to validate that `json.loads(filters)` returns a dict before mutating it; if it's not a dict, return or raise a clear validation error (or convert/ignore non-object types per API contract) so only objects are merged into `params`.
110-110:⚠️ Potential issue | 🟠 MajorURL-encode
providerbefore composing the endpoint path.Line 110 currently interpolates raw user input into a path segment. Encode it first so
/and path tokens cannot alter endpoint shape.Suggested fix
@@ import json from typing import Any +from urllib.parse import quote @@ - data = await client.ocs_get(f"{SEARCH_API}/{provider}/search", params=params) + provider_path = quote(provider, safe="") + data = await client.ocs_get(f"{SEARCH_API}/{provider_path}/search", params=params)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/search.py` at line 110, The code interpolates raw provider into the path (data = await client.ocs_get(f"{SEARCH_API}/{provider}/search", ...)), so URL-encode provider first to prevent path injection; modify the call to build the URL with an encoded_provider produced via urllib.parse.quote(provider, safe='') (or equivalent), update the f-string to use encoded_provider, and add the necessary import of quote from urllib.parse so ocs_get receives a safe, encoded path segment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/nc_mcp_server/tools/search.py`:
- Around line 103-107: The code parses filters into `extra` and assumes it's a
JSON object; if `filters` is valid JSON but not an object (e.g., list, number,
string) the subsequent `extra.pop(...)` and `params.update(extra)` will raise
runtime errors — change the logic in the block that handles `filters` (the
`filters` variable, `extra` and `params`) to validate that `json.loads(filters)`
returns a dict before mutating it; if it's not a dict, return or raise a clear
validation error (or convert/ignore non-object types per API contract) so only
objects are merged into `params`.
- Line 110: The code interpolates raw provider into the path (data = await
client.ocs_get(f"{SEARCH_API}/{provider}/search", ...)), so URL-encode provider
first to prevent path injection; modify the call to build the URL with an
encoded_provider produced via urllib.parse.quote(provider, safe='') (or
equivalent), update the f-string to use encoded_provider, and add the necessary
import of quote from urllib.parse so ocs_get receives a safe, encoded path
segment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bfdafe7-d8a6-405d-bfd2-57b1f9e36bad
📒 Files selected for processing (1)
src/nc_mcp_server/tools/search.py
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
2 participants
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
list_search_providersandunified_searchTools
list_search_providersunified_searchTest plan
Summary by CodeRabbit
New Features
Tests
Documentation