feat(collectives): Add Collectives support (13 tools for page management) - #159
feat(collectives): Add Collectives support (13 tools for page management)#159Pavlinchen wants to merge 6 commits into
Conversation
|
Tested and confirmed working. |
|
Likely open to do: add AI disclaimer (like for PR #155)? @marcelklehr What do you think? |
|
not sure either, will discuss internally :) |
|
To avoid this entirely, perhaps we can replace the update_page_content with more of a "surgical" insert? |
|
How about, we add to the tool description to make sure there is a note at the bottom of the article that says it was edited with the help of AI? Then the agent can make sure the note is always there and we don't have the problem of adding or removing it superfluously |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
I think that could work, but tagging affected pages with It's your call |
|
Hello @marcelklehr friendly reminder on this PR aswell |
|
pinging @janepie for input :) |
c8ee19d to
487e826
Compare
|
(force pushed to use alias e-Mail adress for DCO) |
|
The error is: From what I can see stable 31 - master all support PHP 8.3 (and 8.4 for that matter, see below) - @marcelklehr could it be a fix to bump "php-versions: [ '8.2' ]" to 8.3 in https://github.com/nextcloud/context_agent/blob/main/.github/workflows/integration_test.yml#L29 (like in 4e763ee)?
Alternatively we could do something like at https://github.com/nextcloud/calendar/blob/main/.github/workflows/php-test.yml#L61-L70 & https://github.com/nextcloud/mail/blob/main/.github/workflows/test.yml#L69-L101 - in our case: matrix:
databases: [ 'sqlite' ]
server-versions: [ 'stable31', 'stable32' ]
php-versions: [ '8.1' ]
include:
- server-versions: 'stable33'
php-versions: '8.2'
- server-versions: 'stable34'
php-versions: '8.2'
- server-versions: 'master'
php-versions: '8.3' |
Add a new tool module wrapping the Collectives OCS API (under /ocs/v2.php/apps/collectives/api/v1.0) plus WebDAV for page markdown content, giving the agent 13 tools covering the full page lifecycle. Read (safe): - list_collectives: enumerate the user's collectives with permissions - list_collective_pages: flat list of pages with tree metadata (parentId + subpageOrder) for a collective - get_page: metadata for a single page - get_page_content: markdown body via WebDAV, empty string on 404 - list_page_trash: enumerate trashed pages Write (dangerous): - create_page: new page under a parent - update_page_content: overwrite markdown body via WebDAV PUT - rename_page: change title (also renames the .md file) - move_page: change parentId within the collective - set_page_emoji: set/clear page emoji - trash_page: soft-delete - restore_page: from trash - delete_page_permanently: purge a trashed page Markdown I/O uses the same WebDAV adapter pattern as files.py (nc._session._create_adapter(True)) and the page's collectivePath + filePath + fileName from the metadata. Paths are URL-encoded via urllib.parse.quote. Collectives does not register an OCS capability, so is_available() probes the API (matching the pattern in mail.py) rather than checking nc.capabilities. Unified search already exposes three Collectives providers (collectives, collectives-pages, collectives-page-content) via search.py, so no separate search tool is added here. Tested against Nextcloud 32.0.8 with Collectives 3.6.1 on the full lifecycle (create, write, read, rename, move, emoji, trash, restore, permanent delete) plus URL construction for top-level and nested pages with spaces in names. Signed-off-by: Pavlinchen <69079839+Pavlinchen@users.noreply.github.com>
487e826 to
c8648bf
Compare
|
Hi @janepie, CI is green as far as I can tell, do you need anything else from me? |
|
Ah, this is still to be decided:
|
|
Let's go with @marcelklehr s idea for now, we may change it to a more integrated approach in the future but I'd consider that out of scope for this PR :) |
|
as you wish. I will pick up work on that hopefully some time soon |
…ed note at the bottom Signed-off-by: Marcel Klehr <mklehr@gmx.net>
c5b245d to
3d764e2
Compare
3d764e2 asks the model, through the tool description, to make sure a note is at the bottom of the page. This replaces that instruction with the tool appending the note on its own, which closes two gaps the instruction leaves open. The note is only there when the model remembers it. update_page_content replaces the whole page, and the agent normally builds the new body by reading the old one back with get_page_content first, so every edit is another chance to drop it. And the wording the model re-emits drifts between edits, which leaves no stable string to match against - so a note cannot be replaced, only added, and copies pile up. Appending it in the tool makes the note a property of the write rather than of the prompt, which is how the other write tools in this repo behave. The tool strips its own note before re-appending it, so exactly one survives any number of edits. The pattern matches only that note, on a line of its own, and tolerates what a markdown serializer can do to it on the way back - a dropped emoji or variation selector, backslash-escaped emphasis, CRLF - while leaving a blockquote a user wrote themselves alone, even one that names the Assistant. Removing the note closes only the seam it leaves behind; blank runs anywhere else on the page are kept byte-for-byte, which matters inside fenced code blocks. The docstrings of update_page_content and get_page_content now say the note is appended automatically and replaced rather than duplicated, so the model neither writes one itself nor tries to strip the one it reads back. Verified against Nextcloud 32.0.8 / Collectives 3.6.1: two consecutive Assistant edits leave one note with the first edit's content intact, a human editing the page in the Text editor in between round-trips the note byte-identically, a user-authored blockquote mentioning the Assistant survives, and blank lines inside a fenced code block are untouched. Signed-off-by: Pavlinchen <69079839+Pavlinchen@users.noreply.github.com>
|
Hi @marcelklehr, This way
I've added my changes on top of yours, but removed your docstring line again, so there are no double-disclaimers. |
Nice, that's even better :) |
There was a problem hiding this comment.
Pull request overview
Adds Collectives page management through OCS and WebDAV.
Changes:
- Adds five page discovery/read tools.
- Adds eight page lifecycle/write tools.
- Adds WebDAV path handling and availability detection.
Suppressed comments (1)
ex_app/lib/all_tools/collectives.py:177
- The PUT response is discarded, so a rejected or failed WebDAV write still returns
{"status": "success"}. Retain the response and raise on unsuccessful status codes before reporting success.
await nc._session._create_adapter(True).request('PUT', url, headers={
'Content-Type': 'text/markdown',
}, data=stamped)
return json.dumps({'status': 'success', 'page_id': page_id})
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if response.status_code == 404: | ||
| return '' | ||
| return response.text |
Three findings from the Copilot review on nextcloud#159, all the same shape: a failed request was treated as a successful one. get_page_content only special-cased 404. Any other failure was handed back as if the response body were the page's markdown - in practice a DAV error document, which the agent would then summarize as page content. The 404 contract is unchanged and still comes first; every 4xx and 5xx now raises. update_page_content discarded the PUT response entirely and always reported success, so a refused write looked like a completed one to the agent and to the user. The realistic case is the one the tool's own docstring warns about: a page held open in the real-time editor is locked, the write comes back 423, and the tool said it had gone through. Both use response.raise_for_status(). The review suggested it verbatim for the read; the write is the same problem and gets the same treatment. It is also what nc_py_api's own check_error() uses internally. Its message carries status, reason and URL, and graph.py's handle_tool_error hands repr() of the exception back to the model, so the agent sees what failed rather than a bare status code. 2xx multi-status responses do not raise. is_available caught bare, which also swallows asyncio.CancelledError and turns a cancellation during tool discovery into "Collectives unavailable" while discovery carries on - freezing an incomplete tool list in the 60-second cache around it. It now catches Exception, so cancellation propagates. Signed-off-by: Pavlinchen <69079839+Pavlinchen@users.noreply.github.com>
|
Sorry for the long review process @Pavlinchen I hope to merge the PR this week 🎉 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
| parts = [page['collectivePath'], page.get('filePath') or '', page['fileName']] | ||
| encoded = [quote(p) for p in parts if p] | ||
| return f"{nc.app_cfg.endpoint}/remote.php/dav/files/{user_id}/{'/'.join(encoded)}" |
There was a problem hiding this comment.
Applied in be856f5. '/' stays unencoded on purpose: in these fields it separates real path levels (nested pages return filePath 'A/B').
Encoding it would break every nested page, and a '/' inside a name is really rare, as the values come from the server's own filesystem (and it's really not best practice).
There was a problem hiding this comment.
actually for collectives this seems to not be possible at all: https://github.com/nextcloud/collectives/blob/main/lib/Fs/NodeHelper.php#L75-L110
|
@Pavlinchen Would you mind fixing these? I can also do it, if you're busy by now :) Thank you for your patience! |
|
I will do a bit more testing today and let you know how far I got |
_user_id() fetched /ocs/v2.php/cloud/user on every page read and write for a value the ExApp session already holds: for app sessions nc.user skips that request and returns the id from the AppAPI auth context, the same way memory.py builds its WebDAV paths. The id is load-bearing before this module even runs - the tool discovery cache keys every lookup on await nc.user - and the OCS call authenticated with that same id, so it could only ever echo it back. Saves one round trip per call of get_page_content and update_page_content. Signed-off-by: Pavlinchen <69079839+Pavlinchen@users.noreply.github.com>
…on reads Two findings from the second Copilot review. The page path fields are now joined, split on '/' and every segment quoted with safe='', the stricter of the two idioms already in the repo (memory.py encodes segments the same way). For anything the Collectives API can return this produces the same URL as before: quote()'s default already encoded everything except '/', and the separators stay separators either way. What changes is that the path is canonicalized - empty segments from leading, doubled or trailing slashes collapse instead of reaching the URL - and that the per-segment form makes the invariant the review asked about explicit rather than implicit. The GET in get_page_content sent a Content-Type header, which describes a request body that a GET does not have. It now sends Accept: text/markdown, as suggested. The response is byte-identical either way - sabre/dav does not negotiate on Accept. Signed-off-by: Pavlinchen <69079839+Pavlinchen@users.noreply.github.com>
|
I hope I didn't miss anything... |
Summary
collectives.pytool module wrapping the Collectives OCS API plus WebDAV for page markdown I/Osearch.pyalready auto-generates 3 Collectives search tools from the existing search providers (collectives,collectives-pages,collectives-page-content)Motivation
The Context Agent currently has no interaction with Collectives, Nextcloud's wiki-style knowledge base app. This prevents the agent from reading, creating, or editing user knowledge bases — a core collaboration use case. The existing unified search tools can find Collectives pages, but not enumerate, create, or modify them.
Implementation Details
Disclaimer
Implementation from a technical perspective was done mainly by a LLM (Claude Opus 4.7 max effort).
Every tool and endpoint response shape was validated end-to-end against Nextcloud 32.0.8 + Collectives 3.6.1, with a full lifecycle smoke test completed before opening the PR.
Endpoints used
All OCS endpoints are under
/ocs/v2.php/apps/collectives/api/v1.0/. Markdown I/O uses WebDAV at/remote.php/dav/files/{user}/{collectivePath}/{filePath}/{fileName}, with the path components coming from the page's own metadata./collectives/collectives/{cid}/pages/collectives/{cid}/pages/{pid}/collectives/{cid}/pages/trash/collectives/{cid}/pages/{parentId}/collectives/{cid}/pages/{pid}(body{title})/collectives/{cid}/pages/{pid}(body{parentId})/collectives/{cid}/pages/{pid}/emoji/collectives/{cid}/pages/{pid}/collectives/{cid}/pages/trash/{pid}/collectives/{cid}/pages/trash/{pid}Patterns followed
nc.ocs()(matchescircles.py,shares.py)nc._session._create_adapter(True).request(...)(matchesfiles.py)is_available()probes the OCS endpoint and returnsFalseon exception (matchesmail.py, since Collectives does not register an OCS capability)urllib.parse.quoteto handle spaces and special characters in collective/page namesDesign decisions
get_page_contentreturns an empty string on 404. Pages typically materialize their.mdfile on creation, but under transient conditions the file may not yet exist — the empty string gives the agent a consistent contract.No standalone search tool.
search.pyalready generatessearch_collectives,search_collectives_pages, andsearch_collectives_page_contentvia unified search providers. Adding a scoped search would be redundant.No Collective-level create/delete tools. Creating a Collective also provisions a backing Circle/Team, which is tied to membership management. Better addressed in a separate PR if desired.
No tag tools in this PR. Tags (Collectives 3.1.0+) are a small, well-contained sub-feature that can be added in a follow-up PR without affecting the page tools.
Test Plan
Tested against Nextcloud 32.0.8 + Collectives 3.6.1 with a full-lifecycle smoke test:
list_collectivesreturns all user's collectives withlevel,canEdit,canSharelist_collective_pagesreturns flat list withparentIdandsubpageOrderfor tree reconstructionget_pagereturns single-page metadataget_page_contentfetches markdown via WebDAV for a top-level page (landing)get_page_contentfetches markdown via WebDAV for a nested page in a collective whose name contains spaceslist_page_trashreturns empty when no trashed pagescreate_pagecreates a child under the specified parentupdate_page_contentwrites markdown; subsequentget_page_contentreturns the written contentrename_pageupdates title and thefileNameon diskmove_pageupdatesparentIdand the resultingfilePathset_page_emojiround-trips UTF-8 emoji characterstrash_pagesetstrashTimestamp; page appears inlist_page_trashrestore_pageclearstrashTimestampdelete_page_permanentlyremoves the trashed page (HTTP 200)