Keep team folders from being deleted along with their teams

[oleksandr-nc]

On Nextcloud 35 with the Team folders app, Circles gives every new team a team folder: a shared folder every member sees in Files. POST /apps/circles/circles does so unless createTeamFolder is false, and Collectives' own teams always get one. Destroying the team deletes that folder for all members: its files are in no trash and no longer reachable in Nextcloud (Team folders only drops the folder's record, leaving the files in the data directory where just a server admin could dig them out). An older group folder an admin linked to the team goes the same way. The tools passed none of this on: create_circle quietly made a folder, and delete_circle, leave_circle (the owner leaving as the last member destroys the circle) and delete_collective(delete_team=true) deleted it without a word. Checked on stable35 and on master, where a file put into the folder was gone after deleting the team.

create_circle now makes a team folder only with team_folder=true, and reports it (team_folder, or null with team_folder_note saying why there is none). Nextcloud 34 ignores the flag.

delete_circle, leave_circle and delete_collective(delete_team=true) first ask Circles for the team's folder (GET /apps/circles/teams/{id}/folder) and refuse when it would be lost, unless called with delete_team_folder=true. Each refusal says that nothing was changed and how to keep the files: move them out, add a member before leaving (Circles then hands the circle over), or delete the collective without its team. Without delete_team_folder, a failed check stops the deletion as well, and a 403 there says the caller is not a member (Circles answers the same for circles that do not exist); with it, the caller has already agreed to lose the folder, so the deletion goes ahead.

leave_circle only refuses when the leave really destroys the circle: the caller is the owner and no other member entry is left. Circles makes any other entry the owner, including a group, a nested circle or a pending invitation, which was checked live (an invited user became owner and the folder stayed).

After a team folder is created, create_circle and create_collective log in again. On master, a session that started before the folder existed could not find it for minutes (uploads into it failed with "Not found"), while a new login could; renew_session already exists for the same effect with federated shares.

delete_user now says that the user leaving every team deletes the teams they own with no other member, with their team folders. leave_circle described the owner's leave as destroying the circle; it now says that ownership passes on unless no one is left. The README listed leave_circle as write; it is destructive.

CI installs the Team folders app, so the Nextcloud 35 job runs the new integration tests instead of skipping them. They check with the Team folders admin API that a folder is really gone after a deletion, since the caller loses access to it either way. On Nextcloud 34 they skip.

Tested on stable34, stable35 (35.0.1, Team folders 23) and master (Team folders 24) with the full integration suite, plus live scenarios run by an independent tester: refusals, forced deletions, owner handover to a member and to an invitee, a linked group folder, delete_user, and permission levels.

Summary by CodeRabbit

• New Features

  • Circle creation can now optionally create a team folder on Nextcloud 35+ when the Team folders app is enabled.
  • Collective creation reports the associated team folder’s mount point when available.

• Bug Fixes

  • Leaving or deleting a circle, or deleting a collective’s team, now protects associated team folders and files unless you explicitly confirm their deletion.
  • Circle ownership can be handed over when another member or invitee remains.

[oleksandr-nc]

@coderabbitai review

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 42d7e6ba-32ce-4d60-9406-bcce968fabae

📥 Commits

Reviewing files that changed from the base of the PR and between 34849e5 and 083cabe.

📒 Files selected for processing (12)

• .github/workflows/tests-integration.yml
• PROGRESS.md
• README.md
• src/nc_mcp_server/client.py
• src/nc_mcp_server/tools/circles.py
• src/nc_mcp_server/tools/collectives.py
• src/nc_mcp_server/tools/users.py
• tests/integration/conftest.py
• tests/integration/test_circles.py
• tests/integration/test_collectives.py
• tests/test_circles.py
• tests/test_collectives_pages.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Circle creation can now create and report associated team folders. Circle leave and deletion, and permanent Collective team deletion, check for team folders and require explicit consent when the action would remove one. The change adds unit and integration coverage and updates related documentation.

Changes

Team folder lifecycle

Layer / File(s)	Summary
Circle team-folder creation and lookup .github/workflows/tests-integration.yml, src/nc_mcp_server/client.py, src/nc_mcp_server/tools/circles.py, tests/integration/conftest.py, tests/integration/test_circles.py, tests/integration/test_collectives.py, tests/test_circles.py, tests/test_collectives_pages.py, README.md, PROGRESS.md	create_circle accepts team_folder and reports folder details when available. Integration tests detect whether Team folders are available and cover circle and Collective creation results.
Circle leave and deletion safeguards src/nc_mcp_server/tools/circles.py, tests/integration/test_circles.py, tests/test_circles.py, README.md, PROGRESS.md	leave_circle and delete_circle accept delete_team_folder. Without consent, they block actions that would remove an associated folder. Tests cover last-member departures, ownership handover, folder retention, and explicit deletion.
Collective folder reporting and deletion src/nc_mcp_server/tools/collectives.py, src/nc_mcp_server/tools/users.py, tests/integration/test_collectives.py, tests/test_collectives_pages.py, README.md, PROGRESS.md	create_collective reports the associated folder when lookup succeeds. delete_collective checks for a folder before permanent team deletion and accepts delete_team_folder to proceed. Tests and documentation cover the behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant create_circle
  participant NextcloudCircles
  participant get_team_folder
  Caller->>create_circle: request team_folder
  create_circle->>NextcloudCircles: create circle with createTeamFolder
  create_circle->>get_team_folder: look up folder
  get_team_folder-->>create_circle: folder details or lookup result
  create_circle-->>Caller: circle details and folder status

Loading

Merge Risk: ⚪ Minimal · up to 083ca

The change adds explicit consent before team operations remove associated folders. No actionable merge-blocking issue is established by the supplied evidence; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 083ca

Explicit folder-deletion consent improves the normal path, but the check is separate from the deletion it guards. A successful creation can also be reported as a failure if the subsequent session refresh fails. No new security vulnerability is verified, but these lifecycle guarantees need qualification.

Retained concerns

• Medium · reliability · inferred: After a circle and its folder are created, session renewal can fail before the tool returns the created circle ID. A retry can create another circle because names need not be unique, complicating ownership and cleanup of shared folders.

Security review details

Security Blast Radius

• inferred — A mistaken team deletion can affect the shared folder and files visible to that team's members, not merely the caller's view. The reviewed source does not establish a broader tenant-wide reach.

Security Findings and Attack Paths

• inferred — The new consent check uses a snapshot: another member could leave after the owner-last-member check but before the owner's leave request, or a folder link could change before deletion. The backend's locking behavior is unknown. This qualifies the new safeguard; the ability of these operations to delete a folder existed before the PR and is not established as a newly introduced exposure.

Trust Boundaries and Controls

• observed — Tool-level permission guards remain in place, while Nextcloud receives the authenticated requests and decides whether the caller may operate on the circle. Without consent, a detected folder or most failed prechecks stop the local destructive request.

Resilience and Maintainability Implications

• inferred — The creation response is not retry-safe if session renewal fails after the POST: the server-side team can exist without its ID reaching the caller. Folder lookup failures have a safer, separately handled response path.

Hardening Proposals

• proposed — Where the Nextcloud APIs permit it, bind folder-loss consent to the destructive operation or use a conditional state/version check. Reconcile a completed creation before recommending a retry after a post-create session failure.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 82 functions across 9 files. (3 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing team folders from being deleted when their teams are deleted.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 42.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 82 functions across 9 files. (3 skipped: 3 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.44%. Comparing base (34849e5) to head (083cabe).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   97.37%   97.44%   +0.07%     
==========================================
  Files          33       33              
  Lines        4832     4927      +95     
==========================================
+ Hits         4705     4801      +96     
+ Misses        127      126       -1     

Flag	Coverage Δ
integration	94.43% <77.88%> (-0.37%)	⬇️
nc34	93.72% <59.61%> (-0.75%)	⬇️
nc35	94.05% <75.00%> (-0.43%)	⬇️
py3.12	49.05% <100.00%> (+1.43%)	⬆️
py3.13	49.05% <100.00%> (+1.43%)	⬆️
py3.14	49.05% <100.00%> (+1.43%)	⬆️
session-cache	18.63% <11.53%> (-0.16%)	⬇️
unit	49.05% <100.00%> (+1.43%)	⬆️
user-permissions	40.77% <16.34%> (-0.58%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oleksandr-nc]

Thanks. On the retained concerns:

• Session renewal failing after the circle is created: renew_session cannot fail the tool there. _init_session_auth returns on any non-OK answer or OSError (niquests' request exceptions are OSError subclasses), so a failed login only leaves the client on per-request Basic Auth, and create_circle / create_collective still return the created circle or collective. A failed folder lookup is caught as well and reported in team_folder_note.
• The check being separate from the deletion: Circles has no conditional delete, so a folder linked to the team between the check and the delete would still go. The window is one request long, and the realistic case (a team that already has a folder when the agent decides to delete it) is what the guard covers.
• Docstring coverage: the functions without docstrings are tests, which this repo does not document one by one.