From 08d8436effb270fd409e94d788fb22f82046a9a8 Mon Sep 17 00:00:00 2001 From: Semih702 Date: Sun, 16 Aug 2026 17:51:00 +0300 Subject: [PATCH] fix: refuse an assignee Plane will not accept, rather than lose the field Plane filters an assignee id it will not accept out of the payload during validation, and an update deletes the work item's existing assignees before writing what is left. So a write naming one both fails to apply and clears the field, under a 200 -- the caller cannot tell it from a success. Three ways to land there, none visible in the response: the user is a workspace member but not a member of this project, their project role is guest, or the membership is inactive. _unassignable() reads the project's members and names the rejected ids before the write, for create, update and manage_assignee. An unreadable or empty member list skips the check rather than blocking a write that works today, and Community Edition's members carry no role, so an absent one cannot disqualify. --- plane_mcp/tools/workitem.py | 41 +++++++++++++++ tests/test_coercion.py | 8 ++- tests/tools/test_dispatch.py | 96 ++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 1 deletion(-) diff --git a/plane_mcp/tools/workitem.py b/plane_mcp/tools/workitem.py index 268332e..91fb4c2 100644 --- a/plane_mcp/tools/workitem.py +++ b/plane_mcp/tools/workitem.py @@ -51,6 +51,9 @@ PRIORITIES = get_args(PriorityEnum) +# Plane's floor for an assignable project member; guest sits below it. +_MEMBER_ROLE = 15 + WRITE_FIELDS = ( "name", "assignees", @@ -177,6 +180,37 @@ def _scoped_pql(pql: str, project_id: str) -> str: return f"({pql}) AND {scope}" if pql else scope +def _unassignable(client: Any, workspace_slug: str, project_id: str, wanted: list[str]) -> str | None: + """Name the assignee ids Plane would drop, because dropping them is invisible. + + Validation filters an assignee it will not accept out of the payload, and an + update deletes the work item's existing assignees before writing what is left. + So one unassignable id both fails to apply and clears the field, under a 200. + """ + try: + members = client.projects.get_members(workspace_slug=workspace_slug, project_id=project_id) + except HttpError as exc: + logger.warning("could not read members of project %s (%s); skipping the assignee check", project_id, exc) + return None + # Community Edition omits role and is_active, so an absent one cannot disqualify. + assignable = { + member.id + for member in members + if member.id and member.is_active is not False and (member.role is None or member.role >= _MEMBER_ROLE) + } + if not assignable: # nothing readable to judge against; never block a write that works today + return None + rejected = [user_id for user_id in wanted if user_id not in assignable] + if not rejected: + return None + return ( + f"Error: not assignable in this project: {', '.join(rejected)}. Plane drops an assignee it " + "will not accept without reporting it, and clears the work item's existing assignees in the " + "process. Only active project members at member role or above can be assigned -- " + "`member list_project` lists them." + ) + + def _description_html(description_html: str, description_stripped: str) -> str | None: """Plane recomputes the stripped form server-side, so plain text must be wrapped.""" if description_html: @@ -352,6 +386,10 @@ def write_payload() -> dict[str, Any]: if not project_id: return missing(action, "project_id") + if action in ("create", "update") and (wanted := coerce_list(assignees)): + if error := _unassignable(client, workspace_slug, project_id, wanted): + return error + if action == "create": if not name: return missing(action, "name") @@ -399,6 +437,9 @@ def write_payload() -> dict[str, Any]: return missing(action, f"add_{field[:-1]}_id or remove_{field[:-1]}_id") # Either side takes one id or several, so adding three assignees is one call. adding, removing = coerce_list(add) or [], coerce_list(remove) or [] + if field == "assignees" and adding: + if error := _unassignable(client, workspace_slug, project_id, adding): + return error current = client.work_items.retrieve( workspace_slug=workspace_slug, project_id=project_id, work_item_id=workitem_id ) diff --git a/tests/test_coercion.py b/tests/test_coercion.py index 0d9ce38..3dadf01 100644 --- a/tests/test_coercion.py +++ b/tests/test_coercion.py @@ -8,6 +8,7 @@ from __future__ import annotations import pytest +from plane.models.projects import ProjectMember from plane_mcp.coercion import accepted_types, coerce_arguments @@ -152,10 +153,15 @@ def test_booleans_are_not_treated_as_numbers(): class _Recorder: - """Stands in for the Plane client; records the first SDK call and stops.""" + """Stands in for the Plane client; records the first SDK call and stops. + + A write carrying assignees reads the project's members first, so that lookup + has to answer -- and answer with the assignee -- rather than be the recorded call. + """ def __init__(self): self.kwargs: dict = {} + self.get_members = lambda **_: [ProjectMember(id=ASSIGNEE)] def __getattr__(self, _name): return self diff --git a/tests/tools/test_dispatch.py b/tests/tools/test_dispatch.py index 008c4b8..5215025 100644 --- a/tests/tools/test_dispatch.py +++ b/tests/tools/test_dispatch.py @@ -15,8 +15,11 @@ import inspect import pytest +from plane.errors.errors import HttpError +from plane.models.projects import ProjectMember from plane_mcp.toolkit.spec import action_names +from plane_mcp.tools.workitem import _MEMBER_ROLE # Values that satisfy a parameter well enough to reach the SDK call. Enum-valued # parameters need a member of their own vocabulary; the rest take a plausible id. @@ -228,6 +231,99 @@ def test_a_valid_priority_still_reaches_the_sdk(registered, spy): assert spy.recorder.only().kwargs["data"].priority == "urgent" +# The same defect one layer out: Plane filters an assignee it will not accept out of +# the payload, and the update deletes the existing assignees before writing what is +# left. So naming one both fails to assign and clears the field, under a 200. + +ALICE = ("alice", 20, True) + + +def _project_members(*rows): + return [ + ProjectMember(id=id_, email=f"{id_}@example.com", role=role, is_active=active) for id_, role, active in rows + ] + + +@pytest.mark.parametrize( + ("members", "why"), + [ + ([ALICE], "not a member of this project"), + ([ALICE, ("bob", 5, True)], "a guest, below the assignable role"), + ([ALICE, ("bob", 20, False)], "an inactive membership"), + ], + ids=["non-member", "guest", "inactive"], +) +def test_an_unassignable_assignee_is_refused_before_the_write(members, why, registered, spy): + spy.returns["projects.get_members"] = _project_members(*members) + + result = registered["workitem"].fn(action="update", project_id="p", workitem_id="w", assignees=["bob"]) + + assert isinstance(result, str) and result.startswith("Error:"), f"{why}: {result}" + assert "bob" in result, f"{why}: refused without naming who was rejected" + assert "work_items.update" not in spy.recorder.methods, f"{why}: wrote anyway, clearing the assignees" + + +@pytest.mark.parametrize("role", [20, _MEMBER_ROLE], ids=["above the floor", "exactly the floor"]) +def test_an_assignable_member_reaches_the_sdk(role, registered, spy): + spy.returns["projects.get_members"] = _project_members(("bob", role, True)) + + registered["workitem"].fn(action="create", project_id="p", name="x", assignees=["bob"]) + + create = next(c for c in spy.recorder.calls if c.method == "work_items.create") + assert create.kwargs["data"].assignees == ["bob"] + + +def test_membership_alone_decides_when_the_edition_omits_role(registered, spy): + """Community Edition answers with identity fields only; an absent role cannot disqualify.""" + spy.returns["projects.get_members"] = [ProjectMember(id="bob")] + + registered["workitem"].fn(action="create", project_id="p", name="x", assignees=["bob"]) + + assert "work_items.create" in spy.recorder.methods + + +@pytest.mark.parametrize( + "members", + [HttpError("nope", 404), []], + ids=["lookup fails", "nothing to judge against"], +) +def test_an_unusable_member_list_does_not_block_the_write(members, registered, spy): + """The check guards a write that works today; it must not become a new way to fail.""" + spy.returns["projects.get_members"] = members + + registered["workitem"].fn(action="create", project_id="p", name="x", assignees=["bob"]) + + assert "work_items.create" in spy.recorder.methods + + +def test_a_write_carrying_no_assignees_costs_no_extra_request(registered, spy): + registered["workitem"].fn(action="update", project_id="p", workitem_id="w", name="renamed") + + assert spy.recorder.methods == ["work_items.update"] + + +def test_adding_an_unassignable_assignee_is_refused(registered, spy): + spy.returns["projects.get_members"] = _project_members(ALICE) + + result = registered["workitem"].fn(action="manage_assignee", project_id="p", workitem_id="w", add_user_id="bob") + + assert isinstance(result, str) and result.startswith("Error:"), result + assert "work_items.update" not in spy.recorder.methods + + +def test_removing_an_assignee_is_never_blocked(registered, spy): + """Whoever is on the item may no longer be assignable; that must not trap them there.""" + from types import SimpleNamespace + + spy.returns["projects.get_members"] = _project_members(ALICE) + spy.returns["work_items.retrieve"] = SimpleNamespace(assignees=["bob"], labels=[]) + + registered["workitem"].fn(action="manage_assignee", project_id="p", workitem_id="w", remove_user_id="bob") + + update = next(c for c in spy.recorder.calls if c.method == "work_items.update") + assert update.kwargs["data"].assignees == [] + + @pytest.mark.parametrize( ("end_date", "edits"), [