Add App Management tools and user-permission tests#32
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an App Management MCP module (list/get/enable/disable apps), registers those tools in the server, introduces integration tests for app management and user-permissions, adds a CI job for user-permissions tests, and adjusts Codecov and lint workflow settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Server as MCP Server
participant Auth as Permission Check
participant OCS as Nextcloud OCS API
Client->>Server: list_apps(filter="enabled")
Server->>Auth: verify PermissionLevel.READ
Auth-->>Server: allowed
Server->>OCS: GET /ocs/v2.php/apps?filter=enabled
OCS-->>Server: apps JSON
Server-->>Client: formatted apps JSON
Client->>Server: enable_app(app_id)
Server->>Auth: verify PermissionLevel.WRITE
Auth-->>Server: allowed
Server->>OCS: POST /ocs/v2.php/apps/{app_id}
OCS-->>Server: 200 OK
Server-->>Client: confirmation string
Client->>Server: disable_app(app_id)
Server->>Auth: verify PermissionLevel.DESTRUCTIVE
Auth-->>Server: denied (non-admin)
Server-->>Client: ToolError (403 / Forbidden)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #32 +/- ##
==========================================
+ Coverage 95.80% 95.92% +0.12%
==========================================
Files 22 23 +1
Lines 1501 1546 +45
==========================================
+ Hits 1438 1483 +45
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: 1
🧹 Nitpick comments (1)
tests/integration/test_user_permissions.py (1)
28-41: Consider narrowing exception handling in fixture.The
except NextcloudErrorblock catches all Nextcloud errors, not just "user not found" (404). If the API call fails for other reasons (e.g., 500 server error), the fixture will still attempt to create the user. This is acceptable for CI robustness but could mask transient setup failures.Optional: More explicit 404 handling
`@pytest.fixture`(scope="module") async def _ensure_test_user() -> None: """Create the test user via admin client if it doesn't exist.""" admin_config = Config( nextcloud_url=os.environ.get("NEXTCLOUD_URL", "http://nextcloud.ncmcp"), user=os.environ.get("NEXTCLOUD_USER", "admin"), password=os.environ.get("NEXTCLOUD_PASSWORD", "admin"), ) client = NextcloudClient(admin_config) try: await client.ocs_get(f"cloud/users/{TEST_USER}") except NextcloudError: - await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS}) + # User doesn't exist (or API error) - try to create + try: + await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS}) + except NextcloudError: + pass # User may already exist from previous run await client.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_user_permissions.py` around lines 28 - 41, The fixture _ensure_test_user currently catches any NextcloudError from client.ocs_get and proceeds to create the user; change this to only handle "user not found" (HTTP 404) errors: in the except block inspect the NextcloudError (or its underlying response/status attribute) returned by NextcloudClient.ocs_get and if the status indicates 404 then call client.ocs_post to create the user, otherwise re-raise the exception so real server errors (e.g., 500) are not masked. Ensure you reference _ensure_test_user, NextcloudClient, ocs_get, ocs_post, and NextcloudError when making the conditional handling change.
🤖 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_app_management.py`:
- Around line 88-94: In test_disable_returns_confirmation, capture the return
value from nc_mcp.call("disable_app", app_id=SAFE_APP) instead of discarding it,
then assert the response contains the expected confirmation (e.g., check it's
truthy and includes the word "disabled" or equals your project's expected
confirmation structure/constant). Keep the finally block that calls
nc_mcp.call("enable_app", app_id=SAFE_APP) to restore state; update the test to
use the captured result variable and add an assertion against that variable.
---
Nitpick comments:
In `@tests/integration/test_user_permissions.py`:
- Around line 28-41: The fixture _ensure_test_user currently catches any
NextcloudError from client.ocs_get and proceeds to create the user; change this
to only handle "user not found" (HTTP 404) errors: in the except block inspect
the NextcloudError (or its underlying response/status attribute) returned by
NextcloudClient.ocs_get and if the status indicates 404 then call
client.ocs_post to create the user, otherwise re-raise the exception so real
server errors (e.g., 500) are not masked. Ensure you reference
_ensure_test_user, NextcloudClient, ocs_get, ocs_post, and NextcloudError when
making the conditional handling change.
🪄 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: ef7881fe-8ac5-4d19-9ee9-f7450e9ba9bd
📒 Files selected for processing (7)
.github/workflows/tests-integration.ymlcodecov.ymlsrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/app_management.pytests/integration/test_app_management.pytests/integration/test_server.pytests/integration/test_user_permissions.py
- Remove enabled/type from _format_app (getAppInfo endpoint doesn't return active/type fields, so they were always null) - Use ADDITIVE_IDEMPOTENT annotation for enable_app (idempotent op) - Fix test_disable_returns_confirmation to actually assert the message - Tighten user-permission tests: ToolError instead of bare Exception, regex matches actual Nextcloud error messages - Update PROGRESS.md with app management and collectives entries
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_user_permissions.py`:
- Around line 39-42: The current bare except NextcloudError around
client.ocs_get(...) swallows all HTTP errors; change it to catch NextcloudError
as e and only handle the "user not found" case (HTTP 404) — e.g., if
e.status_code == 404 (or inspect e.response.status_code) then call
client.ocs_post("cloud/users", data={"userid": TEST_USER, "password":
TEST_PASS}); otherwise re-raise the exception so
authentication/permission/server errors surface; reference the symbols
client.ocs_get, client.ocs_post, NextcloudError, TEST_USER and TEST_PASS when
making the change.
🪄 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: 0e7458d2-8d0a-4a9d-a26f-d38298737e66
📒 Files selected for processing (4)
PROGRESS.mdsrc/nc_mcp_server/tools/app_management.pytests/integration/test_app_management.pytests/integration/test_user_permissions.py
✅ Files skipped from review due to trivial changes (1)
- PROGRESS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_app_management.py
Comment on lines
+39
to
+42
| try: | ||
| await client.ocs_get(f"cloud/users/{TEST_USER}") | ||
| except NextcloudError: | ||
| await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS}) |
There was a problem hiding this comment.
Narrow the exception handling to only catch "user not found" (404).
The bare except NextcloudError catches all non-2xx responses including authentication failures (401), permission denied (403), and server errors (500). If admin credentials are misconfigured, this will silently proceed to create_user which will also fail with a confusing error.
🛡️ Proposed fix to check status code
try:
await client.ocs_get(f"cloud/users/{TEST_USER}")
- except NextcloudError:
- await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS})
+ except NextcloudError as e:
+ if e.status_code == 404:
+ await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS})
+ else:
+ raise # Re-raise auth/permission/server errors
await client.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_user_permissions.py` around lines 39 - 42, The current
bare except NextcloudError around client.ocs_get(...) swallows all HTTP errors;
change it to catch NextcloudError as e and only handle the "user not found" case
(HTTP 404) — e.g., if e.status_code == 404 (or inspect e.response.status_code)
then call client.ocs_post("cloud/users", data={"userid": TEST_USER, "password":
TEST_PASS}); otherwise re-raise the exception so
authentication/permission/server errors surface; reference the symbols
client.ocs_get, client.ocs_post, NextcloudError, TEST_USER and TEST_PASS when
making the change.
This was referenced Mar 30, 2026
This was referenced Apr 7, 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
App Management tools (4 tools):
list_appsget_app_infoenable_appdisable_appUser-permission integration tests (new parallel CI job):
CI changes:
test-user-permissionsjob (parallel, NC 32 + NC 33)after_n_buildsupdated to 9Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores / CI