Add Circles (Teams) tools — 14 tools via OCS#51
Merged
Conversation
📝 WalkthroughWalkthroughAdds Circles (Teams) MCP tools and registration, comprehensive integration tests and cleanup, README/PROGRESS updates, and a CI workflow step that attempts to enable the Nextcloud Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "MCP Client"
participant MCP as "nc_mcp_server (circles tools)"
participant OCS as "Nextcloud OCS API"
rect rgba(200,200,255,0.5)
Client->>MCP: call create_circle(name, config)
MCP->>OCS: POST /ocs/v2.php/apps/circles/api/v1/circles (payload)
OCS-->>MCP: 201 {circle_id, details}
MCP-->>Client: return JSON string with created circle
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… relying on 'bob'
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
+ Coverage 95.74% 95.99% +0.24%
==========================================
Files 29 30 +1
Lines 3105 3222 +117
==========================================
+ Hits 2973 3093 +120
+ Misses 132 129 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
(AI) CI green on both NC 32 and NC 33 after the fixture fix (peer user provisioned via fixture). 22 Circles integration tests + 8 server registration tests + 104 unit tests all pass. Marked ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/integration/test_circles.py (1)
50-55: Pagination assertion can be tighter.
len(circles) <= 2is satisfied even if the server ignoredlimitand returned0. Since the test just created threemcp-test-circle-page-*circles and the autouse cleanup runs before each test, asserting== 2gives you a real pagination check.♻️ Proposed nit
circles: list[dict[str, Any]] = json.loads(await nc_mcp.call("list_circles", limit=2)) - assert len(circles) <= 2 + assert len(circles) == 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_circles.py` around lines 50 - 55, The pagination test in test_list_pagination is too permissive; change the assertion to require exactly 2 results so the server honor of the "limit" parameter is actually verified: after creating three circles via _make_circle and calling nc_mcp.call("list_circles", limit=2) in test_list_pagination, replace the current len(circles) <= 2 assertion with an assertion that len(circles) == 2..github/workflows/tests-integration.yml (1)
76-76: Silent fallback will surface as noisy test failures ifcirclesisn't available.If
app:enable circlesfails on a future NC image where the bundled app is absent, every test intests/integration/test_circles.pywill hit the Nextcloud API and fail rather than skip. Consider gating the circles tests on a module/session fixture that verifies the app is enabled (e.g.GET /ocs/v2.php/apps/circles/circlesandpytest.skip(...)on failure), so CI on bare-bones images doesn't turn red. Current behavior is fine for NC 32/33 but this makes the “may not be shipped” comment actually actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests-integration.yml at line 76, The CI job silently ignores failure of `php occ app:enable circles`, causing tests in tests/integration/test_circles.py to fail noisily when the app is absent; add a module- or session-scoped pytest fixture (referencing tests/integration/test_circles.py) that checks the app is enabled by performing an HTTP GET to the Circles OCS endpoint (e.g. GET /ocs/v2.php/apps/circles/circles) using the existing test HTTP client, and call pytest.skip(...) if the request indicates the app is missing or returns a non-success status; ensure all tests in test_circles.py depend on that fixture so CI on bare-bones Nextcloud images will skip instead of fail.
🤖 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_circles.py`:
- Around line 188-202: The test class TestJoinLeave currently only exercises
create_circle → add_circle_member → remove_circle_member and never calls the
join_circle or leave_circle endpoints; update tests to actually cover both flows
by (1) adding a new test that creates an OPEN circle (config=16) using
McpTestHelper/admin, then authenticates a separate peer client (e.g.,
instantiate a NextcloudClient or a peer-authenticated McpTestHelper using
circle_peer credentials) and calls the join_circle endpoint, then asserts the
peer appears via list_circle_members; and (2) add a test where a non-owner peer
calls leave_circle (after being added or joined) and assert they are removed
from list_circle_members while the circle still exists; alternatively, if you
cannot add these tests now, rename TestJoinLeave to TestMemberRemoval to reflect
the current coverage.
---
Nitpick comments:
In @.github/workflows/tests-integration.yml:
- Line 76: The CI job silently ignores failure of `php occ app:enable circles`,
causing tests in tests/integration/test_circles.py to fail noisily when the app
is absent; add a module- or session-scoped pytest fixture (referencing
tests/integration/test_circles.py) that checks the app is enabled by performing
an HTTP GET to the Circles OCS endpoint (e.g. GET
/ocs/v2.php/apps/circles/circles) using the existing test HTTP client, and call
pytest.skip(...) if the request indicates the app is missing or returns a
non-success status; ensure all tests in test_circles.py depend on that fixture
so CI on bare-bones Nextcloud images will skip instead of fail.
In `@tests/integration/test_circles.py`:
- Around line 50-55: The pagination test in test_list_pagination is too
permissive; change the assertion to require exactly 2 results so the server
honor of the "limit" parameter is actually verified: after creating three
circles via _make_circle and calling nc_mcp.call("list_circles", limit=2) in
test_list_pagination, replace the current len(circles) <= 2 assertion with an
assertion that len(circles) == 2.
🪄 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: 4f0a6079-3927-4486-972f-af3215d63ba0
📒 Files selected for processing (8)
.github/workflows/tests-integration.ymlPROGRESS.mdREADME.mdsrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/circles.pytests/integration/conftest.pytests/integration/test_circles.pytests/integration/test_server.py
…ctually exercise join_circle/leave_circle
Contributor
Author
|
(AI) Addressed all three CodeRabbit findings in 6624df7:
Local: 23/23 pass (131s). CI re-running. |
…Iterator for @asynccontextmanager
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/test_circles.py (1)
65-89:_as_peerteardown can shadow the original test exception.If the body under
yieldraises, thefinallyblock runsset_state(...)(fine) and thenawait peer_client.close(). If the close itself raises (e.g., transient network error, cancelled task during event-loop teardown), that new exception replaces the original test failure as the visible error, making triage harder. Since the only purpose of close in the finally is resource cleanup, it's worth suppressing failures here.♻️ Proposed tweak
try: yield finally: set_state(admin_client, admin_config) - await peer_client.close() + with contextlib.suppress(Exception): + await peer_client.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_circles.py` around lines 65 - 89, The teardown in the asynccontextmanager _as_peer can let exceptions raised by await peer_client.close() mask an original test failure; update the finally block in _as_peer (where set_state(admin_client, admin_config) and await peer_client.close() are called) to catch and suppress (or log) any exceptions from peer_client.close() so cleanup failures do not replace the original exception — i.e., wrap the await peer_client.close() in a try/except Exception: (or asyncio.CancelledError handling as appropriate) and swallow or log the error instead of letting it propagate.
🤖 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_circles.py`:
- Around line 116-120: Tests test_create_sets_owner_to_caller and
test_owner_present_in_members hardcode "admin" as the expected initiator userId;
change them to read the active admin from the runtime config instead. Locate the
assertions referencing created["initiator"]["userId"] and replace the literal
"admin" with the configured value (e.g., Config.user or the NEXTCLOUD_USER env
var via os.environ.get("NEXTCLOUD_USER")), ensuring you import/access the same
Config used by the test harness (or nc_mcp helper) so the assertion uses the
actual admin account name in the environment.
---
Nitpick comments:
In `@tests/integration/test_circles.py`:
- Around line 65-89: The teardown in the asynccontextmanager _as_peer can let
exceptions raised by await peer_client.close() mask an original test failure;
update the finally block in _as_peer (where set_state(admin_client,
admin_config) and await peer_client.close() are called) to catch and suppress
(or log) any exceptions from peer_client.close() so cleanup failures do not
replace the original exception — i.e., wrap the await peer_client.close() in a
try/except Exception: (or asyncio.CancelledError handling as appropriate) and
swallow or log the error instead of letting it propagate.
🪄 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: c9be8417-4b85-465f-bf43-60a922747c9d
📒 Files selected for processing (2)
PROGRESS.mdtests/integration/test_circles.py
🚧 Files skipped from review as they are similar to previous changes (1)
- PROGRESS.md
…' in owner assertions (circles + forms)
Deep re-review against the Nextcloud Circles source and live API surfaced three docstring bugs and one permission mismatch: - update_circle_member_level: docstring claimed it returned the updated circle, but the server returns the updated MEMBER (verified in FederatedItems/MemberLevel.php — outcome is the cloned member with the new level). - search_circles: docstring named a "source" field; actual response has "userType" and keys ['displayName','id','instance','userId','userType']. - leave_circle: docstring claimed it returned a membership record; the server actually returns the CIRCLE object (CircleLeave::verify calls circleRequest->getCircle and serializes it as the outcome). Empty when the caller loses visibility (sole-owner destroy case). - leave_circle permission: was WRITE + ADDITIVE_IDEMPOTENT. Sole-owner leave silently destroys the whole circle, and the matching leave_conversation tool in Talk is DESTRUCTIVE. Align annotations and permission level to DESTRUCTIVE. New tests cover the gaps the review surfaced: offset pagination, full_details=True response shape, owner transfer via update_circle_member_level (plus peer cleanup for the post-transfer circle), join on a non-OPEN circle failing, sole-owner leave destroying the circle, and the new permission gate on leave_circle.
Contributor
Author
|
(AI) Deep re-review of the feature surfaced three docstring bugs and one permission mismatch — all verified against the Nextcloud Circles source and live API calls before pushing a fix. Real bugs
Safety/consistency
Test gaps closed
Circles test count: 23 → 29. Commit: 0a68ff4. Monitoring CI. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/test_circles.py (1)
65-89:_as_peerrelies on shared global state — document the sequential-execution assumption.
set_state()swaps the module-level singleton client/config, so any concurrent test (e.g., underpytest-xdistwith processes/threads sharing the interpreter, or parallelasynciotests within the same module) that calls a Circles tool while_as_peeris active would execute under the wrong identity. This is safe today because pytest-asyncio runs tests sequentially, but a future concurrency change could silently break auth assumptions without a visible failure.Low-risk mitigation: add a short comment on the helper noting the sequential-only assumption, or guard with an
asyncio.Lockat module scope to document intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_circles.py` around lines 65 - 89, The _as_peer helper swaps module-global client/config via set_state (using get_client/get_config and NextcloudClient/peer_client) which is unsafe under concurrent tests; add a brief comment above _as_peer stating it assumes sequential test execution and is not concurrency-safe, and protect the swap by introducing a module-level asyncio.Lock used by _as_peer to serialize entry/exit (acquire before changing state and release after restoring state and closing peer_client) so concurrent tasks can't observe or corrupt the global client/config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/test_circles.py`:
- Around line 65-89: The _as_peer helper swaps module-global client/config via
set_state (using get_client/get_config and NextcloudClient/peer_client) which is
unsafe under concurrent tests; add a brief comment above _as_peer stating it
assumes sequential test execution and is not concurrency-safe, and protect the
swap by introducing a module-level asyncio.Lock used by _as_peer to serialize
entry/exit (acquire before changing state and release after restoring state and
closing peer_client) so concurrent tasks can't observe or corrupt the global
client/config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b0fdc77-1d60-48fd-9518-34597bbf47cb
📒 Files selected for processing (4)
PROGRESS.mdsrc/nc_mcp_server/tools/circles.pytests/integration/test_circles.pytests/integration/test_forms.py
🚧 Files skipped from review as they are similar to previous changes (1)
- PROGRESS.md
Contributor
Author
|
(AI) Addressed the CodeRabbit nitpick in 112b39a — added a short |
…ent auto-mutations
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/test_circles.py (1)
297-314: Consider moving the peer-auth cleanup into afinallyblock.If either assertion at lines 310–311 fails after ownership has been transferred, the peer-authenticated
delete_circleat 313–314 never runs, and the remaining circle is owned solely byCIRCLE_TEST_USER. Whether the admin-credentialed conftest cleanup can then delete it depends on admin privileges on the running NC version, which could leak state across runs.♻️ Proposed fix
async def test_promote_to_owner_transfers_and_demotes_caller(self, nc_mcp: McpTestHelper, circle_peer: str) -> None: """Promoting a member to owner transfers ownership; previous owner becomes admin (level=8).""" circle = await _make_circle(nc_mcp, "mcp-test-circle-xfer-owner") added = json.loads(await nc_mcp.call("add_circle_member", circle_id=circle["id"], user_id=circle_peer)) await nc_mcp.call( "update_circle_member_level", circle_id=circle["id"], member_id=added["id"], level="owner", ) - members: list[dict[str, Any]] = json.loads(await nc_mcp.call("list_circle_members", circle_id=circle["id"])) - peer_level = next(m["level"] for m in members if m.get("userId") == circle_peer) - caller_level = next(m["level"] for m in members if m.get("userId") == get_config().user) - assert peer_level == 9, "promoted member should be owner (9)" - assert caller_level == 8, "previous owner should be demoted to admin (8)" - # peer is now the only owner — admin cleanup can't destroy it. Tear down as peer. - async with _as_peer(circle_peer, CIRCLE_TEST_PWD): - await nc_mcp.call("delete_circle", circle_id=circle["id"]) + try: + members: list[dict[str, Any]] = json.loads( + await nc_mcp.call("list_circle_members", circle_id=circle["id"]) + ) + peer_level = next(m["level"] for m in members if m.get("userId") == circle_peer) + caller_level = next(m["level"] for m in members if m.get("userId") == get_config().user) + assert peer_level == 9, "promoted member should be owner (9)" + assert caller_level == 8, "previous owner should be demoted to admin (8)" + finally: + # peer is now the only owner — admin cleanup can't destroy it. Tear down as peer. + async with _as_peer(circle_peer, CIRCLE_TEST_PWD): + await nc_mcp.call("delete_circle", circle_id=circle["id"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_circles.py` around lines 297 - 314, The test_promote_to_owner_transfers_and_demotes_caller test should guarantee the peer-authenticated cleanup runs even if assertions fail: wrap the section from adding/updating the member through the assertions in a try/finally, and move the async with _as_peer(circle_peer, CIRCLE_TEST_PWD): await nc_mcp.call("delete_circle", circle_id=circle["id"]) into the finally block; ensure you check that circle and its id still exist (created by _make_circle) before attempting delete to avoid raising new errors, and keep calls to add_circle_member, update_circle_member_level, list_circle_members, get_config, and delete_circle unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/test_circles.py`:
- Around line 297-314: The test_promote_to_owner_transfers_and_demotes_caller
test should guarantee the peer-authenticated cleanup runs even if assertions
fail: wrap the section from adding/updating the member through the assertions in
a try/finally, and move the async with _as_peer(circle_peer, CIRCLE_TEST_PWD):
await nc_mcp.call("delete_circle", circle_id=circle["id"]) into the finally
block; ensure you check that circle and its id still exist (created by
_make_circle) before attempting delete to avoid raising new errors, and keep
calls to add_circle_member, update_circle_member_level, list_circle_members,
get_config, and delete_circle unchanged otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 426c8313-7e1c-44e2-9fc8-bf937155e7ee
📒 Files selected for processing (3)
PROGRESS.mdsrc/nc_mcp_server/tools/circles.pytests/integration/test_circles.py
✅ Files skipped from review due to trivial changes (2)
- PROGRESS.md
- src/nc_mcp_server/tools/circles.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 14 MCP tools for the Nextcloud Circles / Teams app using the OCS LocalController API at
/ocs/v2.php/apps/circles/.... Full circle and membership management: create, rename, configure, add/remove members, promote/demote, join/leave, search, delete.Why Circles: only pure-OCS app remaining with substantial scope that matches the OCS-only invariant. Bookmarks/Notes/Deck/Photos all require non-OCS REST endpoints and were declined.
Tools
list_circles,get_circle,list_circle_members,search_circlescreate_circle,update_circle_name,update_circle_description,update_circle_config,add_circle_member,update_circle_member_level,join_circle,leave_circledelete_circle,remove_circle_memberDesign notes
idof a member is its membership record id (not the user'suserIdorsingleId); use theidfield returned bylist_circle_memberswhen callingremove_circle_member/update_circle_member_level.member_typeaccepts human-readable strings ("user","group","mail","contact","circle"), mapped server-side to the integer constants (1, 2, 4, 8, 16).levelforupdate_circle_member_levelaccepts"member"/"moderator"/"admin"/"owner"(1/4/8/9).update_circle_configtakes the raw bitmask int; docstring documents the flag values (VISIBLE=8, OPEN=16, INVITE=32, HIDDEN=1024, etc.).leave_circledocstring warns: leaving as the sole owner destroys the circle (verified live against NC 34). Promote another member to"owner"first to avoid this./admin/{emulated}/...) intentionally omitted — callers are already authenticated as themselves; emulated ops add no value for MCP.Total tool count: 126 → 140 (+
upload_file_from_pathconditional → 141).Test plan
test_server.py::test_all_tools_registeredsees all 14 new toolsocc app:enable circles(circles is bundled but not always enabled on minimal images)Summary by CodeRabbit
New Features
Documentation
Tests