Conversation
…pe, and pagination
|
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:
📝 WalkthroughWalkthroughAdds a completed PROGRESS.md entry for a WebDAV SEARCH Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant MCP as MCP Tool\n(search_files)
participant Config as Config\nProvider
participant NCClient as NextcloudClient
participant WebDAV as Nextcloud\nWebDAV
User->>MCP: call search_files(query, path, mimetype, limit, offset)
MCP->>Config: get_config()
Config-->>MCP: return config/user info
MCP->>NCClient: build SEARCH XML (_build_search_xml)
NCClient->>WebDAV: DAV SEARCH request (XML body)
WebDAV-->>NCClient: SEARCH response (multistatus)
NCClient->>MCP: parse response -> structured results
MCP-->>User: return JSON { data: [...], pagination: {...} }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
+ Coverage 96.34% 96.53% +0.19%
==========================================
Files 13 13
Lines 629 664 +35
==========================================
+ Hits 606 641 +35
Misses 23 23
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:
|
|
(AI) Ready for review. All CI jobs green (lint, unit tests py3.12/3.13/3.14, integration NC 32 + NC 33). Codecov checks failing due to repo rename — needs re-setup on codecov.io. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PROGRESS.md (1)
46-58:⚠️ Potential issue | 🟡 MinorTest count discrepancy: sum of module tests (245) doesn't match total (250).
The individual module test counts sum to 245 (35+13+11+48+31+18+25+6+34+10+12+2), but the total shows 250. Please verify the counts are accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PROGRESS.md` around lines 46 - 58, The total tests count in PROGRESS.md is inconsistent: the module rows (Files, Users, Notifications, Talk, Talk Polls, Activity, Comments, Server, Permissions, Errors, Config, State) sum to 245 but the bolded Total shows 250; recalculate the sum of the right-column values and either update the individual module counts to reflect true values or correct the bolded Total to 245 so they match, ensuring the change is made in the PROGRESS.md table row for **Total** (or the specific module rows if one of those numbers was mistyped).
🤖 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/nextcloud_mcp/tools/files.py`:
- Around line 17-50: The _build_search_xml function interpolates unescaped user
inputs (query, mimetype, path, and user) directly into XML, causing XML
injection/malformed XML; fix by escaping these values (use
xml.sax.saxutils.escape or html.escape) before building where_parts,
mime_pattern, and scope so special XML chars (<, >, &, ", ') are replaced
safely, while preserving the intended wildcard logic (e.g., apply escape after
constructing the percent-wrapped query and after forming mime_pattern) and then
use the escaped variables when constructing the final XML string.
---
Outside diff comments:
In `@PROGRESS.md`:
- Around line 46-58: The total tests count in PROGRESS.md is inconsistent: the
module rows (Files, Users, Notifications, Talk, Talk Polls, Activity, Comments,
Server, Permissions, Errors, Config, State) sum to 245 but the bolded Total
shows 250; recalculate the sum of the right-column values and either update the
individual module counts to reflect true values or correct the bolded Total to
245 so they match, ensuring the change is made in the PROGRESS.md table row for
**Total** (or the specific module rows if one of those numbers was mistyped).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da1b4208-d28c-452a-8c93-224f45a6c20d
📒 Files selected for processing (4)
PROGRESS.mdsrc/nextcloud_mcp/tools/files.pytests/integration/test_files.pytests/integration/test_server.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/nextcloud_mcp/tools/files.py (1)
15-20:⚠️ Potential issue | 🟠 MajorEscape the SEARCH inputs before interpolating them into XML.
query,mimetype,path, anduserare still inserted raw into<d:literal>/<d:href>. Values containing&,<, or>will produce malformed XML or alter the generated SEARCH clause.Also applies to: 28-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nextcloud_mcp/tools/files.py` around lines 15 - 20, Escape all user-controlled values before inserting into XML literals/refs: apply an XML-escaping function (e.g. xml.sax.saxutils.escape or html.escape) to query, mimetype, path, and user prior to the f-string interpolation that builds where_parts and any <d:href> strings; for mimetype build mime_pattern first (with your % suffix logic) then escape the resulting mime_pattern, and use the escaped values in the f-strings that create the <d:literal> and <d:href> fragments so characters like &, <, and > cannot break the generated SEARCH XML.
🧹 Nitpick comments (1)
src/nextcloud_mcp/tools/files.py (1)
124-132: Consider moving SEARCH transport/parsing intoNextcloudClient.The tool layer now owns the raw XML body, the
SEARCHcall, the header, and a dependency onNextcloudClient._parse_propfind. Adav_search(...)client method would keep the DAV protocol boundary in one place and avoid new callers reaching into a private parser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nextcloud_mcp/tools/files.py` around lines 124 - 132, The SEARCH request and XML handling should be moved into the Nextcloud client to keep DAV protocol logic encapsulated: add a new NextcloudClient.dav_search(user, query, path, limit, offset, mimetype) method that internally calls _build_search_xml (or embeds that logic), performs client.dav_request("SEARCH", ..., headers={"Content-Type": "text/xml; charset=utf-8"}, body=...), and returns parsed results by calling the existing NextcloudClient._parse_propfind on the response; then update callers that currently build the body and call client.dav_request plus NextcloudClient._parse_propfind to call client.dav_search(...) instead and remove direct references to _build_search_xml and _parse_propfind from the tool layer.
🤖 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/nextcloud_mcp/tools/files.py`:
- Around line 119-124: The code allows negative offsets which produce invalid
XML like <d:firstresult>-1</d:firstresult>; in the function where body is built
(call to _build_search_xml in files.py) validate and sanitize the offset before
use (e.g., coerce to an int and set offset = max(0, offset)) so only
non-negative offsets are passed to _build_search_xml; update any callers or
docstring if needed to reflect that offset is clamped to zero minimum.
- Around line 133-137: The current code JSON-serializes `results` then appends a
text footer which breaks JSON; instead build a JSON-serializable structure (e.g.
an object with keys "results" and "pagination") and dump that: keep `results` as
the array, compute `next_offset = offset + len(results)` and include pagination
fields like `count`, `offset`, and `next_offset` (or null when no more pages),
then return json.dumps(...) of that object (use the existing `results`,
`offset`, and `next_offset` names to locate where to change the return value).
---
Duplicate comments:
In `@src/nextcloud_mcp/tools/files.py`:
- Around line 15-20: Escape all user-controlled values before inserting into XML
literals/refs: apply an XML-escaping function (e.g. xml.sax.saxutils.escape or
html.escape) to query, mimetype, path, and user prior to the f-string
interpolation that builds where_parts and any <d:href> strings; for mimetype
build mime_pattern first (with your % suffix logic) then escape the resulting
mime_pattern, and use the escaped values in the f-strings that create the
<d:literal> and <d:href> fragments so characters like &, <, and > cannot break
the generated SEARCH XML.
---
Nitpick comments:
In `@src/nextcloud_mcp/tools/files.py`:
- Around line 124-132: The SEARCH request and XML handling should be moved into
the Nextcloud client to keep DAV protocol logic encapsulated: add a new
NextcloudClient.dav_search(user, query, path, limit, offset, mimetype) method
that internally calls _build_search_xml (or embeds that logic), performs
client.dav_request("SEARCH", ..., headers={"Content-Type": "text/xml;
charset=utf-8"}, body=...), and returns parsed results by calling the existing
NextcloudClient._parse_propfind on the response; then update callers that
currently build the body and call client.dav_request plus
NextcloudClient._parse_propfind to call client.dav_search(...) instead and
remove direct references to _build_search_xml and _parse_propfind from the tool
layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5b66783-7ae2-48de-a78a-bf347592df41
📒 Files selected for processing (1)
src/nextcloud_mcp/tools/files.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/nextcloud_mcp/tools/comments.py (1)
113-118:⚠️ Potential issue | 🟡 MinorClamp negative comment offsets before building the REPORT body.
Now that
list_comments()exposes explicit pagination state, Line 118 still serializes negative offsets unchanged. That can send<oc:offset>-1</oc:offset>to Nextcloud and makes this pager behave differently fromsearch_files(), which already normalizesoffset.🩹 Minimal fix
limit = max(1, min(100, limit)) + offset = max(0, offset) client = get_client() body = FILTER_COMMENTS_BODY.format(limit=limit, offset=offset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nextcloud_mcp/tools/comments.py` around lines 113 - 118, In list_comments(), normalize the offset before building the REPORT body the same way limit is clamped: ensure offset is at least 0 (e.g., offset = max(0, offset)) so FILTER_COMMENTS_BODY.format(limit=limit, offset=offset) never inserts a negative <oc:offset> value; update the code path that uses FILTER_COMMENTS_BODY to reference the normalized offset variable and keep the existing limit clamping logic intact.src/nextcloud_mcp/tools/files.py (1)
28-29:⚠️ Potential issue | 🟡 MinorEscape
userin the SEARCH scope.Line 29 still interpolates the raw username into
<d:href>. A username containing&or<will break the XML even though the other inputs are already escaped.🛡️ Minimal fix
- safe_path = xml_escape(path.strip("/")) - scope = f"/files/{user}/{safe_path}" if safe_path else f"/files/{user}" + safe_user = xml_escape(user) + safe_path = xml_escape(path.strip("/")) + scope = f"/files/{safe_user}/{safe_path}" if safe_path else f"/files/{safe_user}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nextcloud_mcp/tools/files.py` around lines 28 - 29, The code builds the WebDAV scope string using the raw user which can inject XML special characters into <d:href>; fix by XML-escaping the username before interpolation (e.g., call xml_escape on user to produce a safe_user) and use that safe_user when constructing scope (update the scope assignment where safe_path and user are used); keep safe_path as-is and ensure xml_escape is applied to the username wherever <d:href> or scope is composed.
🧹 Nitpick comments (1)
tests/integration/test_files.py (1)
306-315: Make the offset test prove page 2 is actually different.Lines 310-315 only show that the second call returned something. An implementation that ignores
offsetand always returns the first page would still pass. Compare the returned paths (or IDs) and assert the pages do not overlap.🧪 Tighten the assertion
page1 = await nc_mcp.call("search_files", query="page-file", limit=2, offset=0) data1 = json.loads(page1)["data"] assert len(data1) == 2 page2 = await nc_mcp.call("search_files", query="page-file", limit=2, offset=2) data2 = json.loads(page2)["data"] assert len(data2) >= 1 + paths1 = {e["path"] for e in data1} + paths2 = {e["path"] for e in data2} + assert paths1.isdisjoint(paths2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_files.py` around lines 306 - 315, The test_search_with_offset test currently only checks that page2 has items but doesn't verify offset semantics; change it to extract and compare unique identifiers (e.g., file paths or IDs) from the JSON results returned by nc_mcp.call("search_files") for page1 and page2 (variables page1/page2 -> data1/data2), then assert that the sets/lists do not overlap and that page2 contains the expected remaining items (e.g., different paths than page1) to ensure offset is respected.
🤖 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/nextcloud_mcp/tools/activity.py`:
- Around line 113-119: The pagination cursor currently always uses
min(activity_id) (oldest_id) which only works for sort=="desc"; update the
cursor computation in src/nextcloud_mcp/tools/activity.py to be sort-aware: if
sort == "desc" set since to min(a["activity_id"] for a in activities) (as
before), otherwise set since to max(a["activity_id"] for a in activities) so the
cursor points to the last item returned for ascending pages; adjust the response
pagination "since" assignment (the variable oldest_id can be renamed or
replaced) and update the docstring for the since parameter to state that the
cursor returned is the minimum ID for desc and the maximum ID for asc.
---
Duplicate comments:
In `@src/nextcloud_mcp/tools/comments.py`:
- Around line 113-118: In list_comments(), normalize the offset before building
the REPORT body the same way limit is clamped: ensure offset is at least 0
(e.g., offset = max(0, offset)) so FILTER_COMMENTS_BODY.format(limit=limit,
offset=offset) never inserts a negative <oc:offset> value; update the code path
that uses FILTER_COMMENTS_BODY to reference the normalized offset variable and
keep the existing limit clamping logic intact.
In `@src/nextcloud_mcp/tools/files.py`:
- Around line 28-29: The code builds the WebDAV scope string using the raw user
which can inject XML special characters into <d:href>; fix by XML-escaping the
username before interpolation (e.g., call xml_escape on user to produce a
safe_user) and use that safe_user when constructing scope (update the scope
assignment where safe_path and user are used); keep safe_path as-is and ensure
xml_escape is applied to the username wherever <d:href> or scope is composed.
---
Nitpick comments:
In `@tests/integration/test_files.py`:
- Around line 306-315: The test_search_with_offset test currently only checks
that page2 has items but doesn't verify offset semantics; change it to extract
and compare unique identifiers (e.g., file paths or IDs) from the JSON results
returned by nc_mcp.call("search_files") for page1 and page2 (variables
page1/page2 -> data1/data2), then assert that the sets/lists do not overlap and
that page2 contains the expected remaining items (e.g., different paths than
page1) to ensure offset is respected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca1f22e7-6fc3-45a8-99a5-ee5848d8891a
📒 Files selected for processing (6)
src/nextcloud_mcp/tools/activity.pysrc/nextcloud_mcp/tools/comments.pysrc/nextcloud_mcp/tools/files.pytests/integration/test_activity.pytests/integration/test_comments.pytests/integration/test_files.py
src/nextcloud_mcp/tools/activity.py
Outdated
| oldest_id = min(a["activity_id"] for a in activities) if activities else None | ||
| response: dict[str, Any] = { | ||
| "data": activities, | ||
| "pagination": { | ||
| "count": len(activities), | ||
| "has_more": len(activities) == limit, | ||
| "since": oldest_id, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the Nextcloud Activity OCS API, when paginating with the sinceparameter andsort=asc, should the next-page cursor be the maximum activity_id from the current page rather than the minimum?
💡 Result:
Yes, for the Nextcloud Activity OCS API when paginating with the since parameter and sort=asc, the next-page cursor should be the maximum activity_id from the current page. The API fetches activities starting after the provided since ID (exclusive), ordered by timestamp and activity_id both ASC. It sets the X-Activity-Last-Given header to the activity_id of the last activity returned in the page (which is the maximum ID in that page since sorted ASC). The next page is constructed using since equal to this X-Activity-Last-Given value, as shown in the Link header example and controller code. This ensures the next page fetches activities strictly after the last one seen, avoiding duplicates even if new activities are inserted.
Citations:
- 1: https://github.com/nextcloud/activity/blob/master/docs/endpoint-v2.md
- 2: https://github.com/nextcloud/activity/blob/master/lib/Controller/APIv2Controller.php
- 3: https://github.com/nextcloud/activity/blob/master/lib/Data.php
🏁 Script executed:
cd src/nextcloud_mcp/tools && head -150 activity.py | tail -70Repository: cloud-py-api/nc-mcp-server
Length of output: 3303
🏁 Script executed:
rg -A 40 "def get_activity" src/nextcloud_mcp/tools/activity.pyRepository: cloud-py-api/nc-mcp-server
Length of output: 2160
Fix pagination cursor to be sort-aware.
Lines 113–119 always calculate the pagination cursor as the minimum activity_id, which is correct only for sort="desc". When sort="asc", the cursor must be the maximum activity_id from the current page (the last activity returned); otherwise, the next page request repeats or stalls.
The sort parameter is available in scope and should be checked when computing the cursor:
🔁 Sort-aware cursor calculation
- oldest_id = min(a["activity_id"] for a in activities) if activities else None
+ next_since = (
+ (
+ min(a["activity_id"] for a in activities)
+ if sort == "desc"
+ else max(a["activity_id"] for a in activities)
+ )
+ if activities
+ else None
+ )
response: dict[str, Any] = {
"data": activities,
"pagination": {
"count": len(activities),
"has_more": len(activities) == limit,
- "since": oldest_id,
+ "since": next_since,
},
}Also update the docstring for the since parameter to clarify the cursor behavior depends on sort order.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oldest_id = min(a["activity_id"] for a in activities) if activities else None | |
| response: dict[str, Any] = { | |
| "data": activities, | |
| "pagination": { | |
| "count": len(activities), | |
| "has_more": len(activities) == limit, | |
| "since": oldest_id, | |
| next_since = ( | |
| ( | |
| min(a["activity_id"] for a in activities) | |
| if sort == "desc" | |
| else max(a["activity_id"] for a in activities) | |
| ) | |
| if activities | |
| else None | |
| ) | |
| response: dict[str, Any] = { | |
| "data": activities, | |
| "pagination": { | |
| "count": len(activities), | |
| "has_more": len(activities) == limit, | |
| "since": next_since, | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nextcloud_mcp/tools/activity.py` around lines 113 - 119, The pagination
cursor currently always uses min(activity_id) (oldest_id) which only works for
sort=="desc"; update the cursor computation in
src/nextcloud_mcp/tools/activity.py to be sort-aware: if sort == "desc" set
since to min(a["activity_id"] for a in activities) (as before), otherwise set
since to max(a["activity_id"] for a in activities) so the cursor points to the
last item returned for ascending pages; adjust the response pagination "since"
assignment (the variable oldest_id can be renamed or replaced) and update the
docstring for the since parameter to state that the cursor returned is the
minimum ID for desc and the maximum ID for asc.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nextcloud_mcp/tools/files.py (1)
135-135: Consider exposing_parse_propfindas a public method or module function.Calling the protected method
NextcloudClient._parse_propfindfrom outside the class works but is unconventional. Since it's a pure static method with no side effects, you could either rename it toparse_propfind(without underscore) or extract it to a module-level utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nextcloud_mcp/tools/files.py` at line 135, The code calls the protected static method NextcloudClient._parse_propfind from outside the class; make this function public or module-level to follow convention and clarity: either rename NextcloudClient._parse_propfind to NextcloudClient.parse_propfind (remove leading underscore) and update all callers (e.g., the call in files.py that currently uses NextcloudClient._parse_propfind), or extract the logic into a top-level function parse_propfind in the same module and import/use that from files.py; ensure you update references and any tests to use the new public symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nextcloud_mcp/tools/files.py`:
- Line 135: The code calls the protected static method
NextcloudClient._parse_propfind from outside the class; make this function
public or module-level to follow convention and clarity: either rename
NextcloudClient._parse_propfind to NextcloudClient.parse_propfind (remove
leading underscore) and update all callers (e.g., the call in files.py that
currently uses NextcloudClient._parse_propfind), or extract the logic into a
top-level function parse_propfind in the same module and import/use that from
files.py; ensure you update references and any tests to use the new public
symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92ee8027-456d-4cfe-a851-4388bffddfbf
📒 Files selected for processing (3)
PROGRESS.mdsrc/nextcloud_mcp/tools/comments.pysrc/nextcloud_mcp/tools/files.py
✅ Files skipped from review due to trivial changes (1)
- PROGRESS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nextcloud_mcp/tools/comments.py
2 participants
Summary
search_filestool (READ) using WebDAV SEARCH APITool added
search_filesExamples
search_files(query="report")— find all files with "report" in the namesearch_files(mimetype="image")— find all imagessearch_files(query="notes", mimetype="text", path="Documents")— find text files matching "notes" in Documentssearch_files(query="budget", limit=5)— first 5 resultsTest plan
Summary by CodeRabbit
New Features
Behavior Changes
dataandpagination(count/offset/has_more/since) instead of textual footers.Tests
Documentation
Chores