Add Calendar tools with CalDAV support#33
Conversation
Tools: list_calendars, get_events, get_event, create_event, update_event, delete_event. Uses icalendar v7.x for iCal parsing/generation. CalDAV REPORT queries for reliable event lookup by UID and time-range filtering. ETag-based concurrency on updates. Supports timed events, all-day events, and partial field updates.
|
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 12 minutes and 38 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 (2)
📝 WalkthroughWalkthroughAdds a CalDAV-backed Calendar tool suite (six MCP tools), registers it with the MCP server, introduces a runtime dependency and Pyright config change, updates project progress documentation, and adds comprehensive integration tests for calendar discovery, CRUD, permissions, and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server as "MCP Server\n(calendar tool)"
participant Nextcloud as "Nextcloud CalDAV\n(server)"
Client->>MCP_Server: create_event(calendar_id, event data)
MCP_Server->>Nextcloud: PROPFIND (discover calendar URL)
Nextcloud-->>MCP_Server: calendar URL + properties
MCP_Server->>Nextcloud: PUT (upload .ics with If-Match/headers)
Nextcloud-->>MCP_Server: 201 Created + ETag
MCP_Server-->>Client: success (event UID, etag)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 #33 +/- ##
==========================================
- Coverage 95.92% 95.88% -0.04%
==========================================
Files 23 24 +1
Lines 1546 1848 +302
==========================================
+ Hits 1483 1772 +289
- Misses 63 76 +13
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PROGRESS.md`:
- Around line 36-45: The phase tracker is inconsistent: update the header
"Current Phase: 2 — Communication" and the phase table rows (where Phase 3 is
marked "Not Started" and lists Calendar in scope) to reflect that Calendar tools
are completed and the project has advanced to Phase 3 — Groupware; specifically,
change the Current Phase text to "Phase 3 — Groupware", mark Phase 3 as "In
Progress" or "Active" in the table, remove Calendar from Phase 3 scope (or mark
it completed), and ensure the checklist line for "Calendar tools:
list_calendars, get_events, get_event, create_event, update_event, delete_event
(2026-03-30)" and any totals remain consistent with those changes.
In `@src/nc_mcp_server/tools/calendar.py`:
- Around line 213-220: _parsе_dt currently treats any 10-char string as a date
which causes mixed DATE/DATE-TIME values and timezone errors; change it to
decide based on explicit all_day or the presence of a time component (e.g., look
for 'T' or try datetime.fromisoformat first) so datetime strings are parsed as
timezone-aware datetimes (attach UTC if tzinfo is None) and date-only strings
become date objects only when explicitly all_day or genuinely date-only; then
update get_events (and the other occurrence around the block that previously
sliced original strings at lines ~387-394) to build the REPORT time-range using
the parsed UTC datetime values returned by _parse_dt instead of slicing the raw
input strings so both bounds are normalized to UTC and types are consistent.
- Around line 499-546: update_event currently retrieves the latest ETag via
_find_event then uses it in the PUT If-Match header, which prevents callers from
getting conflicts; change update_event to accept a caller-provided ETag (e.g.,
add parameter like client_etag or if_match: str | None) and, when present,
forward that value in the dav_request If-Match header instead of the etag
returned by _find_event; keep existing behavior when the new parameter is None
(use fetched etag), and update callers to pass the ETag they received from
get_event; references: update_event, _find_event, get_client, _href_to_dav_path,
and client.dav_request.
- Line 5: Replace the unsafe xml.etree.ElementTree import with defusedxml by
changing the top-level import (currently "import xml.etree.ElementTree as ET")
to use defusedxml.ElementTree and add defusedxml to project dependencies; then
remove the "# noqa: S314" suppressions around the ET.fromstring() calls (the
usages at the locations that correspond to the original lines where
ET.fromstring is called) so the code parses untrusted responses with the secure
defusedxml parser instead of suppressing the Bandit warning.
🪄 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: 63d93e8f-1785-48e1-8179-77851ded9de2
📒 Files selected for processing (6)
PROGRESS.mdpyproject.tomlsrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/calendar.pytests/integration/test_calendar.pytests/integration/test_server.py
| - [x] Calendar tools: list_calendars, get_events, get_event, create_event, update_event, delete_event (2026-03-30) | ||
|
|
||
| ### In Progress | ||
|
|
||
| ### Blocked | ||
| (none) | ||
|
|
||
| ### Next Up | ||
| - Phase 3 — Groupware (Calendar, Contacts, Tasks, Deck, Notes) | ||
| - Phase 3 — Groupware (Contacts, Tasks, Deck, Notes) | ||
|
|
There was a problem hiding this comment.
Keep the phase tracker internally consistent.
Calendar is marked complete and the totals are updated here, but the file still says Current Phase: 2 — Communication and the phase table leaves phase 3 as Not Started with Calendar in scope. Please sync those sections so contributors aren't reading conflicting status.
Also applies to: 75-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PROGRESS.md` around lines 36 - 45, The phase tracker is inconsistent: update
the header "Current Phase: 2 — Communication" and the phase table rows (where
Phase 3 is marked "Not Started" and lists Calendar in scope) to reflect that
Calendar tools are completed and the project has advanced to Phase 3 —
Groupware; specifically, change the Current Phase text to "Phase 3 — Groupware",
mark Phase 3 as "In Progress" or "Active" in the table, remove Calendar from
Phase 3 scope (or mark it completed), and ensure the checklist line for
"Calendar tools: list_calendars, get_events, get_event, create_event,
update_event, delete_event (2026-03-30)" and any totals remain consistent with
those changes.
|
|
||
| import json | ||
| import uuid | ||
| import xml.etree.ElementTree as ET |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What do Python's XML security docs and Bandit S314 recommend for parsing untrusted XML, and is defusedxml.ElementTree the standard mitigation?
💡 Result:
Python's official XML security documentation warns that the standard library XML modules, including xml.etree.ElementTree, are not secure against maliciously constructed data for untrusted input. It explicitly recommends using the defusedxml package, which provides modified subclasses of stdlib XML parsers that prevent malicious operations like XXE, billion laughs, and quadratic blowup attacks. The docs state: "Use of this package is recommended for any server code that parses untrusted XML data." Bandit S314 (also referenced as B314 in Bandit's blacklist_calls plugin) flags insecure use of xml.etree.ElementTree methods like parse, iterparse, fromstring, and XMLParser when parsing untrusted XML. It recommends replacing them with the defusedxml equivalents (e.g., from defusedxml.ElementTree import parse) or calling defusedxml.defuse_stdlib. The message is: "Using xml.etree.ElementTree.parse to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.parse with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib is called." Severity: Medium, Confidence: High. Yes, defusedxml.ElementTree is the standard mitigation recommended by both Python docs and Bandit for securely parsing untrusted XML with ElementTree. Note that while modern Python (with Expat >=2.4.1) mitigates some DoS attacks, the docs still advise defusedxml for comprehensive protection against all known vectors, especially entity-related issues.
Citations:
- 1: https://docs.python.org/3.11/library/xml.html
- 2: https://docs.python.org/3.15/library/xml.etree.elementtree.html
- 3: https://docs.python.org/3.12/library/xml.etree.elementtree.html
- 4: https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html
- 5: https://docs.astral.sh/ruff/rules/suspicious-xml-element-tree-usage/
🏁 Script executed:
cat -n src/nc_mcp_server/tools/calendar.py | head -20Repository: cloud-py-api/nc_mcp_server
Length of output: 909
🏁 Script executed:
wc -l src/nc_mcp_server/tools/calendar.pyRepository: cloud-py-api/nc_mcp_server
Length of output: 109
🏁 Script executed:
sed -n '5p;100,115p;135,150p' src/nc_mcp_server/tools/calendar.pyRepository: cloud-py-api/nc_mcp_server
Length of output: 1283
🏁 Script executed:
find . -name "*.txt" -o -name "*.toml" -o -name "*.cfg" | head -20Repository: cloud-py-api/nc_mcp_server
Length of output: 86
🏁 Script executed:
rg -l "defusedxml" .Repository: cloud-py-api/nc_mcp_server
Length of output: 52
🏁 Script executed:
rg "defusedxml" . --type pyRepository: cloud-py-api/nc_mcp_server
Length of output: 52
🏁 Script executed:
cat pyproject.tomlRepository: cloud-py-api/nc_mcp_server
Length of output: 2505
Replace xml.etree.ElementTree with defusedxml.ElementTree and remove S314 suppressions.
The code parses untrusted server responses with ET.fromstring() at lines 109 and 140, which is exactly the network-input case Bandit S314 warns against. Python's official XML security documentation and Bandit both recommend using defusedxml.ElementTree to mitigate XXE and other XML attacks. Add defusedxml to dependencies and swap the import rather than suppressing the warning.
Suggested fix
-import xml.etree.ElementTree as ET
+from defusedxml import ElementTree as ETRemove the # noqa: S314 suppressions at lines 109 and 140.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nc_mcp_server/tools/calendar.py` at line 5, Replace the unsafe
xml.etree.ElementTree import with defusedxml by changing the top-level import
(currently "import xml.etree.ElementTree as ET") to use defusedxml.ElementTree
and add defusedxml to project dependencies; then remove the "# noqa: S314"
suppressions around the ET.fromstring() calls (the usages at the locations that
correspond to the original lines where ET.fromstring is called) so the code
parses untrusted responses with the secure defusedxml parser instead of
suppressing the Bandit warning.
| def _parse_dt(value: str, all_day: bool = False) -> date | datetime: | ||
| """Parse an ISO date or datetime string.""" | ||
| if all_day or len(value) == 10: | ||
| return date.fromisoformat(value) | ||
| dt = datetime.fromisoformat(value) | ||
| if dt.tzinfo is None: | ||
| dt = dt.replace(tzinfo=UTC) | ||
| return dt |
There was a problem hiding this comment.
Stop inferring DATE values from string length.
_parse_dt returns a date for any 10-character input even when all_day is false, and get_events separately rewrites range bounds by slicing strings. That means a timed update can end up with DATE DTSTART plus DATE-TIME DTEND, and offset-aware filters are shifted instead of normalized to UTC.
Suggested direction
def _parse_dt(value: str, all_day: bool = False) -> date | datetime:
"""Parse an ISO date or datetime string."""
- if all_day or len(value) == 10:
+ if all_day:
return date.fromisoformat(value)
- dt = datetime.fromisoformat(value)
+ dt = datetime.fromisoformat(value.replace("Z", "+00:00"))
if dt.tzinfo is None:
dt = dt.replace(tzinfo=UTC)
- return dt
+ else:
+ dt = dt.astimezone(UTC)
+ return dtUse the same parsed UTC value to build the REPORT time-range instead of stripping characters from the original string.
Also applies to: 387-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nc_mcp_server/tools/calendar.py` around lines 213 - 220, _parsе_dt
currently treats any 10-char string as a date which causes mixed DATE/DATE-TIME
values and timezone errors; change it to decide based on explicit all_day or the
presence of a time component (e.g., look for 'T' or try datetime.fromisoformat
first) so datetime strings are parsed as timezone-aware datetimes (attach UTC if
tzinfo is None) and date-only strings become date objects only when explicitly
all_day or genuinely date-only; then update get_events (and the other occurrence
around the block that previously sliced original strings at lines ~387-394) to
build the REPORT time-range using the parsed UTC datetime values returned by
_parse_dt instead of slicing the raw input strings so both bounds are normalized
to UTC and types are consistent.
| async def update_event( | ||
| calendar_id: str, | ||
| event_uid: str, | ||
| summary: str | None = None, | ||
| start: str | None = None, | ||
| end: str | None = None, | ||
| description: str | None = None, | ||
| location: str | None = None, | ||
| status: str | None = None, | ||
| categories: str | None = None, | ||
| ) -> str: | ||
| """Update an existing calendar event. Only provided fields are changed. | ||
|
|
||
| Uses the event's ETag for safe concurrent updates — if the event was | ||
| modified since it was last read, the update will fail with a conflict error. | ||
|
|
||
| Args: | ||
| calendar_id: Calendar identifier (e.g. "personal"). | ||
| event_uid: The event's UID to update. Use get_events to find UIDs. | ||
| summary: New event title. | ||
| start: New start date/datetime in ISO 8601 format. | ||
| end: New end date/datetime in ISO 8601 format. | ||
| description: New description. Pass "" to clear. | ||
| location: New location. Pass "" to clear. | ||
| status: New status: "CONFIRMED", "TENTATIVE", or "CANCELLED". | ||
| categories: New categories as comma-separated string. Pass "" to clear. | ||
|
|
||
| Returns: | ||
| Confirmation message with the updated event UID. | ||
| """ | ||
| validated_status = _validate_status(status) if status is not None else None | ||
| cat_list: list[str] | None = None | ||
| if categories is not None: | ||
| cat_list = [c.strip() for c in categories.split(",") if c.strip()] if categories else [] | ||
| href, etag, ical_data = await _find_event(calendar_id, event_uid) | ||
| cal = ICal.from_ical(ical_data) | ||
| for component in cal.walk(): | ||
| if component.name == "VEVENT": | ||
| _apply_event_updates(component, summary, start, end, description, location, validated_status, cat_list) | ||
| break | ||
|
|
||
| client = get_client() | ||
| await client.dav_request( | ||
| "PUT", | ||
| _href_to_dav_path(href), | ||
| body=cal.to_ical().decode(), | ||
| headers={"Content-Type": "text/calendar; charset=utf-8", "If-Match": f'"{etag}"'}, | ||
| context=f"Update event '{event_uid}'", |
There was a problem hiding this comment.
This If-Match flow still allows stale client overwrites.
update_event fetches the latest ETag itself and then echoes that back in the PUT, so a caller working from an older get_event result never gets a conflict for intervening edits. If this API is meant to provide optimistic concurrency, accept the caller's ETag and forward that in If-Match instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nc_mcp_server/tools/calendar.py` around lines 499 - 546, update_event
currently retrieves the latest ETag via _find_event then uses it in the PUT
If-Match header, which prevents callers from getting conflicts; change
update_event to accept a caller-provided ETag (e.g., add parameter like
client_etag or if_match: str | None) and, when present, forward that value in
the dav_request If-Match header instead of the etag returned by _find_event;
keep existing behavior when the new parameter is None (use fetched etag), and
update callers to pass the ETag they received from get_event; references:
update_event, _find_event, get_client, _href_to_dav_path, and
client.dav_request.
44eba67 to
5a93e65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/test_user_permissions.py (1)
47-57: Retry budget is likely too high for CI startup failures.With the session timeout set to 30s in
src/nc_mcp_server/client.py(Line 119-141), this loop can take ~5+ minutes in worst case (10 request timeouts + sleeps). Consider a bounded max elapsed time (or fewer attempts) to reduce CI stall time while keeping resilience.🤖 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 47 - 57, The retry loop that calls client.ocs_get / client.ocs_post for TEST_USER/TEST_PASS can take ~5+ minutes with 10 attempts and 3s sleeps given the 30s session timeout; change it to use a bounded elapsed-time approach (or reduce attempts) by tracking start = time.monotonic() and breaking when time.monotonic() - start exceeds a shorter max (e.g. ~60–90s) and/or lower the attempt count (e.g. 3 attempts) so that NextcloudError, OSError, TimeoutError, MaxRetryError retries stop earlier; update the loop around client.ocs_get / client.ocs_post and the exception handling to respect this elapsed-time cap.
🤖 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 46-60: The retry loop around client.ocs_get/ocs_post fails to
catch network errors raised by the create call and may leak the admin client on
error; change the loop so that the client.ocs_post(...) call is wrapped in its
own try/except that catches OSError, TimeoutError, MaxRetryError (and handles
retry semantics by setting last_err and continuing as done for the get), and
ensure await client.close() is executed in a finally block (not after the loop)
so the session is always closed even on exceptions; reference client.ocs_get,
client.ocs_post, NextcloudError, the last_err variable and the await
client.close() call when making these changes.
---
Nitpick comments:
In `@tests/integration/test_user_permissions.py`:
- Around line 47-57: The retry loop that calls client.ocs_get / client.ocs_post
for TEST_USER/TEST_PASS can take ~5+ minutes with 10 attempts and 3s sleeps
given the 30s session timeout; change it to use a bounded elapsed-time approach
(or reduce attempts) by tracking start = time.monotonic() and breaking when
time.monotonic() - start exceeds a shorter max (e.g. ~60–90s) and/or lower the
attempt count (e.g. 3 attempts) so that NextcloudError, OSError, TimeoutError,
MaxRetryError retries stop earlier; update the loop around client.ocs_get /
client.ocs_post and the exception handling to respect this elapsed-time cap.
🪄 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: 801f3635-a1da-4d71-9327-83c10137866e
📒 Files selected for processing (1)
tests/integration/test_user_permissions.py
| last_err: Exception | None = None | ||
| for attempt in range(10): | ||
| try: | ||
| await client.ocs_get(f"cloud/users/{TEST_USER}") | ||
| break | ||
| except NextcloudError: | ||
| await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS}) | ||
| break | ||
| except (OSError, TimeoutError, MaxRetryError) as exc: | ||
| last_err = exc | ||
| if attempt < 9: | ||
| await asyncio.sleep(3) | ||
| continue | ||
| raise last_err from exc | ||
| await client.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file to confirm the structure
cat -n tests/integration/test_user_permissions.py | head -65Repository: cloud-py-api/nc_mcp_server
Length of output: 2834
🏁 Script executed:
# Check imports at the top of the file
head -20 tests/integration/test_user_permissions.pyRepository: cloud-py-api/nc_mcp_server
Length of output: 760
🏁 Script executed:
# Let's verify the imports mentioned in the review
grep -n "import asyncio\|from urllib3\|from urllib3.exceptions" tests/integration/test_user_permissions.pyRepository: cloud-py-api/nc_mcp_server
Length of output: 134
🏁 Script executed:
python3 << 'EOF'
# Test: Does an exception raised inside an except block get caught by a sibling except clause?
print("Test 1: Exception in except block with sibling except clause")
try:
try:
raise ValueError("original")
except ValueError:
raise RuntimeError("from except block")
except OSError:
print("OSError caught (should NOT happen)")
except RuntimeError as e:
print(f"RuntimeError caught OUTSIDE: {e}")
print("\nTest 2: Simulating the actual code structure")
last_err = None
for attempt in range(3):
try:
if attempt == 0:
raise RuntimeError("first attempt") # simulates error condition
if attempt == 1:
raise ValueError("get succeeded") # NextcloudError equivalent
break
except ValueError:
# This is the except NextcloudError block
raise OSError("from post call") # Simulates ocs_post failure
except (OSError, TimeoutError) as exc:
print(f"Caught {type(exc).__name__}: {exc}")
last_err = exc
continue
except Exception as exc:
print(f"Caught outer Exception: {type(exc).__name__}: {exc}")
print(f"last_err after loop: {last_err}")
EOFRepository: cloud-py-api/nc_mcp_server
Length of output: 560
🏁 Script executed:
# Let's also verify the structure of the client.close() issue
python3 << 'EOF'
print("Test 3: Resource cleanup without finally block")
import asyncio
async def test_cleanup():
class FakeClient:
async def close(self):
print("Client closed")
client = FakeClient()
try:
for attempt in range(2):
try:
if attempt == 0:
raise RuntimeError("error in loop")
break
except RuntimeError:
print("Caught RuntimeError, doing cleanup implicitly")
break
# close() is HERE, outside try block
await client.close()
except Exception as e:
print(f"Exception escaped: {e}")
print("close() will NOT be called")
async def test_with_finally():
class FakeClient:
async def close(self):
print("Client closed (with finally)")
client = FakeClient()
try:
for attempt in range(2):
try:
if attempt == 0:
raise RuntimeError("error in loop")
break
except RuntimeError:
print("Caught RuntimeError")
raise RuntimeError("re-raised") from None
finally:
await client.close()
print("Without finally:")
try:
asyncio.run(test_cleanup())
except:
pass
print("\nWith finally:")
try:
asyncio.run(test_with_finally())
except:
pass
EOFRepository: cloud-py-api/nc_mcp_server
Length of output: 257
🏁 Script executed:
# Verify that this is indeed a module-scoped fixture used by other tests
grep -n "def.*_ensure_test_user\|_ensure_test_user" tests/integration/test_user_permissions.py | head -10Repository: cloud-py-api/nc_mcp_server
Length of output: 192
🏁 Script executed:
# Check how many test functions depend on this fixture
grep -c "@pytest.fixture" tests/integration/test_user_permissions.py
grep -n "def test_\|_ensure_test_user" tests/integration/test_user_permissions.py | head -20Repository: cloud-py-api/nc_mcp_server
Length of output: 1418
🏁 Script executed:
# Verify the fixture scope in the code
grep -A 5 "@pytest.fixture" tests/integration/test_user_permissions.py | head -15Repository: cloud-py-api/nc_mcp_server
Length of output: 665
Retry logic misses create-call failures and can leak the admin client on error.
At line 52, client.ocs_post(...) runs inside except NextcloudError, so OSError, TimeoutError, or MaxRetryError exceptions from that call escape the sibling except handler at line 54 and propagate unhandled. Also, await client.close() at line 60 is not in a finally block, so failures leave the session unclosed. Since this is a module-scoped fixture used by 14+ tests, any unhandled exception prevents the entire test suite in this file from running.
Wrap the ocs_post call in its own try/except, and move client.close() to a finally block to ensure cleanup.
Proposed fix
async def _ensure_test_user() -> None:
- client = NextcloudClient(admin_config)
- last_err: Exception | None = None
- for attempt in range(10):
- try:
- await client.ocs_get(f"cloud/users/{TEST_USER}")
- break
- except NextcloudError:
- await client.ocs_post("cloud/users", data={"userid": TEST_USER, "password": TEST_PASS})
- break
- except (OSError, TimeoutError, MaxRetryError) as exc:
- last_err = exc
- if attempt < 9:
- await asyncio.sleep(3)
- continue
- raise last_err from exc
- await client.close()
+ client = NextcloudClient(admin_config)
+ try:
+ last_err: Exception | None = None
+ for attempt in range(10):
+ try:
+ await client.ocs_get(f"cloud/users/{TEST_USER}")
+ break
+ except NextcloudError:
+ try:
+ await client.ocs_post(
+ "cloud/users",
+ data={"userid": TEST_USER, "password": TEST_PASS},
+ )
+ break
+ except (OSError, TimeoutError, MaxRetryError) as exc:
+ last_err = exc
+ except (OSError, TimeoutError, MaxRetryError) as exc:
+ last_err = exc
+
+ if attempt < 9:
+ await asyncio.sleep(3)
+ continue
+ raise last_err if last_err is not None else RuntimeError("Failed to ensure test user")
+ finally:
+ 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 46 - 60, The retry
loop around client.ocs_get/ocs_post fails to catch network errors raised by the
create call and may leak the admin client on error; change the loop so that the
client.ocs_post(...) call is wrapped in its own try/except that catches OSError,
TimeoutError, MaxRetryError (and handles retry semantics by setting last_err and
continuing as done for the get), and ensure await client.close() is executed in
a finally block (not after the loop) so the session is always closed even on
exceptions; reference client.ocs_get, client.ocs_post, NextcloudError, the
last_err variable and the await client.close() call when making these changes.
5a93e65 to
b48c8d2
Compare
b48c8d2 to
dd8f775
Compare
2 participants
Summary
Calendar tools (6 tools):
list_calendarsget_eventsget_eventcreate_eventupdate_eventdelete_eventImplementation details:
icalendar>=7for iCalendar parsing/generationupdate_eventusesIf-MatchETag header for safe concurrent modificationTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores