feat(webdav): expose file tags - #1393
Conversation
The WebDAV client already covered system tags -- create_tag, get_or_create_tag, assign_tag_to_file, remove_tag_from_file, get_tag_by_name, get_files_by_tag, find_by_tag -- but not one tool exposed any of it, so tagging was unreachable through the server. Adds the two missing read paths on the client (list_tags, get_file_tags) and five tools: nc_webdav_list_tags every tag on the instance nc_webdav_get_file_tags tags of one file nc_webdav_find_by_tag_name files carrying a tag nc_webdav_tag_file attach a tag, creating it if needed nc_webdav_untag_file detach a tag, leaving the tag itself They take tag *names* rather than ids: an id is unguessable for a language model, a name is not. get_file_tags additionally fetches the full list because Nextcloud only reports ids on the file itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GomNRo5jTc6rDNNLYrRVRX
|
|
There was a problem hiding this comment.
🟡 Changes recommended
The new tools bypass existing excluded-path guards in several places and also introduce untyped dict tool responses and missing end-to-end coverage for new MCP API surface.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Exposes Nextcloud WebDAV “system tags” functionality through new MCP tools so agents can list tags, inspect file tags, find files by tag, and tag/untag files—building on existing client capabilities.
Changes:
- Adds new MCP tools in
server/webdav.pyto list tags, get a file’s tags, find files by tag name, and tag/untag files. - Extends the WebDAV client with missing read paths: listing all tags and listing tags for a specific file.
- Adds unit tests covering multistatus parsing for system tag listing.
File summaries
| File | Description |
|---|---|
nextcloud_mcp_server/server/webdav.py |
Adds MCP tool endpoints for tag operations (list/get/find/tag/untag). |
nextcloud_mcp_server/client/webdav.py |
Implements list_tags() and get_file_tags() plus shared multistatus parsing helper. |
tests/unit/client/test_webdav_tags.py |
Adds unit tests for tag multistatus parsing and PROPFIND body well-formedness. |
Review details
Suppressed comments (3)
nextcloud_mcp_server/server/webdav.py:1205
- nc_webdav_find_by_tag_name returns file paths without filtering out EXCLUDED_TAGS, unlike the other WebDAV search/list tools in this module. This can expose excluded paths in results; filter the list with is_path_excluded before returning.
client = await get_client(ctx)
found = await client.webdav.get_tag_by_name(tag)
if found is None or found.get("id") is None:
return {
"tag": tag,
nextcloud_mcp_server/server/webdav.py:1256
- Untagging a file should also use _resolve_commented_file() for consistent EXCLUDED_TAGS enforcement and ToolError messaging. As written, it bypasses the guard and converts file_id late.
client = await get_client(ctx)
file_id = await client.webdav.get_fileid(path)
if not file_id:
raise ValueError(f"No file id for path: {path}")
found = await client.webdav.get_tag_by_name(tag)
nextcloud_mcp_server/server/webdav.py:1242
- Untagging the same file/tag repeatedly should also be annotated as idempotent for clients (same end state after the first call).
@mcp.tool(
title="Untag File",
annotations=ToolAnnotations(readOnlyHint=False, openWorldHint=True),
)
- Files reviewed: 3/3 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| async def nc_webdav_list_tags(ctx: Context) -> dict: | ||
| """List all system tags available for tagging files.""" | ||
| client = await get_client(ctx) | ||
| tags = await client.webdav.list_tags() | ||
| return {"tags": tags, "total_count": len(tags)} |
| @mcp.tool( | ||
| title="List Tags", | ||
| annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), | ||
| ) | ||
| @require_scopes("files.read") |
| client = await get_client(ctx) | ||
| return await client.webdav.get_file_tags(path) |
| file_id = await client.webdav.get_fileid(path) | ||
| if not file_id: | ||
| raise ValueError(f"No file id for path: {path}") | ||
| resolved = await client.webdav.get_or_create_tag(tag) | ||
| await client.webdav.assign_tag_to_file(int(file_id), resolved["id"]) |
| @mcp.tool( | ||
| title="Tag File", | ||
| annotations=ToolAnnotations(readOnlyHint=False, openWorldHint=True), | ||
| ) |
| name_elem = response_elem.find(".//{http://owncloud.org/ns}display-name") | ||
| id_elem = response_elem.find(".//{http://owncloud.org/ns}id") | ||
| if name_elem is None or id_elem is None or not id_elem.text: | ||
| continue | ||
| assignable = response_elem.find( | ||
| ".//{http://owncloud.org/ns}user-assignable" | ||
| ) | ||
| tags.append( | ||
| { | ||
| "id": int(id_elem.text), | ||
| "name": name_elem.text or "", | ||
| "assignable": (assignable is None or assignable.text != "false"), | ||
| } | ||
| ) |
| "path": path, | ||
| "file_id": file_id, | ||
| "tags": [ | ||
| {"id": i, "name": names.get(i, f"(unbekannt: {i})")} |
| def test_entry_without_id_is_dropped(self): | ||
| broken = ( | ||
| "<d:response><d:href>/remote.php/dav/systemtags/9</d:href>" | ||
| "<d:propstat><d:prop>" | ||
| "<oc:display-name>Nameless</oc:display-name>" | ||
| "</d:prop></d:propstat></d:response>" | ||
| ) | ||
| assert _parse(_multistatus(broken)) == [] |
Review feedback: - Return BaseResponse models (ListTagsResponse, FileTagsResponse, FilesByTagResponse, TagFileResponse) with SystemTag as the item model, instead of raw dicts. - Route path resolution through the shared helper so EXCLUDED_TAGS is enforced and a missing or non-numeric fileid becomes a ToolError rather than a ValueError. The helper is renamed _resolve_file_id, since it is no longer specific to comments. find_by_tag additionally filters excluded paths out of its results -- a tag can be attached to an excluded file. - Annotate the mutating tools with destructiveHint/idempotentHint per ADR-017. Tagging and untagging are both idempotent. - Drop tags whose display name is empty or whitespace: the check only verified the element existed, so such a tag was surfaced with an empty name and an ambiguous sort key, contradicting the docstring. - Replace the German placeholder for an unmapped tag id with English. Adds tests/server/test_webdav_tags_mcp.py for the tag lifecycle and the refusal cases, plus unit tests for the empty-name handling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GomNRo5jTc6rDNNLYrRVRX
|
All seven points addressed. Two of them were plain mistakes on my part — thank you for catching them. Response models. EXCLUDED_TAGS. You were right on both counts. Path resolution now goes through the shared helper, so the guard applies and a missing or non-numeric fileid becomes a Tool annotations. Switched to Empty display names. My docstring claimed entries without a name are dropped, but the check only verified the element existed — an empty German placeholder. Embarrassing leak, fixed — Integration coverage. Added |
There was a problem hiding this comment.
🟡 Changes recommended
The new response models currently risk returning incorrect data (e.g., SystemTag.assignable defaulting to True when unknown), which should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
| assignable: bool = Field( | ||
| default=True, description="Whether users may assign this tag" | ||
| ) |
| """Tags assigned to one file.""" | ||
|
|
||
| path: str = Field(description="Path the tags belong to") | ||
| file_id: str = Field(description="Nextcloud file id") |
| client = await get_client(ctx) | ||
| # Resolving through the shared helper applies the excluded-tag guard, | ||
| # so an excluded path cannot be probed for existence via its tags. | ||
| await _resolve_file_id(client, path) | ||
|
|
||
| data = await client.webdav.get_file_tags(path) |
|
Hi @IchbinkeinReh can you please submit a response to the CLA bot? Otherwise the changes LGTM |
Done |
CI caught it: this tool returns FilesByTagResponse.files (FileInfo entries), which is a registered linkable model, but the tool never applied the decorator that fills in each entry's url field -- so url was always None, same class of bug test_links_tool_coverage.py exists to catch (see its docstring re: the Deck card tools). Added a direct test alongside the existing DirectoryListing one, since the coverage test only proves the decorator is present, not that it actually populates anything for this tool's specific response shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GomNRo5jTc6rDNNLYrRVRX
|
There was a problem hiding this comment.
🔵 Needs a closer look
The current implementation can surface incorrect tag metadata (e.g., placeholder “unknown tag” names / lost assignable state) and does redundant file-id lookups that should be addressed before approval.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
nextcloud_mcp_server/server/webdav.py:1171
- The
nc_webdav_list_tagsdocstring says "available for tagging files", but the tool returns all system tags (including those withassignable=False). This can mislead callers about what they can actually apply; either filter to assignable tags or clarify the wording.
This issue also appears on line 1196 of the same file.
"""List all system tags available for tagging files."""
nextcloud_mcp_server/client/webdav.py:2787
get_file_tags()fabricates placeholder names like "(unknown tag 123)" when a relation references an id not present inlist_tags(). That string can be mistaken for a real tag name by callers; it’s safer to drop unknown ids (or surface them separately) and return the real tag fields (includingassignable) for known tags.
"tags": [
{"id": i, "name": names.get(i, f"(unknown tag {i})")}
for i in sorted(set(ids))
],
nextcloud_mcp_server/server/webdav.py:1198
nc_webdav_get_file_tagsdrops theassignableproperty when buildingSystemTag, so non-assignable tags will be reported as assignable by default. Preserve all fields returned by the client tag dict when constructingSystemTag.
file_id=str(data["file_id"]),
tags=[SystemTag(id=t["id"], name=t["name"]) for t in data["tags"]],
)
nextcloud_mcp_server/server/webdav.py:1193
nc_webdav_get_file_tagsresolves the file id via_resolve_file_id()and then callsclient.webdav.get_file_tags(path), which does anotherget_fileid()lookup internally (client/webdav.py:2758). This doubles the PROPFIND work for every call; consider allowingget_file_tags()to accept a pre-resolved file_id (or adding aget_file_tags_by_id()helper) and pass the_resolve_file_idresult through.
# Resolving through the shared helper applies the excluded-tag guard,
# so an excluded path cannot be probed for existence via its tags.
await _resolve_file_id(client, path)
data = await client.webdav.get_file_tags(path)
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
The WebDAV client already implements system tags in full —
create_tag,get_or_create_tag,assign_tag_to_file,remove_tag_from_file,get_tag_by_name,get_files_by_tag,find_by_tag— but no tool exposes any of it, so tagging is unreachable through the MCP server. This PR closes that gap.Tools
nc_webdav_list_tagsnc_webdav_get_file_tagsnc_webdav_find_by_tag_namenc_webdav_tag_filenc_webdav_untag_fileThey take tag names rather than ids. An id is unguessable for a language model; a name is not.
nc_webdav_get_file_tagsadditionally fetches the full tag list, because Nextcloud reports only ids on the file itself — one extra call, but ids alone are of little use to the caller.Two read paths were missing on the client and are added here:
list_tags()andget_file_tags(). Everything else was already there.Deliberately not included
There is no
delete_tagtool. Deleting a system tag removes it from every file that carries it, and that felt like too blunt an instrument to hand to an agent without a confirmation step. Easy to add if you disagree.Testing
Verified end to end against Nextcloud 33: create a file, tag it, read the tags back, find the file via the tag, untag it, confirm the tags are gone, and confirm that an unknown tag name yields a clear message rather than an error.
tests/unit/client/test_webdav_tags.pycovers the multistatus parsing — collection element skipped, case-insensitive sort,user-assignableflag, and entries without an id dropped rather than surfaced with placeholders. The parsing was factored into_tags_from_multistatus()so the test exercises the real code path rather than a copy of it.Note from the field
On my instance
nc_webdav_list_tagsreturns 337 tags, almost all machine-generated by the Recognize app ("abseiling", "Alligator", "Aircraft"). Anything presenting this list to a user or model will mostly show object-detection vocabulary — worth knowing, though I do not think it changes anything here.🤖 Generated with Claude Code
https://claude.ai/code/session_01GomNRo5jTc6rDNNLYrRVRX