Add limit/offset pagination to unbounded list tools#35
Conversation
Tools updated: list_directory, list_conversations, get_events,
list_shares, list_notifications, list_trash, list_collectives,
get_collective_pages. All now accept limit/offset params and return
{"data": [...], "pagination": {count, offset, limit, has_more}}
matching the existing pattern used by search_files and get_activity.
Default limit is 50 (prevents context overflow on busy instances).
Client-side slicing since most Nextcloud APIs don't support
server-side pagination for these endpoints.
|
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:
📝 WalkthroughWalkthroughThis PR adds limit/offset pagination to multiple MCP read tools, standardizes their responses to {"data": [...], "pagination": {...}}, updates integration tests to the new shape, and adds a test-data seeding script plus a comprehensive pagination integration test suite. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Tool as MCP Tool
participant App as Nextcloud App
Client->>Tool: call list_<resource>(..., limit, offset)
Tool->>App: fetch full resource list
App-->>Tool: full list
Tool->>Tool: clamp limit/offset, slice data[offset:offset+limit]
Tool->>Tool: compute has_more, build pagination
Tool-->>Client: {"data": [...], "pagination": {count, offset, limit, has_more}}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 #35 +/- ##
==========================================
+ Coverage 95.88% 95.92% +0.03%
==========================================
Files 24 24
Lines 1848 1889 +41
==========================================
+ Hits 1772 1812 +40
- Misses 76 77 +1
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_calendar.py (1)
18-25:⚠️ Potential issue | 🟠 Major
get_eventspagination is not fully handled in cleanup and UID checks.Line 21 cleanup and event membership checks only inspect page 1. This can leave stale test events and produce flaky failures when calendars exceed default page size.
🔧 Suggested fix (iterate all `get_events` pages)
+async def _get_all_events(nc_mcp: McpTestHelper, **kwargs: object) -> list[dict[str, object]]: + limit = 500 + offset = 0 + all_events: list[dict[str, object]] = [] + while True: + parsed = json.loads(await nc_mcp.call("get_events", limit=limit, offset=offset, **kwargs)) + all_events.extend(parsed["data"]) + if not parsed["pagination"]["has_more"]: + break + offset += limit + return all_events + `@pytest.fixture`(autouse=True) async def _cleanup_test_events(nc_mcp: McpTestHelper) -> None: - result = await nc_mcp.call("get_events", calendar_id=CAL_ID) - for event in json.loads(result)["data"]: + for event in await _get_all_events(nc_mcp, calendar_id=CAL_ID): uid = event["uid"] if uid.startswith("mcp-test-") or "mcp-test" in event.get("summary", ""): with contextlib.suppress(ToolError): await nc_mcp.call("delete_event", calendar_id=CAL_ID, event_uid=uid)Also applies to: 257-259, 272-275, 329-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_calendar.py` around lines 18 - 25, The cleanup only processes the first page returned by nc_mcp.call("get_events") in _cleanup_test_events (and the other similar blocks) so stale test events on later pages are missed; modify these functions to iterate all pages from get_events by using the pagination cursor/next_page returned in the response (e.g., call nc_mcp.call("get_events", calendar_id=CAL_ID, page=...) or follow a "next" token until none), parse each page's json["data"], and for each event where uid.startswith("mcp-test-") or "mcp-test" in event.get("summary","") call nc_mcp.call("delete_event", calendar_id=CAL_ID, event_uid=uid) inside the existing contextlib.suppress(ToolError) block so deletions run for every page.
🧹 Nitpick comments (1)
src/nc_mcp_server/tools/collectives.py (1)
58-72: Extract the pagination wrapper before it drifts.The same clamp/slice/
{"data": ..., "pagination": ...}logic now exists twice in this file and across the other paginated tools in the PR. A tiny shared helper would keep the response contract consistent and make future changes one-place.♻️ Example helper shape
def _paginate(items: list[Any], limit: int, offset: int, *, max_limit: int) -> dict[str, Any]: limit = max(1, min(max_limit, limit)) offset = max(0, offset) page = items[offset : offset + limit] return { "data": page, "pagination": { "count": len(page), "offset": offset, "limit": limit, "has_more": offset + limit < len(items), }, }Also applies to: 90-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/collectives.py` around lines 58 - 72, Extract the repeated clamp/slice/response construction into a shared helper (e.g. def _paginate(items: list[Any], limit: int, offset: int, *, max_limit: int) -> dict[str, Any]) and replace the inline logic in the function that calls get_client(), client.ocs_get(...), and _format_collective(...) with a call to _paginate(all_collectives, limit, offset, max_limit=200); ensure _paginate enforces limit = max(1, min(max_limit, limit)), offset = max(0, offset), builds "data" = page and the "pagination" dict (count, offset, limit, has_more) and return json.dumps(_paginate(...), default=str); apply the same replacement to the other duplicated block that slices/paginates items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_collectives.py`:
- Around line 37-41: The cleanup and tests only inspect the first page returned
by the "list_collectives" RPC (see _cleanup_collectives and other test
assertions that call nc_mcp.call("list_collectives")), which makes tests flaky
when results are paginated; change the logic to iterate through paginated
responses until either the target collective is found or no more pages remain
(use the response's pagination cursor/next token or page parameter to fetch
subsequent pages), calling _destroy_collective for every matching item across
all pages and updating the assertions to search across all pages rather than
only page 1.
In `@tests/integration/test_files.py`:
- Around line 148-150: The assertion may miss the created path because
list_directory is paginated; change the test that calls
nc_mcp.call("list_directory", path="/") (in tests/integration/test_files.py) to
either request all entries by increasing/setting a large page_size or iterate
through pages (calling list_directory with page/page_size until no more data)
and then search across the combined "data" entries for TEST_BASE_DIR in the
"path" values before asserting; update the code that builds root/paths so it
aggregates pages from the list_directory responses (use the existing call
signature for "list_directory") and only then perform the any(TEST_BASE_DIR in p
for p in paths) assertion.
In `@tests/integration/test_shares.py`:
- Around line 40-43: The assertions inspect only the first page returned by
list_shares and can miss items; replace the paged call
(nc_mcp.call("list_shares") + json.loads(...)"data") with the helper that
fetches all pages (use _list_all_shares(...)) so membership checks like
any(s["share_type"] == 3 for s in data) run against the full result set; update
the occurrences where list_shares is used for deterministic membership checks
(including the other similar assertions) to call _list_all_shares and then
perform the same assertions on that aggregated list.
In `@tests/integration/test_system_tags.py`:
- Around line 22-25: The current helper only inspects the first page returned by
nc_mcp.call("list_directory", path=TEST_BASE_DIR) (default ~50 entries), so
change it to page through results until "tagged.txt" is found or no more pages
remain: repeatedly call nc_mcp.call("list_directory", path=TEST_BASE_DIR,
cursor=...) (or use the API's page_token/next_cursor and optional page_size) and
scan each returned listing for an entry where entry["path"] contains
"tagged.txt"; when found return int(entry["file_id"]); stop and return None or
raise if the paged results are exhausted. Ensure you reference the existing call
to nc_mcp.call("list_directory", ...) and the "tagged.txt" string when
implementing the paging loop.
In `@tests/integration/test_talk.py`:
- Around line 60-63: The test is relying on a single-page response from
nc_mcp.call("list_conversations") which can make token membership assertions
flaky; update tests (where tokens is derived from json.loads(result)["data"] and
assertions like assert room["token"] in tokens) to use a helper that pages
through all results (e.g. a new helper fetch_all_conversations or
list_conversations_all_pages) to collect every conversation token before
checking membership, and for leave tests add an explicit precondition asserting
the room token is present before calling leave and then assert it is absent
after; apply this change to the occurrences around
nc_mcp.call("list_conversations") (lines referenced: 60-63, 72-74, 429-431,
502-505).
In `@tests/integration/test_versions.py`:
- Around line 23-26: Both _create_versioned_file() and _get_file_id() only call
list_directory once against TEST_BASE_DIR and can miss entries once the default
page limit is exceeded; replace the single call with the project’s paged lookup
(iterate pages from list_directory or call the existing paged helper) when
searching for f"{VER_PREFIX}-{name}.txt" so the loop scans all pages until the
file is found (use the same paged approach in both _create_versioned_file and
_get_file_id and return the int(file_id) once a match is found).
---
Outside diff comments:
In `@tests/integration/test_calendar.py`:
- Around line 18-25: The cleanup only processes the first page returned by
nc_mcp.call("get_events") in _cleanup_test_events (and the other similar blocks)
so stale test events on later pages are missed; modify these functions to
iterate all pages from get_events by using the pagination cursor/next_page
returned in the response (e.g., call nc_mcp.call("get_events",
calendar_id=CAL_ID, page=...) or follow a "next" token until none), parse each
page's json["data"], and for each event where uid.startswith("mcp-test-") or
"mcp-test" in event.get("summary","") call nc_mcp.call("delete_event",
calendar_id=CAL_ID, event_uid=uid) inside the existing
contextlib.suppress(ToolError) block so deletions run for every page.
---
Nitpick comments:
In `@src/nc_mcp_server/tools/collectives.py`:
- Around line 58-72: Extract the repeated clamp/slice/response construction into
a shared helper (e.g. def _paginate(items: list[Any], limit: int, offset: int,
*, max_limit: int) -> dict[str, Any]) and replace the inline logic in the
function that calls get_client(), client.ocs_get(...), and
_format_collective(...) with a call to _paginate(all_collectives, limit, offset,
max_limit=200); ensure _paginate enforces limit = max(1, min(max_limit, limit)),
offset = max(0, offset), builds "data" = page and the "pagination" dict (count,
offset, limit, has_more) and return json.dumps(_paginate(...), default=str);
apply the same replacement to the other duplicated block that slices/paginates
items.
🪄 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: 7e0f45bb-8e5a-43b8-a5f9-8b9e13ea1fd5
📒 Files selected for processing (18)
src/nc_mcp_server/tools/calendar.pysrc/nc_mcp_server/tools/collectives.pysrc/nc_mcp_server/tools/files.pysrc/nc_mcp_server/tools/notifications.pysrc/nc_mcp_server/tools/shares.pysrc/nc_mcp_server/tools/talk.pysrc/nc_mcp_server/tools/trashbin.pytests/integration/test_calendar.pytests/integration/test_collectives.pytests/integration/test_files.pytests/integration/test_notifications.pytests/integration/test_permissions.pytests/integration/test_shares.pytests/integration/test_system_tags.pytests/integration/test_talk.pytests/integration/test_trashbin.pytests/integration/test_user_permissions.pytests/integration/test_versions.py
| Returns: | ||
| JSON list of entries, each with: path, is_directory, size, last_modified, content_type. | ||
| JSON with "data" (list of entries with path, is_directory, size, etc.) | ||
| and "pagination" (count, offset, limit, has_more). | ||
| """ | ||
| limit = max(1, min(500, limit)) | ||
| offset = max(0, offset) | ||
| client = get_client() | ||
| entries = await client.dav_propfind(path, depth=1) | ||
| # First entry is the directory itself — skip it | ||
| if entries and entries[0]["path"].rstrip("/") == path.strip("/"): | ||
| entries = entries[1:] | ||
| return json.dumps(entries, default=str) | ||
| page = entries[offset : offset + limit] | ||
| has_more = offset + limit < len(entries) | ||
|
|
||
| return json.dumps( | ||
| { | ||
| "data": page, | ||
| "pagination": {"count": len(page), "offset": offset, "limit": limit, "has_more": has_more}, | ||
| }, | ||
| default=str, | ||
| ) |
There was a problem hiding this comment.
Version or explicitly stage this wire-format change.
list_directory no longer returns a top-level array; it now returns an object with data/pagination. Any existing client doing json.loads(result) and iterating the list will break even if it never opts into pagination. The same contract change is repeated across the other list tools in this PR, so this needs a compatibility story before release.
| async def _cleanup_collectives(nc_mcp: McpTestHelper) -> None: | ||
| result = await nc_mcp.call("list_collectives") | ||
| for c in json.loads(result): | ||
| for c in json.loads(result)["data"]: | ||
| if str(c.get("name", "")).startswith(UNIQUE): | ||
| await _destroy_collective(nc_mcp, c["id"]) |
There was a problem hiding this comment.
First-page-only assertions make these integration tests flaky under pagination defaults.
Line 39 cleanup and the list assertions in Lines 57, 67, 227, and 240 only inspect page 1. With default limits, target collectives can fall outside that slice, causing intermittent failures (or false passes).
🔧 Suggested fix (paginate until found / exhausted)
+async def _list_all_collectives(nc_mcp: McpTestHelper) -> list[dict[str, Any]]:
+ limit = 200
+ offset = 0
+ all_rows: list[dict[str, Any]] = []
+ while True:
+ parsed = json.loads(await nc_mcp.call("list_collectives", limit=limit, offset=offset))
+ rows = parsed["data"]
+ all_rows.extend(rows)
+ if not parsed["pagination"]["has_more"]:
+ break
+ offset += limit
+ return all_rows
+
async def _cleanup_collectives(nc_mcp: McpTestHelper) -> None:
- result = await nc_mcp.call("list_collectives")
- for c in json.loads(result)["data"]:
+ for c in await _list_all_collectives(nc_mcp):
if str(c.get("name", "")).startswith(UNIQUE):
await _destroy_collective(nc_mcp, c["id"])Also applies to: 56-58, 67-68, 227-228, 240-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_collectives.py` around lines 37 - 41, The cleanup and
tests only inspect the first page returned by the "list_collectives" RPC (see
_cleanup_collectives and other test assertions that call
nc_mcp.call("list_collectives")), which makes tests flaky when results are
paginated; change the logic to iterate through paginated responses until either
the target collective is found or no more pages remain (use the response's
pagination cursor/next token or page parameter to fetch subsequent pages),
calling _destroy_collective for every matching item across all pages and
updating the assertions to search across all pages rather than only page 1.
| result = await nc_mcp.call("list_shares") | ||
| data = json.loads(result) | ||
| data = json.loads(result)["data"] | ||
| assert len(data) >= 1 | ||
| assert any(s["share_type"] == 3 for s in data) |
There was a problem hiding this comment.
Share existence checks are vulnerable to page-1 bias.
These assertions only inspect the first page of list_shares. On accounts with many shares, created/deleted targets may be outside that slice, causing flaky results.
🔧 Suggested fix (paginate or use constrained query)
+async def _list_all_shares(nc_mcp: McpTestHelper, *, path: str | None = None) -> list[dict[str, object]]:
+ limit = 200
+ offset = 0
+ all_rows: list[dict[str, object]] = []
+ while True:
+ kwargs = {"limit": limit, "offset": offset}
+ if path is not None:
+ kwargs["path"] = path
+ parsed = json.loads(await nc_mcp.call("list_shares", **kwargs))
+ all_rows.extend(parsed["data"])
+ if not parsed["pagination"]["has_more"]:
+ break
+ offset += limit
+ return all_rowsThen use _list_all_shares(...) where tests need deterministic membership checks.
Also applies to: 58-61, 334-336, 358-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_shares.py` around lines 40 - 43, The assertions
inspect only the first page returned by list_shares and can miss items; replace
the paged call (nc_mcp.call("list_shares") + json.loads(...)"data") with the
helper that fetches all pages (use _list_all_shares(...)) so membership checks
like any(s["share_type"] == 3 for s in data) run against the full result set;
update the occurrences where list_shares is used for deterministic membership
checks (including the other similar assertions) to call _list_all_shares and
then perform the same assertions on that aggregated list.
| listing = json.loads(await nc_mcp.call("list_directory", path=TEST_BASE_DIR))["data"] | ||
| for entry in listing: | ||
| if "tagged.txt" in entry["path"]: | ||
| return int(entry["file_id"]) |
There was a problem hiding this comment.
Page until tagged.txt is found instead of relying on the default page.
This helper now searches only the first 50 entries from list_directory. If TEST_BASE_DIR has more siblings than that, the lookup becomes nondeterministic and the test can fail even though the file exists.
♻️ One way to make the lookup deterministic
- listing = json.loads(await nc_mcp.call("list_directory", path=TEST_BASE_DIR))["data"]
- for entry in listing:
- if "tagged.txt" in entry["path"]:
- return int(entry["file_id"])
+ offset = 0
+ while True:
+ result = json.loads(
+ await nc_mcp.call("list_directory", path=TEST_BASE_DIR, limit=500, offset=offset)
+ )
+ for entry in result["data"]:
+ if "tagged.txt" in entry["path"]:
+ return int(entry["file_id"])
+ if not result["pagination"]["has_more"]:
+ break
+ offset += result["pagination"]["count"]
raise AssertionError("tagged.txt not found in listing")📝 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.
| listing = json.loads(await nc_mcp.call("list_directory", path=TEST_BASE_DIR))["data"] | |
| for entry in listing: | |
| if "tagged.txt" in entry["path"]: | |
| return int(entry["file_id"]) | |
| offset = 0 | |
| while True: | |
| result = json.loads( | |
| await nc_mcp.call("list_directory", path=TEST_BASE_DIR, limit=500, offset=offset) | |
| ) | |
| for entry in result["data"]: | |
| if "tagged.txt" in entry["path"]: | |
| return int(entry["file_id"]) | |
| if not result["pagination"]["has_more"]: | |
| break | |
| offset += result["pagination"]["count"] | |
| raise AssertionError("tagged.txt not found in listing") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_system_tags.py` around lines 22 - 25, The current
helper only inspects the first page returned by nc_mcp.call("list_directory",
path=TEST_BASE_DIR) (default ~50 entries), so change it to page through results
until "tagged.txt" is found or no more pages remain: repeatedly call
nc_mcp.call("list_directory", path=TEST_BASE_DIR, cursor=...) (or use the API's
page_token/next_cursor and optional page_size) and scan each returned listing
for an entry where entry["path"] contains "tagged.txt"; when found return
int(entry["file_id"]); stop and return None or raise if the paged results are
exhausted. Ensure you reference the existing call to
nc_mcp.call("list_directory", ...) and the "tagged.txt" string when implementing
the paging loop.
| result = await nc_mcp.call("list_conversations") | ||
| data = json.loads(result) | ||
| data = json.loads(result)["data"] | ||
| tokens = [c["token"] for c in data] | ||
| assert room["token"] in tokens |
There was a problem hiding this comment.
Conversation membership assertions should not rely on the first page only.
With default pagination, these checks can miss the target room and become flaky. In particular, the leave test can pass without proving the room was listed pre-leave.
🔧 Suggested fix (all-pages helper + precondition)
+async def _list_all_conversations(nc_mcp: McpTestHelper) -> list[dict[str, Any]]:
+ limit = 200
+ offset = 0
+ out: list[dict[str, Any]] = []
+ while True:
+ parsed = json.loads(await nc_mcp.call("list_conversations", limit=limit, offset=offset))
+ out.extend(parsed["data"])
+ if not parsed["pagination"]["has_more"]:
+ break
+ offset += limit
+ return outUse this helper where tests assert inclusion/exclusion by token, and in leave tests assert presence before leave, then absence after.
Also applies to: 72-74, 429-431, 502-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_talk.py` around lines 60 - 63, The test is relying on
a single-page response from nc_mcp.call("list_conversations") which can make
token membership assertions flaky; update tests (where tokens is derived from
json.loads(result)["data"] and assertions like assert room["token"] in tokens)
to use a helper that pages through all results (e.g. a new helper
fetch_all_conversations or list_conversations_all_pages) to collect every
conversation token before checking membership, and for leave tests add an
explicit precondition asserting the room token is present before calling leave
and then assert it is absent after; apply this change to the occurrences around
nc_mcp.call("list_conversations") (lines referenced: 60-63, 72-74, 429-431,
502-505).
| listing = json.loads(await nc_mcp.call("list_directory", path=TEST_BASE_DIR))["data"] | ||
| for entry in listing: | ||
| if f"{VER_PREFIX}-{name}.txt" in entry["path"]: | ||
| return int(entry["file_id"]) |
There was a problem hiding this comment.
Both file-ID helpers are now incomplete after the default limit change.
_create_versioned_file() and _get_file_id() each search only the first default page from list_directory. Once TEST_BASE_DIR has enough entries, these helpers can miss the target file and turn the versions tests flaky. Please reuse a paged lookup helper here instead of a single default call.
Also applies to: 32-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_versions.py` around lines 23 - 26, Both
_create_versioned_file() and _get_file_id() only call list_directory once
against TEST_BASE_DIR and can miss entries once the default page limit is
exceeded; replace the single call with the project’s paged lookup (iterate pages
from list_directory or call the existing paged helper) when searching for
f"{VER_PREFIX}-{name}.txt" so the loop scans all pages until the file is found
(use the same paged approach in both _create_versioned_file and _get_file_id and
return the int(file_id) once a match is found).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/test_talk.py (1)
501-504:⚠️ Potential issue | 🟠 MajorMembership check is still page-biased and can pass vacuously in leave flow.
This assertion still checks only one page (
limit=200) and does not assert pre-leave presence, so it can pass without proving the room was ever listed. This affects the same pattern in other token-membership checks.🔧 Suggested fix (all-pages helper + precondition)
+async def _list_all_conversations(nc_mcp: McpTestHelper) -> list[dict[str, Any]]: + limit = 200 + offset = 0 + out: list[dict[str, Any]] = [] + while True: + parsed = json.loads(await nc_mcp.call("list_conversations", limit=limit, offset=offset)) + out.extend(parsed["data"]) + if not parsed["pagination"]["has_more"]: + break + offset += limit + return out + class TestLeaveConversation: @@ async def test_leave_removes_from_list(self, nc_mcp: McpTestHelper) -> None: room = await _create_room(nc_mcp, "test-leave-list", room_type=3) try: + pre_tokens = [c["token"] for c in await _list_all_conversations(nc_mcp)] + assert room["token"] in pre_tokens await nc_mcp.call("leave_conversation", token=str(room["token"])) - list_result = await nc_mcp.call("list_conversations", limit=200) - conversations = json.loads(list_result)["data"] - tokens = [c["token"] for c in conversations] + tokens = [c["token"] for c in await _list_all_conversations(nc_mcp)] assert room["token"] not in tokens🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_talk.py` around lines 501 - 504, The test currently only fetches one page (limit=200) and never asserts the room was present before the leave, so it can pass vacuously; update the test to (1) assert pre-leave membership by calling "list_conversations" and verifying room["token"] is present, and (2) replace the single-page check after leave with a paginated fetch that iterates through all pages (implement a small helper like list_all_conversation_tokens or get_all_conversation_tokens that repeatedly calls nc_mcp.call("list_conversations", limit=..., cursor/offset/next) to collect every conversation token) and then assert room["token"] not in the full token set; reference the existing variables list_result, conversations, tokens and room["token"] when locating where to add the precondition and the pagination helper call.
🧹 Nitpick comments (1)
tests/integration/test_talk.py (1)
51-54: Rename test to match the new response contract.
test_returns_json_listnow validatesjson.loads(result)["data"]from a paginated envelope, not a raw JSON list. Renaming will keep intent clear.✏️ Suggested rename
- async def test_returns_json_list(self, nc_mcp: McpTestHelper) -> None: + async def test_returns_paginated_data_list(self, nc_mcp: McpTestHelper) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_talk.py` around lines 51 - 54, Rename the test function test_returns_json_list in tests/integration/test_talk.py to reflect that it now verifies a paginated envelope (e.g., test_returns_paginated_envelope_with_data_list or test_returns_data_list_from_paginated_envelope); update the function name where defined and any references to it, and keep the existing assertion that inspects json.loads(result)["data"] after the nc_mcp.call("list_conversations", limit=200).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_talk.py`:
- Around line 501-504: The test currently only fetches one page (limit=200) and
never asserts the room was present before the leave, so it can pass vacuously;
update the test to (1) assert pre-leave membership by calling
"list_conversations" and verifying room["token"] is present, and (2) replace the
single-page check after leave with a paginated fetch that iterates through all
pages (implement a small helper like list_all_conversation_tokens or
get_all_conversation_tokens that repeatedly calls
nc_mcp.call("list_conversations", limit=..., cursor/offset/next) to collect
every conversation token) and then assert room["token"] not in the full token
set; reference the existing variables list_result, conversations, tokens and
room["token"] when locating where to add the precondition and the pagination
helper call.
---
Nitpick comments:
In `@tests/integration/test_talk.py`:
- Around line 51-54: Rename the test function test_returns_json_list in
tests/integration/test_talk.py to reflect that it now verifies a paginated
envelope (e.g., test_returns_paginated_envelope_with_data_list or
test_returns_data_list_from_paginated_envelope); update the function name where
defined and any references to it, and keep the existing assertion that inspects
json.loads(result)["data"] after the nc_mcp.call("list_conversations",
limit=200).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 966868d0-eb9b-486f-b925-914ab1f4259f
📒 Files selected for processing (11)
tests/integration/test_calendar.pytests/integration/test_collectives.pytests/integration/test_files.pytests/integration/test_notifications.pytests/integration/test_permissions.pytests/integration/test_shares.pytests/integration/test_system_tags.pytests/integration/test_talk.pytests/integration/test_trashbin.pytests/integration/test_user_permissions.pytests/integration/test_versions.py
✅ Files skipped from review due to trivial changes (4)
- tests/integration/test_user_permissions.py
- tests/integration/test_versions.py
- tests/integration/test_shares.py
- tests/integration/test_calendar.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/integration/test_permissions.py
- tests/integration/test_trashbin.py
- tests/integration/test_files.py
- tests/integration/test_collectives.py
- tests/integration/test_notifications.py
…ion tests with seed data
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/seed_pagination_data.sh`:
- Around line 21-29: The backgrounded curl uploads inside the for loop (using
variables COUNT, NUM, AUTH, DAV_BASE, DIR) currently rely on a bare wait which
masks failed PUTs; change the loop to capture each background job's PID (e.g.,
save $! into an array), then after starting all jobs iterate over those PIDs and
wait on each explicitly, collecting exit codes and failing the script (exit
non‑zero) or printing an error if any curl returned non‑zero so failed uploads
are detected; ensure you propagate a non‑zero exit when any wait returns
non‑zero and optionally count/report failed uploads.
In `@src/nc_mcp_server/tools/notifications.py`:
- Around line 37-64: The server only returns ~25 notifications per request, so
update list_notifications to respect that server cap: define MAX_SERVER_PAGE =
25, clamp the requested limit to min(limit, MAX_SERVER_PAGE) and clamp offset to
max(0, min(offset, MAX_SERVER_PAGE - 1)); call get_client().ocs_get as before
and build all_notifs = [_format_notification(n) for n in data], then slice page
= all_notifs[offset: offset + limit]; compute has_more as True only if len(data)
== MAX_SERVER_PAGE and (offset + limit) < MAX_SERVER_PAGE (otherwise False);
update the pagination metadata accordingly and update the docstring/comments in
list_notifications to clearly state the 25-item server cap.
In `@tests/integration/test_pagination.py`:
- Around line 117-119: The tests assume a 25-item server-side window
(NC_NOTIF_LIMIT) but currently rely on ambient notifications; update the
pagination tests to either clear existing notifications before creating test
notifications or filter/assert only against the notifications your test created
(subjects starting with "pagtest-" and "format-check"). Concretely, add a
notification-cleanup step at the start of the pagination/formatting tests (or
call the existing notification deletion helper) and/or change assertions that
inspect result["data"][0] and the counts/uniqueness checks to locate items by
their subject prefix ("pagtest-" / "format-check") instead of relying on array
positions so the tests only validate the notifications your test code generated.
🪄 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: e4ddbd54-f8b9-400c-8ea4-5b47ffaed9a9
📒 Files selected for processing (7)
.github/workflows/tests-integration.ymlscripts/seed_pagination_data.shsrc/nc_mcp_server/tools/calendar.pysrc/nc_mcp_server/tools/comments.pysrc/nc_mcp_server/tools/files.pysrc/nc_mcp_server/tools/notifications.pytests/integration/test_pagination.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/nc_mcp_server/tools/calendar.py
- src/nc_mcp_server/tools/files.py
| for i in $(seq 1 $COUNT); do | ||
| NUM=$(printf '%03d' "$i") | ||
| curl -sf -u "$AUTH" -X PUT \ | ||
| -H "Content-Type: text/plain" \ | ||
| --data "Pagination test file $NUM" \ | ||
| "$DAV_BASE/$DIR/pagtest-$NUM.txt" -o /dev/null & | ||
| if (( i % 10 == 0 )); then wait; fi | ||
| done | ||
| wait |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash -lc '
set -euo pipefail
(exit 7) &
(sleep 0.1; exit 0) &
wait
printf "wait_status=%s\n" "$?"
'Repository: cloud-py-api/nc_mcp_server
Length of output: 199
🏁 Script executed:
cat -n scripts/seed_pagination_data.shRepository: cloud-py-api/nc_mcp_server
Length of output: 1219
Track background upload failures explicitly.
wait without explicit PIDs returns success once the background jobs finish, so failed PUTs on lines 23–26 will not fail this script. The step can print Created 55 files even when some uploads were rejected, which makes CI seeding silently flaky.
The pre-execution verification confirms this behavior: a backgrounded process exiting with code 7 does not cause a bare wait to fail.
Proposed fix
echo "Creating $COUNT files..."
+pids=()
for i in $(seq 1 $COUNT); do
NUM=$(printf '%03d' "$i")
curl -sf -u "$AUTH" -X PUT \
-H "Content-Type: text/plain" \
--data "Pagination test file $NUM" \
"$DAV_BASE/$DIR/pagtest-$NUM.txt" -o /dev/null &
- if (( i % 10 == 0 )); then wait; fi
+ pids+=("$!")
+ if (( i % 10 == 0 )); then
+ for pid in "${pids[@]}"; do
+ wait "$pid"
+ done
+ pids=()
+ fi
done
-wait
+for pid in "${pids[@]}"; do
+ wait "$pid"
+done📝 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.
| for i in $(seq 1 $COUNT); do | |
| NUM=$(printf '%03d' "$i") | |
| curl -sf -u "$AUTH" -X PUT \ | |
| -H "Content-Type: text/plain" \ | |
| --data "Pagination test file $NUM" \ | |
| "$DAV_BASE/$DIR/pagtest-$NUM.txt" -o /dev/null & | |
| if (( i % 10 == 0 )); then wait; fi | |
| done | |
| wait | |
| pids=() | |
| for i in $(seq 1 $COUNT); do | |
| NUM=$(printf '%03d' "$i") | |
| curl -sf -u "$AUTH" -X PUT \ | |
| -H "Content-Type: text/plain" \ | |
| --data "Pagination test file $NUM" \ | |
| "$DAV_BASE/$DIR/pagtest-$NUM.txt" -o /dev/null & | |
| pids+=("$!") | |
| if (( i % 10 == 0 )); then | |
| for pid in "${pids[@]}"; do | |
| wait "$pid" | |
| done | |
| pids=() | |
| fi | |
| done | |
| for pid in "${pids[@]}"; do | |
| wait "$pid" | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed_pagination_data.sh` around lines 21 - 29, The backgrounded curl
uploads inside the for loop (using variables COUNT, NUM, AUTH, DAV_BASE, DIR)
currently rely on a bare wait which masks failed PUTs; change the loop to
capture each background job's PID (e.g., save $! into an array), then after
starting all jobs iterate over those PIDs and wait on each explicitly,
collecting exit codes and failing the script (exit non‑zero) or printing an
error if any curl returned non‑zero so failed uploads are detected; ensure you
propagate a non‑zero exit when any wait returns non‑zero and optionally
count/report failed uploads.
| async def list_notifications(limit: int = 50, offset: int = 0) -> str: | ||
| """List notifications for the current Nextcloud user. | ||
|
|
||
| Returns notifications sorted by newest first. Each notification | ||
| includes: notification_id, app, datetime, subject, message, link, | ||
| and actions. | ||
| Returns notifications sorted by newest first. | ||
|
|
||
| Args: | ||
| limit: Maximum number of notifications to return (1-200, default 50). | ||
| offset: Number of notifications to skip for pagination (default 0). | ||
|
|
||
| Returns: | ||
| JSON list of notification objects. | ||
| JSON with "data" (list of notification objects with notification_id, app, | ||
| user, datetime, subject, message, and optionally link and actions) and | ||
| "pagination" (count, offset, limit, has_more). | ||
| """ | ||
| limit = max(1, min(200, limit)) | ||
| offset = max(0, offset) | ||
| client = get_client() | ||
| data = await client.ocs_get( | ||
| "apps/notifications/api/v2/notifications", | ||
| data = await client.ocs_get("apps/notifications/api/v2/notifications") | ||
| all_notifs = [_format_notification(n) for n in data] | ||
| page = all_notifs[offset : offset + limit] | ||
| has_more = offset + limit < len(all_notifs) | ||
|
|
||
| return json.dumps( | ||
| { | ||
| "data": page, | ||
| "pagination": {"count": len(page), "offset": offset, "limit": limit, "has_more": has_more}, | ||
| }, | ||
| default=str, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the Nextcloud OCS notifications endpoint /apps/notifications/api/v2/notificationssupport server-sidelimit/offset parameters, or is each response capped to 25 items?
💡 Result:
The Nextcloud OCS notifications endpoint /apps/notifications/api/v2/notifications does not support server-side limit or offset parameters for pagination. The official API documentation lists no such query parameters, and example responses show a simple array of notifications without pagination metadata or total count indicators. Each response appears capped to a fixed number of recent notifications (likely 25, based on common client observations and design for polling), requiring clients to dismiss viewed notifications and poll repeatedly with ETag support for unchanged responses (304 Not Modified). For full access, clients must iteratively fetch and manage notifications client-side, as there is no documented server-side pagination like limit/offset.
Citations:
- 1: https://github.com/nextcloud/notifications/blob/master/docs/ocs-endpoint-v2.md
- 2: https://github.com/nextcloud/notifications
- 3: https://www.firetower.net
- 4: https://nextcloud.github.io/news/api/api-v1-2/
- 5: https://github.com/nextcloud/activity/blob/master/docs/endpoint-v2.md
🏁 Script executed:
# First, check the test file mentioned to see the 25 notifications reference
cat -n tests/integration/test_pagination.py | sed -n '110,125p'Repository: cloud-py-api/nc_mcp_server
Length of output: 840
🏁 Script executed:
# Check the full implementation of list_notifications function
cat -n src/nc_mcp_server/tools/notifications.py | sed -n '37,70p'Repository: cloud-py-api/nc_mcp_server
Length of output: 1740
🏁 Script executed:
# Search for ocs_get implementation to understand if it returns paginated or single response
rg -A 10 "def ocs_get" src/Repository: cloud-py-api/nc_mcp_server
Length of output: 1034
The pagination implementation is fundamentally broken for offsets beyond 25 items.
The code fetches only a single server response (line 54), which Nextcloud caps at 25 notifications, then applies client-side slicing to that truncated result (lines 55-57). This means:
- Offsets ≥ 25 will always return empty results, despite the function signature accepting
offsetvalues up to any integer has_morewill always reportFalsefor offset ≥ 25, even if newer notifications exist- The
limitparameter claiming support up to 200 is misleading when the actual ceiling is 25
The Nextcloud OCS notifications endpoint /apps/notifications/api/v2/notifications does not support server-side limit/offset parameters—each API call returns a fixed ~25 most recent notifications. Either implement true pagination by repeated server polling with notification dismissal (as Nextcloud intends), or clamp the effective limits to 25 and document this constraint prominently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nc_mcp_server/tools/notifications.py` around lines 37 - 64, The server
only returns ~25 notifications per request, so update list_notifications to
respect that server cap: define MAX_SERVER_PAGE = 25, clamp the requested limit
to min(limit, MAX_SERVER_PAGE) and clamp offset to max(0, min(offset,
MAX_SERVER_PAGE - 1)); call get_client().ocs_get as before and build all_notifs
= [_format_notification(n) for n in data], then slice page = all_notifs[offset:
offset + limit]; compute has_more as True only if len(data) == MAX_SERVER_PAGE
and (offset + limit) < MAX_SERVER_PAGE (otherwise False); update the pagination
metadata accordingly and update the docstring/comments in list_notifications to
clearly state the 25-item server cap.
| # Nextcloud caps notification API responses at 25 items server-side, | ||
| # so we test client pagination within that constraint. | ||
| NC_NOTIF_LIMIT = 25 |
There was a problem hiding this comment.
Tie the notification assertions to the notifications created in each test.
.github/workflows/tests-integration.yml, Lines 71-78 install apps and run a mail sync before the suite, and Lines 117-119 here already note the 25-item server window. Lines 126-149 only assert counts/uniqueness, and Line 156 inspects result["data"][0], so ambient notifications can satisfy these checks or make test_formatted_fields inspect the wrong item. Clear notifications first, or assert against the generated pagtest-* / format-check subjects before checking pagination and formatting.
Also applies to: 123-149, 152-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_pagination.py` around lines 117 - 119, The tests
assume a 25-item server-side window (NC_NOTIF_LIMIT) but currently rely on
ambient notifications; update the pagination tests to either clear existing
notifications before creating test notifications or filter/assert only against
the notifications your test created (subjects starting with "pagtest-" and
"format-check"). Concretely, add a notification-cleanup step at the start of the
pagination/formatting tests (or call the existing notification deletion helper)
and/or change assertions that inspect result["data"][0] and the
counts/uniqueness checks to locate items by their subject prefix ("pagtest-" /
"format-check") instead of relying on array positions so the tests only validate
the notifications your test code generated.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/integration/test_pagination.py (1)
366-400:⚠️ Potential issue | 🟠 MajorScope notification assertions to test-created subjects (still nondeterministic).
This still relies on ambient notification state (counts and
result["data"][0]at Line 400), so the tests can validate the wrong records. This is the same unresolved issue raised earlier.Proposed fix
- result = json.loads(await nc_mcp.call("list_notifications", limit=10)) + result = json.loads(await nc_mcp.call("list_notifications", limit=self.NC_NOTIF_LIMIT)) + ours = [n for n in result["data"] if str(n.get("subject", "")).startswith("pagtest-")] + assert len(ours) >= 10 ... - notif = result["data"][0] + notif = next(n for n in result["data"] if n.get("subject") == "format-check")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_pagination.py` around lines 366 - 400, Tests rely on ambient notifications; scope assertions to only notifications created by the test by generating notifications with a unique subject prefix (e.g., in test_limit_and_offset, test_full_traversal, test_formatted_fields) and then, after calling nc_mcp.call("list_notifications"), filter result["data"] to only entries whose "subject" matches that prefix before checking pagination counts, has_more, or accessing result["data"][0]; update assertions in test_limit_and_offset, test_full_traversal, and test_formatted_fields to use the filtered list (and compute expected counts based on that filtered list) instead of assuming global ordering or that result["data"][0] is the test-created notification.
🧹 Nitpick comments (1)
tests/integration/test_pagination.py (1)
223-224: Avoid treatingpagination.countas total dataset size.At Line 223 (and similarly Lines 273, 326, 450, 485),
pagination["count"]is the current page size, not a global total. This becomes flaky when records exceed the setup call’slimit.Suggested pattern
- self.total = result["pagination"]["count"] + self.total = 0 + offset = 0 + while True: + page = json.loads(await nc_mcp.call("list_conversations", limit=200, offset=offset)) + self.total += page["pagination"]["count"] + if not page["pagination"]["has_more"]: + break + offset += 200Also applies to: 273-274, 326-327, 450-451, 485-486
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/seed_pagination_data.py`:
- Around line 145-146: Wrap the XML parsing call ET.fromstring(resp.text) in a
try/except that catches xml.etree.ElementTree.ParseError (or ET.ParseError) so
malformed PROPFIND responses don’t crash execution; on exception, log or handle
the bad response (e.g., skip processing this resp, return None, or continue)
before attempting to use root or call
root.find(".//{http://owncloud.org/ns}fileid"); update the code around
ET.fromstring and any subsequent use of root/fileid to be guarded by this error
handling.
- Around line 24-26: The current helper _ocs_data returning object prevents
pyright from validating downstream indexing; replace it with two typed helpers
(e.g., _ocs_data_dict -> Dict[str, Any] and _ocs_data_list -> List[Any]) and
import typing names (Any, Dict, List). Update call sites that expect a mapping
to use _ocs_data_dict and those expecting sequences to use _ocs_data_list,
keeping the same extraction logic (resp.json()["ocs"]["data"]) so pyright can
infer indexing/iteration types for functions that currently call _ocs_data.
---
Duplicate comments:
In `@tests/integration/test_pagination.py`:
- Around line 366-400: Tests rely on ambient notifications; scope assertions to
only notifications created by the test by generating notifications with a unique
subject prefix (e.g., in test_limit_and_offset, test_full_traversal,
test_formatted_fields) and then, after calling
nc_mcp.call("list_notifications"), filter result["data"] to only entries whose
"subject" matches that prefix before checking pagination counts, has_more, or
accessing result["data"][0]; update assertions in test_limit_and_offset,
test_full_traversal, and test_formatted_fields to use the filtered list (and
compute expected counts based on that filtered list) instead of assuming global
ordering or that result["data"][0] is the test-created notification.
🪄 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: 89b17c1a-9ff9-4293-b965-a1bc0f295e01
📒 Files selected for processing (4)
.github/workflows/tests-integration.ymlpyproject.tomlscripts/seed_pagination_data.pytests/integration/test_pagination.py
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/tests-integration.yml
- pyproject.toml
| def _ocs_data(resp: niquests.Response) -> object: | ||
| """Extract data from an OCS JSON response.""" | ||
| return resp.json()["ocs"]["data"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/seed_pagination_data.pyRepository: cloud-py-api/nc_mcp_server
Length of output: 8309
Fix the OCS helper typing contract (current form blocks pyright).
At Line 24, returning object makes all downstream indexing/iteration unknown (lines 45, 99, 106, 110, etc.), which matches pyright lint failures. The OCS endpoints return heterogeneous structures—some return lists, others return dicts—so split into two typed helpers so pyright can validate usage.
Proposed fix
+from typing import Any, cast
...
-def _ocs_data(resp: niquests.Response) -> object:
- """Extract data from an OCS JSON response."""
- return resp.json()["ocs"]["data"]
+def _ocs_data_list(resp: niquests.Response) -> list[dict[str, Any]]:
+ """Extract list payload from an OCS JSON response."""
+ return cast(list[dict[str, Any]], resp.json()["ocs"]["data"])
+
+
+def _ocs_data_dict(resp: niquests.Response) -> dict[str, Any]:
+ """Extract object payload from an OCS JSON response."""
+ return cast(dict[str, Any], resp.json()["ocs"]["data"])- existing = {r["name"] for r in _ocs_data(s.get(api))}
+ existing = {r["name"] for r in _ocs_data_list(s.get(api))}
...
- collectives = _ocs_data(s.get(f"{api}/collectives"))
+ collectives = _ocs_data_dict(s.get(f"{api}/collectives"))
...
- coll = _ocs_data(resp)["collective"]
+ coll = _ocs_data_dict(resp)["collective"]
...
- pages_data = _ocs_data(s.get(f"{api}/collectives/{coll_id}/pages"))
+ pages_data = _ocs_data_dict(s.get(f"{api}/collectives/{coll_id}/pages"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed_pagination_data.py` around lines 24 - 26, The current helper
_ocs_data returning object prevents pyright from validating downstream indexing;
replace it with two typed helpers (e.g., _ocs_data_dict -> Dict[str, Any] and
_ocs_data_list -> List[Any]) and import typing names (Any, Dict, List). Update
call sites that expect a mapping to use _ocs_data_dict and those expecting
sequences to use _ocs_data_list, keeping the same extraction logic
(resp.json()["ocs"]["data"]) so pyright can infer indexing/iteration types for
functions that currently call _ocs_data.
| root = ET.fromstring(resp.text) | ||
| fileid_el = root.find(".//{http://owncloud.org/ns}fileid") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "seed_pagination_data.py" -type f 2>/dev/null | head -5Repository: cloud-py-api/nc_mcp_server
Length of output: 52
🏁 Script executed:
fd -t f "seed_pagination_data.py" || find . -name "seed_pagination_data.py" 2>/dev/nullRepository: cloud-py-api/nc_mcp_server
Length of output: 101
🏁 Script executed:
git ls-files | grep -i seed_paginationRepository: cloud-py-api/nc_mcp_server
Length of output: 101
🏁 Script executed:
find . -type f -name "*.py" | head -20Repository: cloud-py-api/nc_mcp_server
Length of output: 784
🏁 Script executed:
ls -la scripts/ 2>/dev/null || find . -type d -name scripts 2>/dev/null | head -5Repository: cloud-py-api/nc_mcp_server
Length of output: 253
🏁 Script executed:
git ls-files scripts/Repository: cloud-py-api/nc_mcp_server
Length of output: 101
🏁 Script executed:
cat -n scripts/seed_pagination_data.pyRepository: cloud-py-api/nc_mcp_server
Length of output: 8309
Guard XML parsing against malformed PROPFIND responses.
At Line 145, ET.fromstring(resp.text) can raise ParseError if the response contains invalid XML. Wrap the parsing in a try-except block to handle malformed responses gracefully.
Proposed fix
- root = ET.fromstring(resp.text)
+ try:
+ root = ET.fromstring(resp.text)
+ except ET.ParseError:
+ print(" WARNING: invalid PROPFIND XML for comments, skipping")
+ return📝 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.
| root = ET.fromstring(resp.text) | |
| fileid_el = root.find(".//{http://owncloud.org/ns}fileid") | |
| try: | |
| root = ET.fromstring(resp.text) | |
| except ET.ParseError: | |
| print(" WARNING: invalid PROPFIND XML for comments, skipping") | |
| return | |
| fileid_el = root.find(".//{http://owncloud.org/ns}fileid") |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 145-145: pyright: Argument of type 'str | None' cannot be assigned to parameter 'text' of type 'str | ReadableBuffer' in function 'XML' (reportArgumentType).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed_pagination_data.py` around lines 145 - 146, Wrap the XML parsing
call ET.fromstring(resp.text) in a try/except that catches
xml.etree.ElementTree.ParseError (or ET.ParseError) so malformed PROPFIND
responses don’t crash execution; on exception, log or handle the bad response
(e.g., skip processing this resp, return None, or continue) before attempting to
use root or call root.find(".//{http://owncloud.org/ns}fileid"); update the code
around ET.fromstring and any subsequent use of root/fileid to be guarded by this
error handling.
|
(AI) Ready for review. All CI checks green. Changes since draft:
|
2 participants
Summary
Add
limitandoffsetparameters to 8 list tools that previously returned unbounded results, preventing context overflow on busy Nextcloud instances.Tools updated:
list_directorylist_conversationsget_eventslist_shareslist_notificationslist_trashlist_collectivesget_collective_pagesAll return
{"data": [...], "pagination": {"count", "offset", "limit", "has_more"}}.Additional fixes:
_format_notification()to strip noisy OCS fields (subjectRich, messageRich, icon, shouldNotify)limitkey tosearch_filesandlist_commentspagination dictsdefault=strtoget_eventsjson.dumpsTest infrastructure:
scripts/seed_pagination_data.py) that populates 55 items across 6 apps: files, talk, calendar, trash, collectives, commentsTest plan