From 95756623552286498111610ad6a234623e4d59e2 Mon Sep 17 00:00:00 2001 From: akhil-vamshi-konam Date: Mon, 17 Aug 2026 14:51:55 +0530 Subject: [PATCH 1/3] feat: add templates, page collections and page lifecycle --- CLAUDE.md | 13 +- README.md | 7 +- plane_mcp/middleware.py | 48 +++++-- plane_mcp/server.py | 5 +- plane_mcp/tools/README.md | 10 +- plane_mcp/tools/collection.py | 243 ++++++++++++++++++++++++++++++++ plane_mcp/tools/legacy.py | 7 +- plane_mcp/tools/page.py | 126 ++++++++++++++++- plane_mcp/tools/registry.py | 9 +- plane_mcp/tools/template.py | 207 +++++++++++++++++++++++++++ pyproject.toml | 4 +- tests/tools/test_conformance.py | 16 ++- tests/tools/test_dispatch.py | 9 +- tests/tools/test_governance.py | 1 + uv.lock | 19 ++- 15 files changed, 681 insertions(+), 43 deletions(-) create mode 100644 plane_mcp/tools/collection.py create mode 100644 plane_mcp/tools/template.py diff --git a/CLAUDE.md b/CLAUDE.md index b165763..50a92b4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,6 +57,16 @@ Ordered as registered; the earlier one wraps the later: | `CoerceArguments` | repairs arguments a client encoded as strings, before validation (`coercion.py`) | | `ValidateActionArguments` | refuses arguments the chosen action does not accept, from the `ACTIONS` declaration | +A dispatch tool's name is not its operation, so `PlaneLoggingMiddleware` adds `resource` and `action` to every `tools/call` record — start, success and error alike, where previously only success and error carried anything. For a retired name both are resolved through the alias table, since it carries no `action` of its own. + +| field | means | +|---|---| +| `tool` | the name the caller used — **unchanged**, so existing dashboards keep counting the same thing | +| `resource` | the resource tool that ran (`workitem`); equals `tool` for a current call | +| `action` | the operation (`count`), absent only for a tool that has no actions | + +`resource` + `action` names one operation however it was reached, and `tool != resource` is exactly the set of calls still arriving on a retired name. `legacy.py` also keeps its per-resolution log line, which predates these fields. + Coercion runs before validation so an argument is judged by the value it repairs to. `ValidateActionArguments` closes a gap a per-tool schema cannot: every action's parameters share one schema, so an argument meant for another action validated cleanly and was then dropped, and the call answered a different question than the one asked. Only arguments carrying a value are judged, and retired names are exempt — they arrive with no `action` and under their own parameter spelling. ### Client Context (`client.py`) @@ -70,7 +80,7 @@ Coercion runs before validation so an argument is judged by the value it repairs ### Tools (`tools/`) -One action-dispatch tool per Plane resource: **28 tools, 183 actions, ~57k chars advertised**. `tools/__init__.py` re-exports `register_tools`, so `server.py` and `__main__.py` see a single entry point. +One action-dispatch tool per Plane resource: **30 tools, 204 actions, ~67k chars advertised**. `tools/__init__.py` re-exports `register_tools`, so `server.py` and `__main__.py` see a single entry point. One module per resource, each exporting `NAME`, `ACTIONS`, `LEGACY` and `register(mcp)`. `ACTIONS` is the single source of truth: the tool description and its `ToolAnnotations` are generated from it, and the conformance suite asserts they agree with the function signature. See `tools/README.md` for the full convention. @@ -122,3 +132,4 @@ Integration tests in `tests/test_integration.py` use `FastMCP.Client` with `Stre | `PLANE_OAUTH_PROVIDER_*` | http/sse OAuth | OAuth client credentials and base URL | | `PLANE_OAUTH_ALLOWED_REDIRECT_URIS` | http/sse OAuth (optional) | Comma-separated redirect URI patterns appended to the built-in allowlist (onboard clients without a release) | | `LOG_USER_INFO` | all (optional, default: false) | When `true`, include user info (PII such as display name) in logs alongside the opaque user id | +| `LOG_PAYLOADS` | all (optional, default: true) | Log request payloads.| diff --git a/README.md b/README.md index 1b069f6..d07ff43 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ work items, cycles, modules, releases, customers and more. Built on [FastMCP](https://github.com/jlowin/fastmcp) and the official [`plane-sdk`](https://pypi.org/project/plane-sdk/). -- **28 tools**, one per Plane resource, covering 183 operations +- **30 tools**, one per Plane resource, covering 204 operations - **Local or remote** — stdio, streamable HTTP, SSE - **OAuth or API key** authentication @@ -98,7 +98,7 @@ HTTP transport instead. ## Tools -The server advertises 28 tools, one per resource. Each takes an `action` +The server advertises 30 tools, one per resource. Each takes an `action` parameter that selects the operation: ```python @@ -179,7 +179,8 @@ Structured JSON. Each tool call logs its name, duration, status and — when available — an opaque user id and the workspace slug. ```bash -export LOG_USER_INFO=true # also log the display name (PII); default false +export LOG_USER_INFO=true # also log the display name (PII); +export LOG_PAYLOADS=false # keep request payloads out of logs; default true ``` Only the OAuth and PAT transports carry a display name; stdio is unaffected. diff --git a/plane_mcp/middleware.py b/plane_mcp/middleware.py index 7cece18..35605a3 100644 --- a/plane_mcp/middleware.py +++ b/plane_mcp/middleware.py @@ -3,6 +3,7 @@ from __future__ import annotations from collections.abc import Collection +from typing import Any from fastmcp.server.middleware import Middleware, MiddlewareContext from fastmcp.server.middleware.logging import StructuredLoggingMiddleware @@ -10,7 +11,7 @@ from fastmcp.utilities.logging import get_logger from plane_mcp.coercion import coerce_arguments -from plane_mcp.tools.registry import action_arguments +from plane_mcp.tools.registry import action_arguments, alias_table logger = get_logger(__name__) @@ -91,15 +92,44 @@ async def _schema(context: MiddlewareContext) -> dict | None: class PlaneLoggingMiddleware(StructuredLoggingMiddleware): - """StructuredLoggingMiddleware that also records the tool name.""" - - def _with_tool_name(self, context: MiddlewareContext, message: dict) -> dict: - if context.method == "tools/call": - message["tool"] = getattr(context.message, "name", "unknown") - return message + """StructuredLoggingMiddleware that records which operation ran, not just which tool. + + A dispatch tool's name is not its operation -- `workitem` covers 23 of them -- so + `resource` and `action` are recorded beside it, resolved through the alias table for + a retired name, which carries no `action` of its own. `resource` + `action` then + names one operation however it was reached, and `tool != resource` is exactly the + set of calls still arriving on a retired name. + + `tool` keeps its previous meaning -- the name the caller used -- so dashboards built + on it keep counting the same thing. The two additions are additive, and they are on + the start record as well, which previously carried neither. + """ + + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + # Built once; per record this is a dict lookup. + self._aliases = alias_table() + + def _operation(self, context: MiddlewareContext) -> dict[str, str]: + """What the caller called, and which operation that is.""" + if context.method != "tools/call": + return {} + name = getattr(context.message, "name", "unknown") + if alias := self._aliases.get(name): + resource, action = alias + else: + resource = name + action = (getattr(context.message, "arguments", None) or {}).get("action") + fields = {"tool": name, "resource": resource} + if action: + fields["action"] = action + return fields + + def _create_before_message(self, context: MiddlewareContext, *args: Any, **kwargs: Any) -> dict: + return super()._create_before_message(context, *args, **kwargs) | self._operation(context) def _create_after_message(self, context: MiddlewareContext, start_time: float) -> dict: - return self._with_tool_name(context, super()._create_after_message(context, start_time)) + return super()._create_after_message(context, start_time) | self._operation(context) def _create_error_message(self, context: MiddlewareContext, start_time: float, error: Exception) -> dict: - return self._with_tool_name(context, super()._create_error_message(context, start_time, error)) + return super()._create_error_message(context, start_time, error) | self._operation(context) diff --git a/plane_mcp/server.py b/plane_mcp/server.py index 91cbd49..2ccc2cc 100644 --- a/plane_mcp/server.py +++ b/plane_mcp/server.py @@ -48,9 +48,12 @@ def get_allowed_client_redirect_uris() -> list[str]: return allowed +LOG_PAYLOADS = os.getenv("LOG_PAYLOADS", "true").lower() == "true" + + def _configured(mcp: FastMCP) -> FastMCP: """The middleware stack and tools every transport shares.""" - mcp.add_middleware(PlaneLoggingMiddleware(include_payloads=True)) + mcp.add_middleware(PlaneLoggingMiddleware(include_payloads=LOG_PAYLOADS)) mcp.add_middleware(CoerceArguments()) mcp.add_middleware(ValidateActionArguments()) register_tools(mcp) diff --git a/plane_mcp/tools/README.md b/plane_mcp/tools/README.md index eb57f06..27e08e4 100644 --- a/plane_mcp/tools/README.md +++ b/plane_mcp/tools/README.md @@ -1,7 +1,7 @@ # The tool surface -**28 tools**, one per Plane resource, each taking an `action` parameter that -selects the operation. 183 actions in total. +**30 tools**, one per Plane resource, each taking an `action` parameter that +selects the operation. 204 actions in total. ```python workitem(action="create", project_id=..., name="Fix login") @@ -9,7 +9,7 @@ workitem(action="list", project_id=..., pql='state__group = "started"') cycle(action="archive", project_id=..., cycle_id=...) ``` -A compact catalogue — 28 tools, ~57k characters — loads fully in every MCP client +A compact catalogue — 30 tools, ~67k characters — loads fully in every MCP client and leaves the context budget to the conversation. ## The shape of a resource module @@ -194,6 +194,7 @@ adopt anyway if the project write is refused. | Tool | Actions | |---|---| +| `collection` | `list` · `retrieve` · `create` · `update` · `delete` · `list_pages` · `search_pages` · `add_pages` · `remove_page` · `list_members` · `add_member` · `update_member` · `remove_member` | | `customer` | `list` · `retrieve` · `create` · `update` · `delete` · `list_workitems` · `manage_workitems` | | `customer_property` | `list` · `retrieve` · `create` · `update` · `delete` · `get_values` · `set_values` | | `customer_request` | `list` · `retrieve` · `create` · `update` · `delete` | @@ -205,13 +206,14 @@ adopt anyway if the project write is refused. | `member` | `me` · `list_workspace` · `list_project` · `list_roles` · `retrieve_role` | | `milestone` | `list` · `retrieve` · `create` · `update` · `delete` · `list_workitems` · `manage_workitems` | | `module` | `list` · `retrieve` · `create` · `update` · `delete` · `list_workitems` · `manage_workitems` · `archive` · `unarchive` | -| `page` | `list` · `retrieve` · `create` · `list_workitem_pages` · `attach_to_workitem` · `detach_from_workitem` | +| `page` | `list` · `retrieve` · `create` · `update` · `archive` · `delete` · `set_collection` · `list_workitem_pages` · `attach_to_workitem` · `detach_from_workitem` | | `project` | `list` · `retrieve` · `create` · `update` · `delete` · `archive` · `unarchive` · `worklog_summary` · `get_features` · `update_features` | | `project_estimate` | `retrieve` · `create` · `update` · `delete` · `link` · `list_points` · `create_points` · `update_point` · `delete_point` | | `release` | `list` · `retrieve` · `create` · `update` · `delete` · `get_changelog` · `update_changelog` · `list_workitems` · `manage_workitems` | | `release_label` | `list` · `create` · `update` · `delete` · `attach` · `detach` | | `release_tag` | `list` · `retrieve` · `create` · `update` · `delete` | | `state` | `list` · `retrieve` · `create` · `update` · `delete` | +| `template` | `list` · `create` · `update` · `delete` | | `work_log` | `list` · `create` · `update` · `delete` | | `workitem` | `list` · `list_archived` · `retrieve` · `retrieve_by_identifier` · `search` · `count` · `create` · `update` · `delete` · `archive` · `manage_assignee` · `manage_label` | | `workitem_activity` | `list` · `retrieve` | diff --git a/plane_mcp/tools/collection.py b/plane_mcp/tools/collection.py new file mode 100644 index 0000000..1c71b7d --- /dev/null +++ b/plane_mcp/tools/collection.py @@ -0,0 +1,243 @@ +"""Collections: the workspace-level folders that group pages.""" + +from __future__ import annotations + +from typing import Any, Literal + +from fastmcp import FastMCP +from plane.models.collections import ( + AddCollectionPages, + Collection, + CollectionMember, + CollectionPage, + CollectionPageSearchResult, + CreateCollection, + CreateCollectionMember, + UpdateCollection, + UpdateCollectionMember, +) +from plane.models.query_params import CollectionPageQueryParams + +from plane_mcp.client import get_plane_client_context +from plane_mcp.toolkit import ( + Action, + as_params, + build_annotations, + build_description, + coerce_list, + envelope, + missing, + needs, + one_of, + opt, +) + +NAME = "collection" +TITLE = "Page collections" + +# 0 is a real level in both, so neither can use the 0 sentinel, and they are +# different scales -- reading one as the other silently grants or denies access. +ACCESS = {"public": 0, "private": 1} +MEMBER_ACCESS = {"view": 0, "comment": 1, "edit": 2} + +ACTIONS = ( + Action("list", (), read=True), + Action("retrieve", ("collection_id",), read=True), + Action("create", ("name",), ("access",), note="access is fixed at creation and cannot be changed afterwards"), + Action("update", ("collection_id",), ("name", "sort_order"), note="only the fields you pass are changed"), + Action( + "delete", + ("collection_id",), + ("archive_pages",), + note="the pages survive; archive_pages defaults to true, pass false to leave them unfiled instead", + destructive=True, + ), + Action("list_pages", ("collection_id",), ("search", "parent_id", "cursor", "per_page"), read=True), + Action( + "search_pages", + ("collection_id",), + ("search",), + note="pages not yet in this collection, to pick ids for add_pages", + read=True, + ), + Action("add_pages", ("collection_id", "page_ids"), note="files existing pages; use page create to make new ones"), + Action( + "remove_page", + ("collection_id", "page_collection_id"), + note="page_collection_id is the membership id from list_pages, not the page id; the page itself is kept", + ), + Action("list_members", ("collection_id",), read=True), + Action("add_member", ("collection_id", "user_id", "member_access")), + Action("update_member", ("collection_id", "collection_member_id", "member_access")), + Action( + "remove_member", + ("collection_id", "collection_member_id"), + note="collection_member_id is the membership id from list_members, not the user id", + ), +) + +FOOTER = ( + f"access is one of: {', '.join(ACCESS)} -- a private collection is visible only to its members, " + "and the level cannot be changed once the collection exists. " + f"member_access is one of: {', '.join(MEMBER_ACCESS)} and is a separate scale from access. " + "Collections group workspace pages only; a project's pages cannot be filed in one. " + "To file or move a page, use `page set_collection` -- it works out which collection holds the " + "page for you, so it needs no membership id." +) + +# A new resource: nothing was ever advertised under another name. +LEGACY: dict[str, str] = {} + + +def register(mcp: FastMCP) -> None: + @mcp.tool( + name=NAME, + description=build_description("Collections grouping workspace pages.", ACTIONS, FOOTER), + annotations=build_annotations(TITLE, ACTIONS), + ) + def collection( + action: Literal[ + "list", + "retrieve", + "create", + "update", + "delete", + "list_pages", + "search_pages", + "add_pages", + "remove_page", + "list_members", + "add_member", + "update_member", + "remove_member", + ], + collection_id: str = "", + name: str = "", + access: str = "", + member_access: str = "", + user_id: str = "", + collection_member_id: str = "", + page_ids: str = "", + page_collection_id: str = "", + parent_id: str = "", + search: str = "", + # Tri-state: the server's own default is true, which is not the same as unset. + archive_pages: bool | None = None, + # 0 is a real sort position, so it cannot use the 0 sentinel. + sort_order: float | None = None, + cursor: str = "", + per_page: int = 0, + ) -> ( + Collection + | list[Collection] + | CollectionMember + | list[CollectionMember] + | CollectionPage + | list[CollectionPage] + | list[CollectionPageSearchResult] + | dict[str, Any] + | str + | None + ): + client, workspace_slug = get_plane_client_context() + collections = client.collections + + if error := one_of("access", access, tuple(ACCESS)): + return error + if error := one_of("member_access", member_access, tuple(MEMBER_ACCESS)): + return error + + if action == "list": + return collections.list(workspace_slug=workspace_slug) + + if action == "create": + if not name: + return missing(action, "name") + return collections.create( + workspace_slug=workspace_slug, + data=CreateCollection(name=name, access=ACCESS.get(access)), + ) + + if not collection_id: + return missing(action, "collection_id") + + if action == "retrieve": + return collections.retrieve(workspace_slug=workspace_slug, collection_id=collection_id) + + if action == "update": + return collections.update( + workspace_slug=workspace_slug, + collection_id=collection_id, + data=UpdateCollection(name=opt(name), sort_order=sort_order), + ) + + if action == "delete": + collections.delete(workspace_slug=workspace_slug, collection_id=collection_id, archive_pages=archive_pages) + return None + + if action == "list_pages": + response = collections.pages.list( + workspace_slug=workspace_slug, + collection_id=collection_id, + params=as_params( + CollectionPageQueryParams, + search=search, + parent_id=parent_id, + cursor=cursor, + per_page=per_page, + ), + ) + return envelope(response) + + if action == "search_pages": + return collections.pages.search( + workspace_slug=workspace_slug, collection_id=collection_id, search=opt(search) + ) + + if action == "add_pages": + ids = coerce_list(page_ids) + if not ids: + return missing(action, "page_ids") + return collections.pages.add( + workspace_slug=workspace_slug, collection_id=collection_id, data=AddCollectionPages(page_ids=ids) + ) + + if action == "remove_page": + if not page_collection_id: + return missing(action, "page_collection_id") + collections.pages.remove( + workspace_slug=workspace_slug, + collection_id=collection_id, + page_collection_id=page_collection_id, + ) + return None + + if action == "list_members": + return collections.members.list(workspace_slug=workspace_slug, collection_id=collection_id) + + if action == "add_member": + if error := needs(action, user_id=user_id, member_access=member_access): + return error + return collections.members.add( + workspace_slug=workspace_slug, + collection_id=collection_id, + data=CreateCollectionMember(member=user_id, access=MEMBER_ACCESS[member_access]), + ) + + if not collection_member_id: + return missing(action, "collection_member_id") + + if action == "update_member": + if not member_access: + return missing(action, "member_access") + return collections.members.update( + workspace_slug=workspace_slug, + collection_id=collection_id, + member_id=collection_member_id, + data=UpdateCollectionMember(access=MEMBER_ACCESS[member_access]), + ) + + collections.members.remove( + workspace_slug=workspace_slug, collection_id=collection_id, member_id=collection_member_id + ) + return None diff --git a/plane_mcp/tools/legacy.py b/plane_mcp/tools/legacy.py index 1524c44..f9e750c 100644 --- a/plane_mcp/tools/legacy.py +++ b/plane_mcp/tools/legacy.py @@ -11,7 +11,8 @@ its old name has no reason to have followed that: resolving the name but rejecting `work_item_id` would be a rename dressed up as compatibility. -Each resolution is logged, so when removing these is scheduled, "nobody still +Each resolution is logged, and every call through one records `tool` != `resource` +(see `PlaneLoggingMiddleware`), so when removing these is scheduled, "nobody still calls them" is an observation rather than an assumption. """ @@ -41,8 +42,8 @@ async def get_tool(self, name: str, call_next: GetToolNext, *, version=None) -> return await call_next(name, version=version) tool_name, action = target - # Grep-able on purpose: the names appearing here over a release are the - # callers that removing these aliases would break. + # Kept alongside the `resource`/`retired` log fields: this line predates them and + # is grep-able, so removing it would break whatever already counts these. logger.info("Plane MCP: retired tool name %r resolved to %r %r", name, tool_name, action) parent = await call_next(tool_name, version=version) if parent is None: diff --git a/plane_mcp/tools/page.py b/plane_mcp/tools/page.py index 2eb09d0..e4f3581 100644 --- a/plane_mcp/tools/page.py +++ b/plane_mcp/tools/page.py @@ -1,4 +1,4 @@ -"""Pages, at workspace or project scope, and their links to work items. +"""Pages, at workspace or project scope, their hierarchy, and their links to work items. Every page action is scoped by whether project_id is supplied: with it the page is a project page, without it a workspace page. The SDK has a separate endpoint @@ -10,7 +10,8 @@ from typing import Any, Literal from fastmcp import FastMCP -from plane.models.pages import CreatePage, Page +from plane.models.collections import AddCollectionPages, UpdateCollectionPage +from plane.models.pages import CreatePage, Page, UpdatePage from plane.models.query_params import PaginatedQueryParams from plane.models.work_item_pages import CreateWorkItemPage, WorkItemPage @@ -28,7 +29,44 @@ Action( "create", ("name", "description_html"), - ("project_id", "access", "color", "is_locked", "external_source", "external_id"), + ( + "project_id", + "parent_id", + "collection_id", + "access", + "color", + "is_locked", + "external_source", + "external_id", + ), + note="parent_id nests the new page under an existing one; collection_id files it. " + "Pass one or the other, never both", + ), + Action( + "update", + ("page_id",), + ("project_id", "name", "description_html"), + note="pass name, description_html, or both; description_html replaces the whole body, " + "so retrieve the page first when editing part of it; a locked or archived page is refused", + ), + Action( + "archive", + ("page_id",), + ("project_id", "archive"), + note="archive defaults to true; pass archive=false to restore", + ), + Action( + "delete", + ("page_id",), + ("project_id",), + note="requires the page to be archived first", + destructive=True, + ), + Action( + "set_collection", + ("page_id", "collection_id"), + note="files a page into a collection, or moves it out of the one it is in; workspace pages only, " + "and collection_id comes from the collection tool", ), Action("list_workitem_pages", ("project_id", "workitem_id"), read=True), Action("attach_to_workitem", ("project_id", "workitem_id", "page_id")), @@ -42,7 +80,13 @@ FOOTER = ( "description_html is the page body as HTML. access is the page access level. " - "Omit project_id to work with workspace-level pages." + "update changes only the fields you pass. A page must be archived before it can be deleted. " + "Omit project_id to work with workspace-level pages. " + "A page's parent is fixed at creation -- pass parent_id to create to build a hierarchy, since " + "nothing can reparent it afterwards. list and retrieve both report a page's parent_id and the " + "collection_id it is filed in, so neither needs looking up. " + "Collections themselves live in the collection tool; here, create files a new page into one and " + "set_collection files or moves an existing page." ) LEGACY = { @@ -66,12 +110,18 @@ def page( "list", "retrieve", "create", + "update", + "archive", + "delete", + "set_collection", "list_workitem_pages", "attach_to_workitem", "detach_from_workitem", ], project_id: str = "", page_id: str = "", + parent_id: str = "", + collection_id: str = "", workitem_id: str = "", workitem_page_id: str = "", name: str = "", @@ -80,6 +130,7 @@ def page( access: int | None = None, color: str = "", is_locked: bool | None = None, + archive: bool = True, external_source: str = "", external_id: str = "", cursor: str = "", @@ -106,15 +157,51 @@ def page( ) return client.pages.retrieve_workspace_page(workspace_slug=workspace_slug, page_id=page_id) + if action == "archive": + if not page_id: + return missing(action, "page_id") + if project_id: + mover = client.pages.archive_project_page if archive else client.pages.unarchive_project_page + mover(workspace_slug=workspace_slug, project_id=project_id, page_id=page_id) + else: + mover = client.pages.archive_workspace_page if archive else client.pages.unarchive_workspace_page + mover(workspace_slug=workspace_slug, page_id=page_id) + # Plane answers nothing, and delete depends on this having happened. + return {"page_id": page_id, "archived": archive} + + if action in ("update", "delete"): + if not page_id: + return missing(action, "page_id") + scope = {"project_id": project_id} if project_id else {} + if action == "delete": + deleter = client.pages.delete_project_page if project_id else client.pages.delete_workspace_page + deleter(workspace_slug=workspace_slug, page_id=page_id, **scope) + return None + if not (name or description_html): + return missing(action, "name or description_html") + updater = client.pages.update_project_page if project_id else client.pages.update_workspace_page + return updater( + workspace_slug=workspace_slug, + page_id=page_id, + **scope, + data=UpdatePage(name=opt(name), description_html=opt(description_html)), + ) + if action == "create": if error := needs(action, name=name, description_html=description_html): return error + if parent_id and collection_id: + return "Error: pass parent_id or collection_id, not both. A nested page takes its parent's collection." + if collection_id and project_id: + return "Error: collections hold workspace pages only. Omit project_id, or omit collection_id." data = CreatePage( name=name, description_html=description_html, access=access, color=opt(color), is_locked=is_locked, + parent_id=opt(parent_id), + collection_id=opt(collection_id), external_id=opt(external_id), external_source=opt(external_source), ) @@ -122,6 +209,37 @@ def page( return client.pages.create_project_page(workspace_slug=workspace_slug, project_id=project_id, data=data) return client.pages.create_workspace_page(workspace_slug=workspace_slug, data=data) + if action == "set_collection": + if error := needs(action, page_id=page_id, collection_id=collection_id): + return error + + filed = client.pages.retrieve_workspace_page(workspace_slug=workspace_slug, page_id=page_id) + + if not filed.collection_id: + added = client.collections.pages.add( + workspace_slug=workspace_slug, + collection_id=collection_id, + data=AddCollectionPages(page_ids=[page_id]), + ) + if not added: + return None + membership_id = added[0].id + elif str(filed.collection_id) == collection_id: + membership_id = filed.page_collection_id + else: + membership_id = client.collections.pages.update( + workspace_slug=workspace_slug, + collection_id=str(filed.collection_id), + page_collection_id=str(filed.page_collection_id), + data=UpdateCollectionPage(collection=collection_id), + ).id + + return { + "page_id": page_id, + "collection_id": collection_id, + "page_collection_id": str(membership_id), + } + if error := needs(action, project_id=project_id, workitem_id=workitem_id): return error diff --git a/plane_mcp/tools/registry.py b/plane_mcp/tools/registry.py index e320247..aa96c62 100644 --- a/plane_mcp/tools/registry.py +++ b/plane_mcp/tools/registry.py @@ -17,6 +17,7 @@ from types import ModuleType from plane_mcp.tools import ( + collection, customer, customer_property, customer_request, @@ -35,6 +36,7 @@ release_label, release_tag, state, + template, work_log, workitem, workitem_activity, @@ -47,11 +49,6 @@ workspace, ) -# Order is load-bearing: tool definitions sit at the front of a client's prompt -# cache, so reordering them invalidates the whole conversation. Append new -# resources; do not re-sort. `test_resource_order_is_pinned` holds this to a -# literal list, so any change shows up as a diff rather than as a silent -# cache-buster. RESOURCES: tuple[ModuleType, ...] = ( customer, customer_property, @@ -81,6 +78,8 @@ workitem_relation, workitem_type, workspace, + template, + collection, ) diff --git a/plane_mcp/tools/template.py b/plane_mcp/tools/template.py new file mode 100644 index 0000000..5bbdeae --- /dev/null +++ b/plane_mcp/tools/template.py @@ -0,0 +1,207 @@ +"""Templates: the reusable shape of a work item, a page, or a project. + +`kind` says what is templated, project_id where it lives -- with it a project +template, without it a workspace one. A project kind exists only at workspace scope, +since a project template is what creates projects. `template_data` carries the body +being templated; everything else on the tool is the template's own metadata. + +Listing without project_id is the wider view, not the other half: for a workitem or +page kind it returns a project's templates too, marked by a non-null `project`. + +Governance refuses project-scoped writes where the workspace owns its templates. +Reads are never refused, so a project still lists what it already has. +""" + +from __future__ import annotations + +import json +from typing import Any, Literal + +from fastmcp import FastMCP +from plane.errors.errors import HttpError +from plane.models import project_templates as project_models +from plane.models import workspace_templates as workspace_models + +from plane_mcp.client import get_plane_client_context +from plane_mcp.toolkit import ( + Action, + build_annotations, + build_description, + missing, + needs, + one_of, + opt, + workspace_owns, +) + +NAME = "template" +TITLE = "Templates" + +KINDS = ("workitem", "page", "project") + +ACTIONS = ( + Action("list", ("kind",), ("project_id",), read=True), + Action("create", ("kind", "name", "template_data"), ("project_id", "description")), + Action( + "update", + ("kind", "template_id"), + ("project_id", "name", "description", "template_data"), + note="only the fields you pass are changed", + ), + Action("delete", ("kind", "template_id"), ("project_id",), destructive=True), +) + +FOOTER = ( + f"kind is one of: {', '.join(KINDS)}. project_id chooses the scope a write lands in; omit it " + "for the workspace, which is also the only scope a project kind can live at. " + "For a workitem or page kind, listing without project_id is the wider view: it returns a " + "project's templates as well as the workspace's own, told apart by a non-null project. " + "Pass project_id to list one project's alone. " + "template_data is a JSON object holding what gets templated -- for a work item template, " + 'the fields a work item created from it starts with, such as {"name": "Spec", ' + '"description_html": "

Context / Acceptance criteria

", "priority": "medium"}. ' + "update merges it into what is there rather than replacing it, so pass only what changes. " + "description is the template's own summary, not the templated body." +) + +# Templates are new to this surface, so no retired name maps onto them. +LEGACY: dict[str, str] = {} + +OWNED_BY_WORKSPACE = ( + "Error: this workspace owns its templates, so a project's own are read-only -- listing them " + "still works, but they cannot be created, changed or deleted. Omit project_id to write at " + "the workspace instead." +) + +_WORKSPACE = { + "workitem": (workspace_models.CreateWorkItemTemplate, workspace_models.UpdateWorkItemTemplate), + "page": (workspace_models.CreatePageTemplate, workspace_models.UpdatePageTemplate), + "project": (workspace_models.CreateProjectTemplate, workspace_models.UpdateProjectTemplate), +} + +_PROJECT = { + "workitem": (project_models.CreateWorkItemTemplate, project_models.UpdateWorkItemTemplate), + "page": (project_models.CreatePageTemplate, project_models.UpdatePageTemplate), +} + + +def _scope_of(client: Any, kind: str, project_id: str) -> tuple[Any, dict[str, Any], tuple[Any, Any]] | None: + """The namespace, scope kwargs and payload models that kind and project_id select. + + None when the pair has no endpoint, which is only a project template asked for + inside a project. + """ + if project_id: + if kind not in _PROJECT: + return None + namespace = { + "workitem": client.project_templates.work_item_templates, + "page": client.project_templates.page_templates, + }[kind] + return namespace, {"project_id": project_id}, _PROJECT[kind] + namespace = { + "workitem": client.workspace_templates.work_items, + "page": client.workspace_templates.pages, + "project": client.workspace_templates.projects, + }[kind] + return namespace, {}, _WORKSPACE[kind] + + +def _body(raw: str) -> dict[str, Any] | None: + """Parse template_data, raising ValueError with a correctable message.""" + if not raw: + return None + try: + parsed = json.loads(raw) + except ValueError as exc: + raise ValueError(f"template_data must be a JSON object; it is not valid JSON ({exc})") from exc + if not isinstance(parsed, dict): + raise ValueError(f"template_data must be a JSON object; got {type(parsed).__name__}") + return parsed + + +def _payload(model: Any, name: str, description: str, body: dict[str, Any] | None) -> Any: + """Build a create or update payload for whichever scope's model this is. + + The two scopes name the summary field differently -- `description_html` at the + workspace, `short_description` in a project -- so it is set by whichever the + model actually declares rather than by branching on scope again. + """ + fields: dict[str, Any] = {"name": opt(name), "template_data": body} + if description: + summary = "description_html" if "description_html" in model.model_fields else "short_description" + fields[summary] = description + return model(**{key: value for key, value in fields.items() if value is not None}) + + +def register(mcp: FastMCP) -> None: + @mcp.tool( + name=NAME, + description=build_description("Reusable templates for work items, pages and projects.", ACTIONS, FOOTER), + annotations=build_annotations(TITLE, ACTIONS), + ) + def template( + action: Literal["list", "create", "update", "delete"], + kind: str = "", + project_id: str = "", + template_id: str = "", + name: str = "", + description: str = "", + template_data: str = "", + ) -> Any: + client, workspace_slug = get_plane_client_context() + + if not kind: + return missing(action, "kind") + if error := one_of("kind", kind, KINDS): + return error + + scope = _scope_of(client, kind, project_id) + if scope is None: + return ( + "Error: a project template lives at the workspace, not inside a project. " + "Omit project_id to work with it." + ) + namespace, target, (create_model, update_model) = scope + + if action == "list": + return namespace.list(workspace_slug=workspace_slug, **target) + + try: + body = _body(template_data) + except ValueError as exc: + return f"Error: {exc}." + + try: + if action == "create": + if error := needs(action, name=name, template_data=template_data): + return error + return namespace.create( + workspace_slug=workspace_slug, + **target, + data=_payload(create_model, name, description, body), + ) + + if not template_id: + return missing(action, "template_id") + + if action == "update": + if body == {}: + return "Error: template_data must contain at least one field." + if not (name or description or body): + return missing(action, "name, description or template_data") + return namespace.update( + workspace_slug=workspace_slug, + **target, + template_id=template_id, + data=_payload(update_model, name, description, body), + ) + + namespace.delete(workspace_slug=workspace_slug, **target, template_id=template_id) + return None + except HttpError as exc: + # Only a project-scoped write draws this, so the answer is the same for + # all three: the workspace owns them, write there instead. + if workspace_owns(exc): + return OWNED_BY_WORKSPACE + raise diff --git a/pyproject.toml b/pyproject.toml index e5c59f0..9fee8cc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "plane-mcp-server" -version = "0.3.0" +version = "0.3.1" description = "A Model Context Protocol server for Plane integration" readme = "README.md" requires-python = ">=3.10" @@ -13,7 +13,7 @@ keywords = ["mcp", "plane", "fastmcp", "ai", "automation"] dependencies = [ "fastmcp==3.2.0", - "plane-sdk==0.2.20", + "plane-sdk==0.2.23", "py-key-value-aio[redis]>=0.4.4,<0.5.0", "mcp==1.26.0", "PyJWT>=2.12.0", diff --git a/tests/tools/test_conformance.py b/tests/tools/test_conformance.py index 5503eaf..9f0bd45 100644 --- a/tests/tools/test_conformance.py +++ b/tests/tools/test_conformance.py @@ -62,6 +62,9 @@ def _module_ids(mods): "workitem_relation", "workitem_type", "workspace", + # Appended, not sorted in: see registry.py. + "template", + "collection", ] @@ -193,9 +196,16 @@ def test_tool_names_leave_room_for_client_prefixes(listing): def test_listing_is_deterministic(registered): - """Tool definitions head a client's prompt cache; reordering invalidates it.""" - names = list(registered) - assert names == sorted(names) + """Tool definitions head a client's prompt cache; reordering invalidates it. + + Pinned to `CATALOGUE` rather than asserted `sorted()`. Alphabetical order was an + artefact of the `pkgutil` scan this package used to do over sorted filenames; kept + as an assertion afterwards, it forced every new resource into the middle of the + listing, shifting every tool below it and costing the cached prefix each time. + `test_resource_order_is_pinned` checks the same order at the module level, so a + mismatch between the two means registration dropped or reordered a resource. + """ + assert list(registered) == CATALOGUE def test_tool_count_is_within_client_caps(listing): diff --git a/tests/tools/test_dispatch.py b/tests/tools/test_dispatch.py index 008c4b8..f68c3ca 100644 --- a/tests/tools/test_dispatch.py +++ b/tests/tools/test_dispatch.py @@ -26,15 +26,22 @@ "group": "started", "relation_type": "blocked_by", "property_type": "TEXT", - "access": 1, + # page and project take the numeric level; collection names it. + "access": {"page": 1, "project": 1, "collection": "private"}, + "member_access": "edit", "network": 2, "timezone": "UTC", "workitem_identifier": "ENG-42", + "kind": "workitem", + "template_data": '{"name": "Spec"}', } # Actions that require *one of* several optional parameters -- a condition the # declaration cannot express, so the case is spelled out here. CONDITIONAL: dict[tuple[str, str], dict[str, object]] = { + # An update has to carry a field to change; page_id alone is refused. + ("page", "update"): {"name": "Renamed"}, + ("template", "update"): {"name": "Renamed"}, ("cycle", "manage_workitems"): {"add_ids": "id-1"}, ("module", "manage_workitems"): {"add_ids": "id-1"}, ("milestone", "manage_workitems"): {"add_ids": "id-1"}, diff --git a/tests/tools/test_governance.py b/tests/tools/test_governance.py index 3c59691..1e5521e 100644 --- a/tests/tools/test_governance.py +++ b/tests/tools/test_governance.py @@ -252,6 +252,7 @@ def test_the_feature_toggles_the_sdk_offers_are_all_reachable(): missing_flags = set(ProjectFeature.model_fields) - declared assert not missing_flags, f"ProjectFeature flags with no way to set them: {sorted(missing_flags)}" + PROPERTY_REFUSAL = HttpError( "Bad Request", status_code=400, response={"error": "This resource is managed at the workspace level"} ) diff --git a/uv.lock b/uv.lock index 4605575..1bec8a0 100644 --- a/uv.lock +++ b/uv.lock @@ -894,7 +894,7 @@ wheels = [ [[package]] name = "plane-mcp-server" -version = "0.3.0" +version = "0.3.1" source = { editable = "." } dependencies = [ { name = "authlib" }, @@ -920,7 +920,7 @@ requires-dist = [ { name = "fakeredis", extras = ["lua"], specifier = ">=2.32.1,<2.35.0" }, { name = "fastmcp", specifier = "==3.2.0" }, { name = "mcp", specifier = "==1.26.0" }, - { name = "plane-sdk", specifier = "==0.2.20" }, + { name = "plane-sdk", directory = "/Users/akhilvamshikonam/Documents/plane-python-sdk" }, { name = "py-key-value-aio", extras = ["redis"], specifier = ">=0.4.4,<0.5.0" }, { name = "pyjwt", specifier = ">=2.12.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=7.0.0" }, @@ -930,16 +930,21 @@ provides-extras = ["dev"] [[package]] name = "plane-sdk" -version = "0.2.20" -source = { registry = "https://pypi.org/simple" } +version = "0.2.23" +source = { directory = "/Users/akhilvamshikonam/Documents/plane-python-sdk" } dependencies = [ { name = "pydantic" }, { name = "requests" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/a1/d8/d9465cdf001aac5988cde173af812b298c85afb8550682fad11ba897112f/plane_sdk-0.2.20.tar.gz", hash = "sha256:d4559e00281be200e386bd322e53dbaafd9f1968bd528bb555f6eda2115518b4", size = 86662, upload-time = "2026-07-20T16:57:41.599Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/a7/8b/c2c52c8c632947e1e12c5612be31bd6180bb6c02046bceb66991f44545ab/plane_sdk-0.2.20-py3-none-any.whl", hash = "sha256:b0a55fec3140025761e8a6c3766b2eed9bea4828ae98c45e3df335cffd1df774", size = 123900, upload-time = "2026-07-20T16:57:40.32Z" }, + +[package.metadata] +requires-dist = [ + { name = "pydantic", specifier = ">=2.4.0" }, + { name = "pytest", marker = "extra == 'dev'" }, + { name = "pytest-dependency", marker = "extra == 'dev'" }, + { name = "requests", specifier = ">=2.31.0" }, ] +provides-extras = ["dev"] [[package]] name = "platformdirs" From d9e77cb7532db26d7ed264c0ac132c3674379cbb Mon Sep 17 00:00:00 2001 From: akhil-vamshi-konam Date: Mon, 17 Aug 2026 14:52:45 +0530 Subject: [PATCH 2/3] feat: support workspace-governed states --- CLAUDE.md | 2 +- plane_mcp/toolkit/README.md | 53 +++++++++ plane_mcp/toolkit/__init__.py | 4 + plane_mcp/toolkit/governance.py | 39 +++++++ plane_mcp/tools/README.md | 163 +++++---------------------- plane_mcp/tools/state.py | 153 ++++++++++++++++--------- plane_mcp/tools/workitem_property.py | 2 + plane_mcp/tools/workitem_type.py | 2 + tests/toolkit/test_governance.py | 72 ++++++++++++ tests/tools/test_conformance.py | 44 ++++++++ tests/tools/test_dispatch.py | 2 + tests/tools/test_governance.py | 72 ++++++++++++ 12 files changed, 420 insertions(+), 188 deletions(-) create mode 100644 plane_mcp/toolkit/README.md diff --git a/CLAUDE.md b/CLAUDE.md index 50a92b4..1481bca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,7 +106,7 @@ Shared building blocks for the tool surface, split by *when* they act: | `governance.py` | policy | `workspace_owns_resource`, `GOVERNED_BY`, `workspace_owns`, `migration_in_progress`, `plan_gated` | | `transforms.py` | listing | `StripOutputSchemas` | -Governance has two questions, and both matter. `workspace_owns_resource` reads the workspace flag that governs a resource — used *before* a write, to pick the scope. `workspace_owns` reads the refusal — used *after*, because the flag is cached and the lockout outlives it being toggled off. There is no single flag: work item types carry their own (`is_work_item_types_enabled`, public `work_item_types`), while states, labels, workflows, templates and automations share `workspace_governance_status` (public `states_owned_by_workspace`). `GOVERNED_BY` maps resource to flag so a newly governed resource is one row. +Governance, plan gates and the rest of the toolkit conventions are documented in `plane_mcp/toolkit/README.md`, including what adding a newly governed resource takes. Names are re-exported from `plane_mcp/toolkit/__init__.py`, so a resource module needs one import: `from plane_mcp.toolkit import Action, build_description, missing, opt`. diff --git a/plane_mcp/toolkit/README.md b/plane_mcp/toolkit/README.md new file mode 100644 index 0000000..805064b --- /dev/null +++ b/plane_mcp/toolkit/README.md @@ -0,0 +1,53 @@ +# toolkit + +Shared building blocks for the tool surface, split by *when* they act. All re-exported from `__init__.py`, so a resource module needs one import. + +| Module | Acts at | Provides | +|---|---|---| +| `spec.py` | declaration | `Action`, `build_description`, `build_annotations` | +| `runtime.py` | call | `missing`, `needs`, `require`, `one_of`, `opt`, `coerce_list`, `page_params`, `as_params`, `ids_of` | +| `paging.py` | response | `envelope`, `dump_results`, `pql_failure`, `workitem_page` | +| `governance.py` | policy | `workspace_owns_resource`, `workspace_owns`, `scoped`, `plan_gated` | +| `transforms.py` | listing | `StripOutputSchemas` | + +Nothing here knows which catalogue is calling it. Anything encoding this server's own history — the `RESOURCES` tuple, retired names — lives under `tools/`. + +## Workspace governance + +Plane can move a resource's catalogue from the project to the workspace. Which scope owns it decides where writes go, and the wrong scope is refused in **both** directions: + +| Write | Refused with | Means | +|---|---|---| +| project-scoped, workspace owns it | `workspace_managed` | omit `project_id` | +| catalogue, project still owns it | `workspace_not_managed` | pass `project_id` | + +**There is no single flag.** `GOVERNED_BY` maps each resource to the one that governs it: + +| Resource | Flag | +|---|---| +| work item types, epics, properties | `work_item_types` | +| states, labels, workflows, templates, automations | `states_owned_by_workspace` | + +A workspace can own one and not the other, so never read one flag for the other resource. + +**Two ways to ask, both needed:** + +| Call | Reads | Use | +|---|---|---| +| `workspace_owns_resource(client, slug, resource)` | the flag | pick a scope *before* writing | +| `workspace_owns(exc, *fields)` | the refusal | after — the flag is cached and the lockout outlives it being toggled off | + +### Adding a newly governed resource + +1. **Add a row to `GOVERNED_BY`** naming its flag. +2. **Decorate the tool with `@scoped("")`**, below `@mcp.tool`. Either refusal becomes a message naming the scope that owns it. Where the API refuses by field rather than code — work item types do — pass the field: `@scoped("work item types", "work_item_types")`. +3. **Resolve the scope once** at the top of the dispatch, not per call site. Shape is yours: `workitem_type` returns a tuple, `state` and `workitem_property` a small local `_Scope`. +4. **Refuse fields the other scope lacks.** A catalogue state has no ordering, triage flag or default; sent anyway they are dropped in silence, which reads as success. + +`workitem_type resolve` is the worked ask-first example: reads the flag, adopts the type from the catalogue, imports it into the project — and still handles the refusal in case the flag is stale. + +`test_a_wrong_scope_refusal_is_answered_not_raised` drives a real refusal through every scoped resource, so step 2 cannot be forgotten. + +## Plan gates + +`@plan_gated("")` turns a 402 — or a 400 whose prose says "upgrade your plan" — into a message naming the feature, rather than an error a caller will retry. The argument is the fallback label: where the refusal names the feature itself, that wins, since one resource can trip several gates (`project` trips five). diff --git a/plane_mcp/toolkit/__init__.py b/plane_mcp/toolkit/__init__.py index 15d287a..4d4a8d8 100644 --- a/plane_mcp/toolkit/__init__.py +++ b/plane_mcp/toolkit/__init__.py @@ -28,6 +28,8 @@ migration_in_progress, plan_gated, plan_required, + project_owns, + scoped, workspace_owns, workspace_owns_resource, ) @@ -71,8 +73,10 @@ "page_params", "plan_gated", "plan_required", + "project_owns", "pql_failure", "require", + "scoped", "workspace_owns", "workspace_owns_resource", "workitem_page", diff --git a/plane_mcp/toolkit/governance.py b/plane_mcp/toolkit/governance.py index ca907bc..ca1fdae 100644 --- a/plane_mcp/toolkit/governance.py +++ b/plane_mcp/toolkit/governance.py @@ -10,6 +10,8 @@ from plane.errors.errors import HttpError WORKSPACE_MANAGED = "workspace_managed" +# Its inverse: a catalogue endpoint called in a workspace that still owns per project. +WORKSPACE_NOT_MANAGED = "workspace_not_managed" MIGRATION_IN_PROGRESS = "governance_migration_in_progress" # The governed resources, named as this server refers to them. @@ -29,6 +31,8 @@ AUTOMATIONS: "states_owned_by_workspace", } +NOT_MANAGED_PROSE = re.compile(r"workspace work item type.*not enabled", re.IGNORECASE) + # Some validators raise 400 with the plan gate only in prose; matched as a fallback. PLAN_GATE_PROSE = "upgrade your plan" @@ -56,6 +60,41 @@ def workspace_owns_resource(client: Any, workspace_slug: str, resource: str) -> return bool(features.model_dump().get(flag)) +def project_owns(exc: HttpError) -> bool: + """Whether a refusal means this resource still lives per project.""" + body = _body(exc) + if body.get("code") == WORKSPACE_NOT_MANAGED: + return True + return bool(NOT_MANAGED_PROSE.search(str(body.get("error", "")))) + + +def wrong_scope(exc: HttpError, resource: str, *fields: str, project_id: str = "") -> str | None: + """The message telling a caller which scope owns `resource`, or None.""" + if workspace_owns(exc, *fields): + return f"Error: this workspace owns its {resource}. Omit project_id to work with the catalogue." + if not project_id and project_owns(exc): + return f"Error: this workspace keeps {resource} per project. Pass project_id." + return None + + +def scoped(resource: str, *fields: str) -> Callable[[F], F]: + """Turn a wrong-scope refusal anywhere in a resource's dispatch into that message.""" + + def decorate(fn: F) -> F: + @functools.wraps(fn) + def guarded(*args: Any, **kwargs: Any) -> Any: + try: + return fn(*args, **kwargs) + except HttpError as exc: + if message := wrong_scope(exc, resource, *fields, project_id=kwargs.get("project_id", "")): + return message + raise + + return guarded # type: ignore[return-value] + + return decorate + + def migration_in_progress(exc: HttpError) -> bool: """Whether a refusal means a governance migration is running, so the write may be retried.""" return _body(exc).get("code") == MIGRATION_IN_PROGRESS diff --git a/plane_mcp/tools/README.md b/plane_mcp/tools/README.md index 27e08e4..75c2dd4 100644 --- a/plane_mcp/tools/README.md +++ b/plane_mcp/tools/README.md @@ -1,7 +1,6 @@ # The tool surface -**30 tools**, one per Plane resource, each taking an `action` parameter that -selects the operation. 204 actions in total. +**30 tools**, one per Plane resource, each taking an `action` parameter that selects the operation. 204 actions in total. ```python workitem(action="create", project_id=..., name="Fix login") @@ -9,12 +8,9 @@ workitem(action="list", project_id=..., pql='state__group = "started"') cycle(action="archive", project_id=..., cycle_id=...) ``` -A compact catalogue — 30 tools, ~67k characters — loads fully in every MCP client -and leaves the context budget to the conversation. +A compact catalogue — 30 tools, ~67k characters — loads fully in every MCP client and leaves the context budget to the conversation. -## The shape of a resource module - -One module per resource, five parts, in this order: +Five parts, in this order: ```python NAME = "label" # 1. identity @@ -27,7 +23,6 @@ ACTIONS = ( # 2. the declaration -- single source of tr ) FOOTER = "color is a hex code such as #EF4444." # 3. cross-cutting notes - LEGACY = {"list_labels": "list", ...} # 4. retired name -> action def register(mcp): # 5. dispatch @@ -40,76 +35,21 @@ def register(mcp): # 5. dispatch ... ``` -`ACTIONS` generates the description and the MCP annotations, so documentation -cannot drift from behaviour. `Action(read=True)` and `Action(destructive=True)` -become `readOnlyHint` and `destructiveHint`. - -Register a new resource by adding the module and one entry in `registry.py`. +`ACTIONS` generates the description, the MCP annotations *and* argument validation, so documentation cannot drift from behaviour. Add the module, add one entry to `registry.py`, done. ## Conventions -**Parameters are plain typed defaults** — `= ""`, `= 0`, `= False`. Never -`X | None = None`: Pydantic renders every optional union as a verbose -`anyOf`-with-null block. Where `False` or `0` is a *meaningful value* distinct -from "not supplied" — a visibility of `0`, an intake status of `-2` — use -`bool | None` or `int | None` and say why in a comment. - -**Validate enum-valued parameters.** They are `str` in the schema, so check them -in the dispatch with `one_of()`. An unrecognised value then returns an error -naming the permitted set, rather than being dropped from the payload — dropping it -writes the record without the field and reports success. - -**Declare every parameter an action takes, and only those.** The declaration is not -just documentation: `ValidateActionArguments` checks each call against it, so an -argument belonging to a different action is refused instead of silently dropped. One -flat schema per tool cannot catch that — `query` is a real `workitem` parameter, just -not one `count` has any use for, and a `count` call carrying it validated cleanly and -then answered a different question. An argument left at its default is ignored, since -some clients pad a request with every parameter. Retired names are exempt: they arrive -with no `action` and under their own spelling. - -**Guard a plan-gated resource** with `@plan_gated("")` below `@mcp.tool`, so a -402 becomes a message naming the feature rather than an error worth retrying. The -argument is only the fallback label: where the refusal names the feature itself -("Upgrade your plan to enable Epics"), that wins — one resource can trip several -gates, and `project` trips five. - -**A guard names only what is absent.** Guard order is shared-prefix first: what -every action needs, then what one action needs. For a guard covering more than -one parameter use `needs()` rather than a shared condition: - -```python -if error := needs(action, name=name, owned_by=owned_by): # names owned_by only - return error - -if not name or not owned_by: # blames both - return missing(action, "name", "owned_by") -``` - -The error string is the model's self-correction channel: naming exactly what is -absent lets it retry correctly on the next call. - -**An action that accepts a `cursor` must return one.** Return `envelope(response)` -rather than `response.results`, so the caller receives `next_cursor` and can page -through the full set. - -**Match the SDK's `params` type.** Some endpoints take `Mapping[str, Any]` -(`page_params`), others a Pydantic query-params model (`as_params`) and call -`.model_dump()` on it. A dict passed to the second kind raises `AttributeError` -at call time. - -**Declare the type you mean; encoding is handled upstream.** A client that sends -`'["uuid"]'` for an array parameter is repaired by `CoerceArguments` middleware -before validation, driven by the schema alone. So type a list parameter as a list -and a number as a number — there is no need to widen it to `str` to survive a -client that stringifies. `coerce_list` remains for parameters genuinely declared -`str`, such as `add_ids`, where a comma is a separator. - -**The surface spells it `workitem`; `plane-sdk` spells it `work_item`.** Tool -names, action names and parameters use `workitem`. SDK namespaces, keyword -arguments and model names keep `work_item`, as do Plane's PQL field names -(`work_items__release_id`). `test_vocabulary.py` pins the boundary in both -directions. +| Rule | Why | +|---|---| +| Parameters are plain typed defaults (`= ""`, `= 0`) | `X \| None` renders a verbose `anyOf`-with-null block. Use `bool \| None` only where `False`/`0` is a real value distinct from unset — and say so in a comment | +| Validate enums with `one_of()` | They are `str` in the schema; an unchecked value is dropped from the payload and the write reports success | +| Declare every parameter an action takes, and only those | `ValidateActionArguments` checks calls against it. `query` is a real `workitem` parameter but useless to `count` — sent there it used to validate cleanly and answer a different question | +| Use `needs()` for multi-parameter guards | It names only what is absent. `if not a or not b` blames both, so a caller that supplied `a` is told to send it again | +| An action accepting `cursor` must return `envelope(response)` | Returning `response.results` lets a caller page in but never page on | +| Match the SDK's `params` type: `page_params` vs `as_params` | Some endpoints take a `Mapping`, others a Pydantic model they call `.model_dump()` on. The wrong one raises at call time | +| Declare the type you mean | `CoerceArguments` repairs `'["uuid"]'` before validation, so type a list as a list. `coerce_list` is for parameters genuinely declared `str`, where a comma separates | +| `@plan_gated("")` on a plan-gated resource | Turns a 402 into a message naming the feature. The argument is a fallback — where the refusal names the feature, that wins | +| The surface spells it `workitem`; `plane-sdk` spells it `work_item` | Tool names, actions and parameters use `workitem`; SDK namespaces, kwargs and PQL fields keep `work_item`. `test_vocabulary.py` pins both directions | ## Layout @@ -118,77 +58,34 @@ directions. | `.py` | one module per resource | | `registry.py` | `RESOURCES` in advertised order, plus the alias tables | | `legacy.py` | `LegacyNames` — resolves retired tool names | -| `../../toolkit/` | shared helpers: `spec`, `runtime`, `paging`, `governance`, `transforms` | +| `../toolkit/` | shared helpers — see [`../toolkit/README.md`](../toolkit/README.md) | -`RESOURCES` is an explicit tuple, not a directory scan. Its order is the -advertised order and therefore a wire-format guarantee: tool definitions head a -client's prompt cache, so reordering invalidates live conversations. Append; -never re-sort. `test_resource_order_is_pinned` holds it to a literal list. +`RESOURCES` is an explicit tuple, not a directory scan. Its order is the advertised order and therefore a wire-format guarantee: tool definitions head a client's prompt cache, so reordering invalidates live conversations. `test_resource_order_is_pinned` holds it to a literal list. ## Listing transforms -Two `Transform`s wrap the registered tools. Both implement `list_tools`/`get_tool` -only, so execution keeps the full schema and results — including -`structuredContent` — are unchanged. +Both implement `list_tools`/`get_tool` only, so execution keeps the full schema and results are unchanged. -- **`StripOutputSchemas`** (`toolkit/transforms.py`) drops `outputSchema` from the - listing: roughly two thirds of the wire payload, for a field the MCP spec - defines as a client-side validation contract and no client forwards to a model. -- **`LegacyNames`** (`legacy.py`) resolves a retired tool name to its - `(tool, action)` pair on lookup, with `action` hidden and pre-filled. +| Transform | Effect | +|---|---| +| `StripOutputSchemas` | Drops `outputSchema` from the listing — roughly two thirds of the wire payload, for a field no client forwards to a model | +| `LegacyNames` | Resolves a retired tool name to its `(tool, action)` pair, with `action` hidden and pre-filled | ## Retired tool names -Before consolidation this server exposed 177 tools, one per API operation. 169 of -those names still resolve. They are not advertised, so they cost nothing in the -listing, but a saved prompt or script calling `create_work_item` keeps working — -including the parameter names it shipped with (`work_item_id`, not `workitem_id`). -Each resolution is logged, so the set of remaining callers is an observation -rather than a guess. +Before consolidation this server exposed 177 tools, one per API operation. **169 still resolve**, unadvertised — so they cost nothing in the listing, but a saved prompt calling `create_work_item` keeps working, including the parameter names it shipped with (`work_item_id`, not `workitem_id`). Every resolution is logged, so the remaining callers are an observation rather than a guess. -`tests/tools/_retired_names.py` is the frozen record of all 177, and the -conformance suite asserts every one is aliased, declared unmappable, or still -registered under the same name. +**7 cannot be mapped.** An alias renames a tool; it cannot reshape one, and these chose between two operations with a parameter (`manage_project_archive(archive=False)`). Each is declared in its module's `LEGACY_UNMAPPED` with the replacement to use. -An alias renames a tool; it cannot reshape one. Seven names chose between two -operations with a parameter (`manage_project_archive(archive=False)`, -`manage_release_labels(action="detach")`) and no single `(tool, action)` pair -reproduces that. Each is declared in its module's `LEGACY_UNMAPPED` with the -replacement to use, and the conformance suite holds that list to a budget. +`tests/tools/_retired_names.py` is the frozen record of all 177; the conformance suite asserts every one is aliased, declared unmappable, or still registered. ## Scope: project vs workspace -Plane governs some resources at the workspace as well as the project — the same -resource under two SDK namespaces, with different id keyword names. The idiom a -model sees is uniform: **supply `project_id` for the project's own set, omit it -for the workspace's.** - -Getting it wrong is quiet — the call succeeds against the wrong scope — so each -resource resolves scope once in a local `_scope_of`, and `test_governance.py` -pins both namespaces and both id keywords against the live SDK. - -Each resource resolves scope locally rather than through a shared abstraction, -because the shapes differ: `workitem_type` is a two-way split, `workitem_property` -is three-way and also varies the method name. Keep new ones local until a common -shape is established by more than one caller. - -**When the workspace owns the resource outright**, the project-scoped write is -refused. Two helpers, used together: - -- `workspace_owns_resource(client, slug, resource)` reads the workspace flag that - governs `resource`, so a caller can take the workspace path instead of provoking a - refusal it already knows is coming. There is no single governance flag: work item - types carry their own, while everything the governance migration moved (states, - labels, workflows, templates, automations) shares `states_owned_by_workspace`. A - workspace can own one and not the other, so `GOVERNED_BY` maps each resource to - the flag that actually governs it — a newly governed resource adds one row. -- `workspace_owns(exc, field)` reads the refusal, which is what settles it: the flag - is cached and the lockout outlives it being toggled off, so a write can still be - refused after the flag reads false. It handles both shapes Plane uses. - -`workitem_type resolve` is the worked example: ask who owns types, adopt from the -workspace catalogue and import if it does, otherwise create in the project — and -adopt anyway if the project write is refused. +Plane governs some resources at the workspace as well as the project. The idiom a model sees is uniform: **supply `project_id` for the project's own set, omit it for the workspace's.** + +Getting it wrong is quiet — the call succeeds against the wrong scope — so each resource resolves scope once, at the top of its dispatch, and `test_governance.py` pins the namespaces and id keywords against the live SDK. How it resolves is the resource's own business: `workitem_type` returns a tuple, `state` and `workitem_property` a small local `_Scope`, because what differs between their scopes differs. + +Where the workspace owns a resource outright, both directions of wrong-scope write are refused. `@scoped("")` turns either into a message naming the scope that owns it — see [`../toolkit/README.md`](../toolkit/README.md). ## Tools diff --git a/plane_mcp/tools/state.py b/plane_mcp/tools/state.py index 3a4c54c..9b38cae 100644 --- a/plane_mcp/tools/state.py +++ b/plane_mcp/tools/state.py @@ -1,39 +1,69 @@ -"""Workflow states within a project.""" +"""Workflow states, at project or workspace scope.""" from __future__ import annotations +from dataclasses import dataclass from typing import Any, Literal, get_args from fastmcp import FastMCP -from plane.models.enums import GroupEnum -from plane.models.states import CreateState, PaginatedStateResponse, State, UpdateState +from plane.models.enums import CatalogGroupEnum, GroupEnum +from plane.models.states import ( + CreateState, + CreateWorkspaceState, + PaginatedStateResponse, + State, + UpdateState, + UpdateWorkspaceState, +) from plane_mcp.client import get_plane_client_context -from plane_mcp.toolkit import Action, build_annotations, build_description, envelope, missing, needs, opt, page_params +from plane_mcp.toolkit import ( + Action, + build_annotations, + build_description, + envelope, + missing, + needs, + one_of, + opt, + page_params, + scoped, +) NAME = "state" TITLE = "Workflow states" -GROUPS = get_args(GroupEnum) +TRIAGE = "triage" +SETTABLE_GROUPS = tuple(group for group in get_args(GroupEnum) if group != TRIAGE) +assert SETTABLE_GROUPS == get_args(CatalogGroupEnum) ACTIONS = ( - Action("list", ("project_id",), ("cursor", "per_page"), read=True), - Action("retrieve", ("project_id", "state_id"), read=True), + Action( + "list", (), ("project_id", "cursor", "per_page"), note="workspace scope when project_id is omitted", read=True + ), + Action("retrieve", ("state_id",), ("project_id",), read=True), Action( "create", - ("project_id", "name", "color"), - ("description", "sequence", "group", "is_triage", "default", "external_source", "external_id"), + ("name", "color"), + ("project_id", "description", "sequence", "group", "default", "external_source", "external_id"), + note="group is required at workspace scope", ), Action( "update", - ("project_id", "state_id"), - ("name", "color", "description", "sequence", "group", "is_triage", "default"), + ("state_id",), + ("project_id", "name", "color", "description", "sequence", "group", "default"), note="only the fields you pass are changed", ), - Action("delete", ("project_id", "state_id"), destructive=True), + Action("delete", ("state_id",), ("project_id",), destructive=True), ) -FOOTER = f"group is one of: {', '.join(GROUPS)}. color is a hex code such as #EF4444." +FOOTER = ( + f"group is one of: {', '.join(SETTABLE_GROUPS)}. color is a hex code such as #EF4444. " + "A project also has a triage state, but Plane owns it: it cannot be created here and is " + "not listed, and Triage is a reserved name. " + "Omit project_id to work with the workspace catalogue, which is where states live once the " + "workspace owns them; sequence and default apply to a project's states only." +) LEGACY = { "list_states": "list", @@ -44,9 +74,31 @@ } -def _group(value: str) -> GroupEnum | None: - """Accept only a known group; anything else is dropped rather than sent.""" - return value if value in GROUPS else None # type: ignore[return-value] +@dataclass(frozen=True, slots=True) +class _Scope: + """Where a state lives, resolved once from project_id.""" + + namespace: Any + kwargs: dict[str, Any] + create: type + update: type + + +def _scope_of(client: Any, project_id: str) -> _Scope: + """project_id selects the project's states; without it, the workspace catalogue.""" + if project_id: + return _Scope( + namespace=client.states, + kwargs={"project_id": project_id}, + create=CreateState, + update=UpdateState, + ) + return _Scope( + namespace=client.workspace_states, + kwargs={}, + create=CreateWorkspaceState, + update=UpdateWorkspaceState, + ) def register(mcp: FastMCP) -> None: @@ -55,6 +107,7 @@ def register(mcp: FastMCP) -> None: description=build_description("Workflow states within a project.", ACTIONS, FOOTER), annotations=build_annotations(TITLE, ACTIONS), ) + @scoped("states") def state( action: Literal["list", "retrieve", "create", "update", "delete"], project_id: str = "", @@ -66,7 +119,6 @@ def state( sequence: float | None = None, group: str = "", # Tri-state: False is a meaningful value distinct from "not supplied". - is_triage: bool | None = None, default: bool | None = None, external_source: str = "", external_id: str = "", @@ -74,60 +126,53 @@ def state( per_page: int = 0, ) -> State | dict[str, Any] | str | None: client, workspace_slug = get_plane_client_context() + scope = _scope_of(client, project_id) if not project_id: - return missing(action, "project_id") + elsewhere = [field for field, value in (("sequence", sequence), ("default", default)) if value is not None] + if elsewhere: + return f"Error: {', '.join(elsewhere)} apply to a project's states only, not the workspace catalogue." + if error := one_of("group", group, SETTABLE_GROUPS): + return error + + def payload(model: type) -> Any: + """The fields this scope's model actually declares, minus the unset ones.""" + fields = { + "name": opt(name), + "color": opt(color), + "group": opt(group), + "description": opt(description), + "sequence": sequence, + "default": default, + "external_source": opt(external_source), + "external_id": opt(external_id), + } + return model(**{k: v for k, v in fields.items() if v is not None and k in model.model_fields}) if action == "list": - response: PaginatedStateResponse = client.states.list( - workspace_slug=workspace_slug, - project_id=project_id, - params=page_params(cursor, per_page), + response: PaginatedStateResponse = scope.namespace.list( + workspace_slug=workspace_slug, **scope.kwargs, params=page_params(cursor, per_page) ) return envelope(response) if action == "create": if error := needs(action, name=name, color=color): return error - return client.states.create( - workspace_slug=workspace_slug, - project_id=project_id, - data=CreateState( - name=name, - color=color, - description=opt(description), - sequence=sequence, - group=_group(group), - is_triage=is_triage, - default=default, - external_source=opt(external_source), - external_id=opt(external_id), - ), - ) + if not project_id and not group: + # The catalogue endpoint requires a group; the project one defaults it. + return missing(action, "group") + return scope.namespace.create(workspace_slug=workspace_slug, **scope.kwargs, data=payload(scope.create)) if not state_id: return missing(action, "state_id") if action == "retrieve": - return client.states.retrieve(workspace_slug=workspace_slug, project_id=project_id, state_id=state_id) + return scope.namespace.retrieve(workspace_slug=workspace_slug, **scope.kwargs, state_id=state_id) if action == "update": - return client.states.update( - workspace_slug=workspace_slug, - project_id=project_id, - state_id=state_id, - data=UpdateState( - name=opt(name), - color=opt(color), - description=opt(description), - sequence=sequence, - group=_group(group), - is_triage=is_triage, - default=default, - external_source=opt(external_source), - external_id=opt(external_id), - ), + return scope.namespace.update( + workspace_slug=workspace_slug, **scope.kwargs, state_id=state_id, data=payload(scope.update) ) - client.states.delete(workspace_slug=workspace_slug, project_id=project_id, state_id=state_id) + scope.namespace.delete(workspace_slug=workspace_slug, **scope.kwargs, state_id=state_id) return None diff --git a/plane_mcp/tools/workitem_property.py b/plane_mcp/tools/workitem_property.py index 599e817..19c98d6 100644 --- a/plane_mcp/tools/workitem_property.py +++ b/plane_mcp/tools/workitem_property.py @@ -47,6 +47,7 @@ opt, page_params, plan_gated, + scoped, workspace_owns, ) @@ -319,6 +320,7 @@ def register(mcp: FastMCP) -> None: annotations=build_annotations(TITLE, ACTIONS), ) @plan_gated("Work item properties") + @scoped("work item properties") def workitem_property( action: Literal[ "list", diff --git a/plane_mcp/tools/workitem_type.py b/plane_mcp/tools/workitem_type.py index af292a4..8c15c8a 100644 --- a/plane_mcp/tools/workitem_type.py +++ b/plane_mcp/tools/workitem_type.py @@ -26,6 +26,7 @@ opt, page_params, plan_gated, + scoped, workspace_owns, workspace_owns_resource, ) @@ -143,6 +144,7 @@ def register(mcp: FastMCP) -> None: annotations=build_annotations(TITLE, ACTIONS), ) @plan_gated("Work item types") + @scoped("work item types", PROJECT_TYPES_FEATURE) def workitem_type( action: Literal["list", "retrieve", "resolve", "create", "update", "delete", "import_to_project"], project_id: str = "", diff --git a/tests/toolkit/test_governance.py b/tests/toolkit/test_governance.py index df99e05..cd802ba 100644 --- a/tests/toolkit/test_governance.py +++ b/tests/toolkit/test_governance.py @@ -18,10 +18,13 @@ WORK_ITEM_TYPES, WORKFLOWS, WORKSPACE_MANAGED, + WORKSPACE_NOT_MANAGED, migration_in_progress, plan_required, + scoped, workspace_owns, workspace_owns_resource, + wrong_scope, ) @@ -215,3 +218,72 @@ def test_a_governance_refusal_is_not_reported_as_a_plan_gate(body): """ assert plan_required(_error(400, body), "This project feature") is None assert workspace_owns(_error(400, body), *body) + + +# Plane refuses a write to the wrong scope in both directions, with a matched pair of +# codes. A resource that later moves to the workspace catalogue -- labels, templates, +# automations -- should need nothing here beyond its noun. + +WRONG_SCOPE = [ + (WORKSPACE_MANAGED, "owns its states", "Omit project_id"), + (WORKSPACE_NOT_MANAGED, "keeps states per project", "Pass project_id"), +] + + +@pytest.mark.parametrize(("code", "says", "tells"), WRONG_SCOPE) +def test_each_refusal_names_the_scope_that_owns_the_resource(code, says, tells): + message = wrong_scope(_error(400, {"code": code}), "states") + assert message and says in message + assert tells in message, "named the problem without naming the fix" + + +def test_the_two_directions_do_not_give_the_same_advice(): + """Telling a governed workspace to pass project_id would loop the caller.""" + governed = wrong_scope(_error(400, {"code": WORKSPACE_MANAGED}), "states") + ungoverned = wrong_scope(_error(400, {"code": WORKSPACE_NOT_MANAGED}), "states") + + assert "Omit project_id" in governed and "Pass project_id" not in governed + assert "Pass project_id" in ungoverned and "Omit project_id" not in ungoverned + + +def test_the_field_keyed_refusal_is_still_recognised(): + """work_item_types answers with a field rather than a code; both reach the message.""" + refusal = _error(400, {"work_item_types": ["Cannot enable project-level work item types"]}) + assert wrong_scope(refusal, "work item types", "work_item_types") + + +@pytest.mark.parametrize( + "exc", + [_error(400, {"detail": "bad request"}), _error(403, {"code": WORKSPACE_MANAGED}), _error(500, {})], +) +def test_anything_else_is_not_a_scope_refusal(exc): + assert wrong_scope(exc, "states") is None + + +def test_a_new_governed_resource_needs_only_its_noun(): + """The codes are generic, so labels moving to the workspace is a one-word change.""" + for noun in ("labels", "templates", "automations"): + message = wrong_scope(_error(400, {"code": WORKSPACE_NOT_MANAGED}), noun) + assert message and noun in message + + +def test_the_decorator_answers_a_scope_refusal_and_re_raises_everything_else(): + @scoped("states") + def dispatch(code: str, status: int = 400): + raise HttpError("refused", status_code=status, response={"code": code}) + + assert "Omit project_id" in dispatch(WORKSPACE_MANAGED) + assert "Pass project_id" in dispatch(WORKSPACE_NOT_MANAGED) + with pytest.raises(HttpError): + dispatch(WORKSPACE_MANAGED, status=403) + + +def test_the_scoped_decorator_keeps_the_signature_fastmcp_reads(): + import inspect + + @scoped("states") + def original(action: str, project_id: str = "") -> str: + return "ok" + + assert list(inspect.signature(original).parameters) == ["action", "project_id"] + assert original("list") == "ok" diff --git a/tests/tools/test_conformance.py b/tests/tools/test_conformance.py index 9f0bd45..d6a3eac 100644 --- a/tests/tools/test_conformance.py +++ b/tests/tools/test_conformance.py @@ -12,6 +12,7 @@ import pytest +from plane_mcp.toolkit.governance import WORKSPACE_MANAGED from plane_mcp.toolkit.spec import action_names # Ratchets. Lowering these is routine; raising one is a design decision. @@ -261,3 +262,46 @@ def test_unmapped_stays_small(unmapped): def test_aliases_do_not_shadow_a_resource_tool(aliases, registered): assert not set(aliases) & set(registered) + + +# A resource whose scope can be refused must say which noun it is refusing, or the +# caller gets a raw 400 and no idea which scope to use. Structure is each resource's +# own business; this is about what a caller sees. + +SCOPED_WRITES = [ + ( + "state", + "states.create", + {"action": "create", "project_id": "p", "name": "S", "color": "#fff"}, + {"code": WORKSPACE_MANAGED}, + ), + ( + "workitem_type", + "work_item_types.create", + {"action": "create", "project_id": "p", "name": "T"}, + {"code": WORKSPACE_MANAGED}, + ), + ( + "workitem_property", + "workspace_work_item_properties.create", + {"action": "create", "workitem_type_id": "t", "display_name": "P", "property_type": "TEXT"}, + {"error": "Workspace work item types are not enabled"}, + ), +] + + +@pytest.mark.parametrize( + ("name", "method", "arguments", "refusal"), SCOPED_WRITES, ids=[case[0] for case in SCOPED_WRITES] +) +def test_a_wrong_scope_refusal_is_answered_not_raised(name, method, arguments, refusal, registered, spy): + """Without `@scoped` the caller gets a raw 400 and no idea which scope to use.""" + from plane.errors.errors import HttpError + + spy.returns[method] = HttpError("refused", status_code=400, response=refusal) + + result = registered[name].fn(**arguments) + + assert isinstance(result, str) and result.startswith("Error:"), ( + f"{name} let a wrong-scope refusal out raw; it needs @scoped" + ) + assert "project_id" in result, "named the problem without naming the fix" diff --git a/tests/tools/test_dispatch.py b/tests/tools/test_dispatch.py index f68c3ca..48ff6cc 100644 --- a/tests/tools/test_dispatch.py +++ b/tests/tools/test_dispatch.py @@ -41,6 +41,8 @@ CONDITIONAL: dict[tuple[str, str], dict[str, object]] = { # An update has to carry a field to change; page_id alone is refused. ("page", "update"): {"name": "Renamed"}, + # Without project_id this is the workspace catalogue, which requires a group. + ("state", "create"): {"group": "started"}, ("template", "update"): {"name": "Renamed"}, ("cycle", "manage_workitems"): {"add_ids": "id-1"}, ("module", "manage_workitems"): {"add_ids": "id-1"}, diff --git a/tests/tools/test_governance.py b/tests/tools/test_governance.py index 1e5521e..427e601 100644 --- a/tests/tools/test_governance.py +++ b/tests/tools/test_governance.py @@ -358,3 +358,75 @@ def test_an_unrelated_failure_on_a_property_write_still_surfaces(registered, spy display_name="Root cause", property_type="TEXT", ) + + +STATE_SCOPES = [ + ("list", {}, "states.list", "workspace_states.list"), + ("retrieve", {"state_id": "s-1"}, "states.retrieve", "workspace_states.retrieve"), + ("delete", {"state_id": "s-1"}, "states.delete", "workspace_states.delete"), +] + + +@pytest.mark.parametrize(("action", "extra", "project_method", "workspace_method"), STATE_SCOPES) +def test_project_id_selects_the_state_scope(registered, spy, action, extra, project_method, workspace_method): + tool = registered["state"].fn + + tool(action=action, project_id=PROJECT, **extra) + assert spy.recorder.only().method == project_method + + spy.recorder.calls.clear() + tool(action=action, **extra) + catalogue = spy.recorder.only() + assert catalogue.method == workspace_method + assert "project_id" not in catalogue.kwargs + + +def test_creating_a_catalogue_state_uses_the_workspace_endpoint(registered, spy): + registered["state"].fn(action="create", name="Blocked", color="#EF4444", group="started") + + call = spy.recorder.only() + assert call.method == "workspace_states.create" + assert "project_id" not in call.kwargs + + +def test_a_catalogue_state_needs_a_group(registered, spy): + """The project endpoint defaults it; the catalogue one requires it.""" + result = registered["state"].fn(action="create", name="Blocked", color="#EF4444") + + assert isinstance(result, str) and "group" in result + assert not spy.recorder.calls, "asked Plane for something it would refuse" + + +@pytest.mark.parametrize("scope", [{}, {"project_id": PROJECT}], ids=["catalogue", "project"]) +def test_triage_is_not_a_settable_group_at_either_scope(registered, spy, scope): + """Plane creates the triage state itself and both serializers refuse `group=triage`.""" + result = registered["state"].fn(action="create", name="Blocked", color="#EF4444", group="triage", **scope) + + assert isinstance(result, str) and "backlog" in result + assert "triage" not in result.removeprefix("Error: group must be one of: "), "offered triage back" + assert not spy.recorder.calls, "asked Plane for something it would refuse" + + +def test_the_triage_flag_is_not_settable(registered): + """`is_triage=True` used to be accepted, and Plane created a state that no endpoint + could reach again: both the list and the delete queryset filter `is_triage=False`.""" + assert "is_triage" not in registered["state"].parameters["properties"] + + +@pytest.mark.parametrize("field", ["sequence", "default"]) +def test_project_only_fields_are_refused_at_workspace_scope(registered, spy, field): + """A catalogue state has no ordering or default -- those live on a workflow, and + sending them would be dropped without a word.""" + value = 1.0 if field == "sequence" else True + result = registered["state"].fn(action="create", name="Blocked", color="#EF4444", group="started", **{field: value}) + + assert isinstance(result, str) and field in result + assert not spy.recorder.calls + + +def test_those_fields_still_work_on_a_project_state(registered, spy): + registered["state"].fn( + action="create", project_id=PROJECT, name="Blocked", color="#EF4444", group="started", default=True + ) + + assert spy.recorder.only().kwargs["data"].default is True From 9fd2c0887fa04ecf77c15698369b6be83b856152 Mon Sep 17 00:00:00 2001 From: Akhil Vamshi Konam Date: Mon, 17 Aug 2026 16:39:31 +0530 Subject: [PATCH 3/3] fix: update logging configuration and improve error handling in collection updates (#206) --- README.md | 2 +- plane_mcp/tools/README.md | 2 +- plane_mcp/tools/collection.py | 2 ++ tests/tools/test_dispatch.py | 1 + 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d07ff43..bfcc67d 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ Structured JSON. Each tool call logs its name, duration, status and — when available — an opaque user id and the workspace slug. ```bash -export LOG_USER_INFO=true # also log the display name (PII); +export LOG_USER_INFO=false # also log the display name (PII); export LOG_PAYLOADS=false # keep request payloads out of logs; default true ``` diff --git a/plane_mcp/tools/README.md b/plane_mcp/tools/README.md index 75c2dd4..4fb71fc 100644 --- a/plane_mcp/tools/README.md +++ b/plane_mcp/tools/README.md @@ -68,7 +68,7 @@ Both implement `list_tools`/`get_tool` only, so execution keeps the full schema | Transform | Effect | |---|---| -| `StripOutputSchemas` | Drops `outputSchema` from the listing — roughly two thirds of the wire payload, for a field no client forwards to a model | +| `StripOutputSchemas` | Drops `outputSchema` from the listing — roughly two-thirds of the wire payload, for a field no client forwards to a model | | `LegacyNames` | Resolves a retired tool name to its `(tool, action)` pair, with `action` hidden and pre-filled | ## Retired tool names diff --git a/plane_mcp/tools/collection.py b/plane_mcp/tools/collection.py index 1c71b7d..4c8d626 100644 --- a/plane_mcp/tools/collection.py +++ b/plane_mcp/tools/collection.py @@ -165,6 +165,8 @@ def collection( return collections.retrieve(workspace_slug=workspace_slug, collection_id=collection_id) if action == "update": + if not name and sort_order is None: + return missing(action, "name or sort_order") return collections.update( workspace_slug=workspace_slug, collection_id=collection_id, diff --git a/tests/tools/test_dispatch.py b/tests/tools/test_dispatch.py index 48ff6cc..93af333 100644 --- a/tests/tools/test_dispatch.py +++ b/tests/tools/test_dispatch.py @@ -44,6 +44,7 @@ # Without project_id this is the workspace catalogue, which requires a group. ("state", "create"): {"group": "started"}, ("template", "update"): {"name": "Renamed"}, + ("collection", "update"): {"name": "Renamed"}, ("cycle", "manage_workitems"): {"add_ids": "id-1"}, ("module", "manage_workitems"): {"add_ids": "id-1"}, ("milestone", "manage_workitems"): {"add_ids": "id-1"},