Add Collectives tools#31
Merged
Merged
Conversation
…llective_page, create_collective, create_collective_page, delete_collective, delete_collective_page
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Nextcloud Collectives support: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FastMCP as FastMCP Server
participant Tool as Collectives Tool
participant Client as NextcloudClient
participant NC as Nextcloud OCS
User->>FastMCP: invoke collectives tool (e.g., create_collective)
FastMCP->>Tool: dispatch request & params
Tool->>Client: ocs_post / ocs_patch / ocs_get / ocs_delete (API path, data)
Client->>NC: HTTP OCS request
NC-->>Client: OCS JSON response
Client-->>Tool: parsed ocs.data
Tool-->>FastMCP: formatted JSON result or confirmation
FastMCP-->>User: tool response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 95.48% 95.80% +0.31%
==========================================
Files 21 22 +1
Lines 1396 1501 +105
==========================================
+ Hits 1333 1438 +105
Misses 63 63
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: 2
🧹 Nitpick comments (1)
tests/integration/test_collectives.py (1)
23-27: Avoid index-based landing page selection in helper.Picking
pages[0]makes tests brittle to ordering changes. Resolve landing page by explicit predicate (and fail with a clear assertion if missing).🔍 More robust helper
async def _get_landing_page_id(nc_mcp: McpTestHelper, collective_id: int) -> int: result = await nc_mcp.call("get_collective_pages", collective_id=collective_id) pages = json.loads(result) - return pages[0]["id"] + for page in pages: + if page.get("title") == "Landing page": + return int(page["id"]) + raise AssertionError(f"Landing page not found for collective {collective_id}")🤖 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 23 - 27, The helper _get_landing_page_id currently returns pages[0], which is brittle; instead load the pages from get_collective_pages into the pages list and search for the landing page by an explicit predicate (e.g., check page.get("is_landing") or page.get("isLanding") or page.get("type") == "landing" or other project-specific landing marker) and return that page's "id"; if no page matches, raise/assert with a clear message indicating no landing page found for the given collective_id so tests fail fast and clearly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests-integration.yml:
- Line 73: The CI step currently swallows failures by appending `|| echo
"collectives already installed"` to the install command; replace that fallback
so the job fails on real errors — remove the `|| echo ...` (or replace it with
an explicit failure like `|| exit 1`) for the `php occ app:install collectives`
invocation (the `$OCC "php occ app:install collectives"` command) so install
errors surface and break the CI run instead of being masked.
In `@src/nc_mcp_server/tools/collectives.py`:
- Line 60: The code calls data.get(...) unconditionally which crashes when data
is a list; update the places that build collectives, pages, and page to guard
the shape first: when constructing collectives use something like source = data
if isinstance(data, list) else data.get("collectives", []) (or source = [] if
not dict/list) and pass items to _format_collective; similarly for pages use
pages_source = data.get("pages", data) only after confirming isinstance(data,
dict) (otherwise treat data as the pages list), and for page read page_value =
data.get("page") only if isinstance(data, dict) else set a sensible default;
replace the three unconditional data.get(...) uses (the collectives list
comprehension, the pages handling, and the page access) with these isinstance
checks so list responses no longer call .get().
---
Nitpick comments:
In `@tests/integration/test_collectives.py`:
- Around line 23-27: The helper _get_landing_page_id currently returns pages[0],
which is brittle; instead load the pages from get_collective_pages into the
pages list and search for the landing page by an explicit predicate (e.g., check
page.get("is_landing") or page.get("isLanding") or page.get("type") == "landing"
or other project-specific landing marker) and return that page's "id"; if no
page matches, raise/assert with a clear message indicating no landing page found
for the given collective_id so tests fail fast and clearly.
🪄 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: 5d600d8a-62f7-428f-9dfd-ff8f60044f6d
📒 Files selected for processing (6)
.github/workflows/tests-integration.ymlsrc/nc_mcp_server/client.pysrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/collectives.pytests/integration/test_collectives.pytests/integration/test_server.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/nc_mcp_server/tools/collectives.py (1)
55-56:⚠️ Potential issue | 🟠 MajorGuard raw-list responses before accessing
.get().These two handlers try to support both wrapped dict payloads and bare list payloads, but the current expression still calls
.get()first. If Nextcloud returns a raw list here, both tools fail withAttributeErrorinstead of using the fallback path.🛠️ Suggested fix
- collectives = [_format_collective(c) for c in data.get("collectives", data if isinstance(data, list) else [])] + raw_collectives = ( + data.get("collectives", []) + if isinstance(data, dict) + else data + if isinstance(data, list) + else [] + ) + collectives = [_format_collective(c) for c in raw_collectives] return json.dumps(collectives, indent=2, default=str) ... - pages = [_format_page(p) for p in data.get("pages", data if isinstance(data, list) else [])] + raw_pages = ( + data.get("pages", []) + if isinstance(data, dict) + else data + if isinstance(data, list) + else [] + ) + pages = [_format_page(p) for p in raw_pages] return json.dumps(pages, indent=2, default=str)Also applies to: 74-75
🤖 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 55 - 56, The current expression calls .get() on data which breaks if Nextcloud returns a bare list; update the handling after the client.ocs_get(...) call to first check the type of data: if data is a dict, read items = data.get("collectives", []); else if data is a list, set items = data; otherwise items = []. Then build collectives = [_format_collective(c) for c in items]. Apply the same defensive check to the similar block around lines 74-75 that handles other collectives responses.
🤖 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/collectives.py`:
- Around line 28-39: _format_page currently strips the page body and only
returns metadata, causing get_collective_page() to lose the page content; add a
page-detail serializer (or extend _format_page with an include_body flag) that
preserves the body/content field (e.g., "body" or "content" from the OCS
payload) and use that serializer in get_collective_page() instead of the
metadata-only _format_page; also replace the other single-page return sites (the
code referenced around the second occurrence) to use the detail serializer so
single-page reads include the full content.
In `@tests/integration/test_collectives.py`:
- Around line 138-157: Both tests only assert metadata and miss verifying the
page content, so update test_get_landing_page and test_get_page_includes_tags
(which call _get_landing_page_id and get_collective_page) to assert the returned
page includes the normalized body; specifically add assertions that "body" is
present in page and that page["body"].get("normalized") (or otherwise non-empty
normalized content) is truthy to ensure the API returns the page content, not
just metadata.
- Around line 323-330: The test function
test_write_allows_create_but_blocks_trash should guarantee cleanup even if the
pytest.raises assertion fails: wrap the create + assert block in try/except or
try/finally, set coll = None before calling nc_mcp_write.call, then in a finally
block check if coll is set and call client.ocs_delete for both the normal and
trash endpoints (using coll["id"]) via nc_mcp_write.client; this ensures the
manual ocs_delete cleanup always runs regardless of assertion outcome.
---
Duplicate comments:
In `@src/nc_mcp_server/tools/collectives.py`:
- Around line 55-56: The current expression calls .get() on data which breaks if
Nextcloud returns a bare list; update the handling after the client.ocs_get(...)
call to first check the type of data: if data is a dict, read items =
data.get("collectives", []); else if data is a list, set items = data; otherwise
items = []. Then build collectives = [_format_collective(c) for c in items].
Apply the same defensive check to the similar block around lines 74-75 that
handles other collectives responses.
🪄 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: 191ce5a7-3f3f-4d00-808f-726c42ec2566
📒 Files selected for processing (3)
src/nc_mcp_server/tools/collectives.pytests/integration/test_collectives.pytests/integration/test_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_server.py
Comment on lines
+28
to
+39
| def _format_page(p: dict[str, Any]) -> dict[str, Any]: | ||
| return { | ||
| "id": p["id"], | ||
| "title": p.get("title", ""), | ||
| "emoji": p.get("emoji"), | ||
| "timestamp": p.get("timestamp"), | ||
| "size": p.get("size"), | ||
| "file_name": p.get("fileName"), | ||
| "file_path": p.get("filePath"), | ||
| "last_user_id": p.get("lastUserId"), | ||
| "tags": p.get("tags", []), | ||
| } |
There was a problem hiding this comment.
Use a page-detail serializer for get_collective_page().
get_collective_page() promises the page content, but _format_page() only keeps metadata. As written, single-page reads always strip the body from the OCS payload.
🛠️ Suggested fix
def _format_page(p: dict[str, Any]) -> dict[str, Any]:
return {
"id": p["id"],
"title": p.get("title", ""),
"emoji": p.get("emoji"),
"timestamp": p.get("timestamp"),
"size": p.get("size"),
"file_name": p.get("fileName"),
"file_path": p.get("filePath"),
"last_user_id": p.get("lastUserId"),
"tags": p.get("tags", []),
}
+
+
+def _format_page_detail(p: dict[str, Any]) -> dict[str, Any]:
+ detail = _format_page(p)
+ for key, value in p.items():
+ if key not in {"id", "title", "emoji", "timestamp", "size", "fileName", "filePath", "lastUserId", "tags"}:
+ detail[key] = value
+ return detail
...
- page = data.get("page", data)
- return json.dumps(_format_page(page), indent=2, default=str)
+ page = data.get("page", data)
+ return json.dumps(_format_page_detail(page), indent=2, default=str)Also applies to: 92-95
🤖 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 28 - 39, _format_page
currently strips the page body and only returns metadata, causing
get_collective_page() to lose the page content; add a page-detail serializer (or
extend _format_page with an include_body flag) that preserves the body/content
field (e.g., "body" or "content" from the OCS payload) and use that serializer
in get_collective_page() instead of the metadata-only _format_page; also replace
the other single-page return sites (the code referenced around the second
occurrence) to use the detail serializer so single-page reads include the full
content.
Comment on lines
+138
to
+157
| async def test_get_landing_page(self, nc_mcp: McpTestHelper) -> None: | ||
| coll = await _create_collective(nc_mcp, "getpg") | ||
| try: | ||
| landing_id = await _get_landing_page_id(nc_mcp, coll["id"]) | ||
| result = await nc_mcp.call("get_collective_page", collective_id=coll["id"], page_id=landing_id) | ||
| page = json.loads(result) | ||
| assert page["id"] == landing_id | ||
| assert page["title"] == "Landing page" | ||
| finally: | ||
| await _destroy_collective(nc_mcp, coll["id"]) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_page_includes_tags(self, nc_mcp: McpTestHelper) -> None: | ||
| coll = await _create_collective(nc_mcp, "tags") | ||
| try: | ||
| landing_id = await _get_landing_page_id(nc_mcp, coll["id"]) | ||
| result = await nc_mcp.call("get_collective_page", collective_id=coll["id"], page_id=landing_id) | ||
| page = json.loads(result) | ||
| assert "tags" in page | ||
| assert isinstance(page["tags"], list) |
There was a problem hiding this comment.
Assert the page body in the single-page tests.
These cases only validate metadata, so get_collective_page() can drop the page content and still pass. Please add an assertion for the normalized body field here.
🤖 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 138 - 157, Both tests
only assert metadata and miss verifying the page content, so update
test_get_landing_page and test_get_page_includes_tags (which call
_get_landing_page_id and get_collective_page) to assert the returned page
includes the normalized body; specifically add assertions that "body" is present
in page and that page["body"].get("normalized") (or otherwise non-empty
normalized content) is truthy to ensure the API returns the page content, not
just metadata.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nc_mcp_server/tools/collectives.py (1)
28-39:⚠️ Potential issue | 🟠 MajorPreserve full page payload for single-page reads.
get_collective_page()promises content, but Line 95 serializes through_format_page()(Line 28), which drops non-metadata fields (including body/content). This breaks the tool contract for page reads.💡 Proposed fix
def _format_page(p: dict[str, Any]) -> dict[str, Any]: return { "id": p["id"], "title": p.get("title", ""), "emoji": p.get("emoji"), "timestamp": p.get("timestamp"), "size": p.get("size"), "file_name": p.get("fileName"), "file_path": p.get("filePath"), "last_user_id": p.get("lastUserId"), "tags": p.get("tags", []), } +def _format_page_detail(p: dict[str, Any]) -> dict[str, Any]: + detail = _format_page(p) + for key, value in p.items(): + if key not in {"id", "title", "emoji", "timestamp", "size", "fileName", "filePath", "lastUserId", "tags"}: + detail[key] = value + return detail ... - return json.dumps(_format_page(page), indent=2, default=str) + return json.dumps(_format_page_detail(page), indent=2, default=str)Also applies to: 80-95
🤖 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 28 - 39, The _format_page helper currently strips out non-metadata fields (e.g., body/content) causing get_collective_page to return incomplete page payloads; change the behavior so single-page reads preserve the full page: either adjust get_collective_page to return the raw page dict before calling _format_page or modify _format_page to detect a full-page read (presence of content/body field or a flag) and include all original keys (or at least include body/content, fileName/filePath and any non-metadata fields) instead of dropping them; update callers (get_collective_page and any list endpoints) so list responses still use the trimmed metadata shape while get_collective_page returns the complete page.
🤖 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/collectives.py`:
- Around line 28-39: The _format_page helper currently strips out non-metadata
fields (e.g., body/content) causing get_collective_page to return incomplete
page payloads; change the behavior so single-page reads preserve the full page:
either adjust get_collective_page to return the raw page dict before calling
_format_page or modify _format_page to detect a full-page read (presence of
content/body field or a flag) and include all original keys (or at least include
body/content, fileName/filePath and any non-metadata fields) instead of dropping
them; update callers (get_collective_page and any list endpoints) so list
responses still use the trimmed metadata shape while get_collective_page returns
the complete page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f50b65e2-4c86-4540-aee8-32c0d56f8cd6
📒 Files selected for processing (1)
src/nc_mcp_server/tools/collectives.py
This was referenced Mar 30, 2026
This was referenced Apr 23, 2026
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
Adds 7 Collectives tools for managing shared knowledge bases and wiki pages via OCS API.
Tools:
list_collectivesget_collective_pagesget_collective_pagecreate_collectivecreate_collective_pagedelete_collectivedelete_collective_pageAlso:
ocs_patchmethod to client (needed for future restore-from-trash support)Tested locally on: NC 33 (stable33) and NC 34 (master) — 21/21 pass on both.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores