fix: reject unassignable assignees before Plane silently clears the field

plane_mcp/tools/work_items.py

@@ -49,6 +49,86 @@ def _ids(items: Any) -> list[str]:
     return ids
 
 
+# Plane's project role ladder: guest=5, member=15, admin=20. Only member and
+# above can hold a work item assignment.
+_MEMBER_ROLE = 15
+
+
+def _is_assignable(member: Any) -> bool:
+    """Whether a project member can hold a work item assignment.
+
+    `role` and `is_active` are missing from some deployments' member payloads.
+    Only reject on a field the server actually reported — an unreported role
+    must not disqualify every member.
+    """
+    if getattr(member, "is_active", None) is False:
+        return False
+    role = getattr(member, "role", None)
+    return role is None or role >= _MEMBER_ROLE
+
+
+def _assert_assignable(client: Any, workspace_slug: str, project_id: str, assignees: list[str]) -> None:
+    """Reject assignees Plane would drop, before a write that would clear the field.
+
+    Plane filters unassignable ids out of `assignees` during validation rather
+    than rejecting them, and an update deletes the work item's existing
+    assignees before writing that filtered list. A single unassignable id
+    therefore clears the assignees and still answers 200, so the caller sees a
+    successful write that both failed and destroyed data. Checking first turns
+    it into an error naming the ids that would work.
+
+    Args:
+        client: Plane client.
+        workspace_slug: The workspace slug identifier.
+        project_id: UUID of the project the work item belongs to.
+        assignees: User IDs the caller asked to assign.
+
+    Raises:
+        ValueError: If any id is not an assignable member of the project.
+    """
+    if not assignees:
+        return
+
+    try:
+        members = client.projects.get_members(workspace_slug=workspace_slug, project_id=project_id)
+        known = {str(m.id): m for m in members}
+        assignable = {uid: m for uid, m in known.items() if _is_assignable(m)}
+    # A pre-check must not become a new failure mode: if the lookup is
+    # unavailable or answers in an unexpected shape, let the write proceed
+    # exactly as it did before.
+    except Exception as e:
+        logger.warning("Could not verify assignees against project %s members: %s", project_id, e)
+        return
+
+    rejected = [uid for uid in assignees if uid not in assignable]
+    if not rejected:
+        return
+
+    details = []
+    for uid in rejected:
+        member = known.get(uid)
+        if member is None:
+            details.append(f"{uid} (not a member of this project)")
+        elif getattr(member, "is_active", None) is False:
+            details.append(f"{uid} ({member.email}, project membership is inactive)")
+        else:
+            details.append(f"{uid} ({member.email}, project role {member.role} is below member)")
+
+    roster = sorted(f"{m.email}={uid}" for uid, m in assignable.items())
+    shown = ", ".join(roster[:25]) or "(none)"
+    if len(roster) > 25:
+        shown += f", … and {len(roster) - 25} more"
+
+    raise ValueError(
+        "Plane silently drops assignees it will not accept, and an update clears the work item's "
+        "existing assignees in the process. Rejected: "
+        + "; ".join(details)
+        + ". Only active project members at member role or above can be assigned. "
+        + f"Assignable members of this project: {shown}. "
+        + "Add the user to the project first, or use one of the IDs above."
+    )
+
+
 def _resolve_description_html(description_html: str | None, description_stripped: str | None) -> str | None:
     """Resolve the description_html to persist.
 
@@ -262,6 +342,8 @@ def create_work_item(
             priority if priority in get_args(PriorityEnum) else None  # type: ignore[assignment]
         )
 
+        _assert_assignable(client, workspace_slug, project_id, assignees or [])
+
         data = CreateWorkItem(
             name=name,
             assignees=assignees,
@@ -445,6 +527,8 @@ def update_work_item(
             priority if priority in get_args(PriorityEnum) else None  # type: ignore[assignment]
         )
 
+        _assert_assignable(client, workspace_slug, project_id, assignees or [])
+
         data = UpdateWorkItem(
             name=name,
             assignees=assignees,
@@ -508,6 +592,12 @@ def manage_work_item_assignee(
             Updated WorkItem object
         """
         client, workspace_slug = get_plane_client_context()
+        # The update below rewrites the whole list, so an unassignable add_user_id
+        # would clear the very assignees this tool exists to preserve. Only the
+        # incoming id is checked: an already-assigned member who has since lost
+        # access must not block a removal.
+        _assert_assignable(client, workspace_slug, project_id, [add_user_id] if add_user_id else [])
+
         current = client.work_items.retrieve(
             workspace_slug=workspace_slug, project_id=project_id, work_item_id=work_item_id
         )

tests/test_work_items.py

@@ -2,7 +2,10 @@
 
 import asyncio
 
+import pytest
 from fastmcp import Client, FastMCP
+from fastmcp.exceptions import ToolError
+from plane.models.projects import ProjectMember
 from plane.models.work_items import WorkItem, WorkItemDetail
 
 from plane_mcp.tools import work_items as work_item_tools
@@ -14,6 +17,12 @@ class FakeWorkItems:
 
     def __init__(self):
         self.updated = None
+        self.created = None
+
+    def create(self, workspace_slug, project_id, data):
+        """Record the create so a test can assert it was never issued."""
+        self.created = data
+        return WorkItem.model_validate({"id": "w", "name": data.name})
 
     def retrieve(self, workspace_slug, project_id, work_item_id):
         return WorkItemDetail.model_validate(
@@ -95,3 +104,139 @@ def test_manage_assignee_remove_bare_string(monkeypatch):
         {"project_id": "p", "work_item_id": "w", "remove_user_id": "existing-user"},
     )
     assert client.work_items.updated.assignees == []
+
+
+# --- assignee assignability guard -------------------------------------------
+#
+# Plane filters unassignable ids out of `assignees` during validation instead of
+# rejecting them, then deletes the work item's existing assignees before writing
+# that filtered list. One bad id clears the field and still answers 200, so the
+# guard has to run before the write.
+
+MEMBER = {"id": "member-1", "email": "member@example.com", "role": 20, "is_active": True}
+AT_THRESHOLD = {"id": "member-2", "email": "member2@example.com", "role": 15, "is_active": True}
+GUEST = {"id": "guest-1", "email": "guest@example.com", "role": 5, "is_active": True}
+INACTIVE = {"id": "inactive-1", "email": "inactive@example.com", "role": 20, "is_active": False}
+BARE = {"id": "bare-1", "email": "bare@example.com"}  # CE omits role/is_active
+
+
+class FakeProjects:
+    """`get_members` returns a bare list, the shape the non-lite endpoint serves."""
+
+    def __init__(self, members, error=None):
+        self.members = members
+        self.error = error
+        self.calls = 0
+
+    def get_members(self, workspace_slug, project_id):
+        """Return the configured members, or raise to simulate the lookup being unavailable."""
+        self.calls += 1
+        if self.error:
+            raise self.error
+        return [ProjectMember.model_validate(m) for m in self.members]
+
+
+class MemberAwareClient(FakeClient):
+    """A FakeClient that can also answer the project-member lookup the guard makes."""
+
+    def __init__(self, members, error=None):
+        super().__init__()
+        self.projects = FakeProjects(members, error)
+
+
+def _assign(monkeypatch, client, assignees, tool="update_work_item"):
+    """Call a write tool with the given assignees, defaulting to the destructive update path."""
+    args = {"project_id": "p", "work_item_id": "w", "assignees": assignees}
+    if tool == "create_work_item":
+        args = {"project_id": "p", "name": "X", "assignees": assignees}
+    return _call(monkeypatch, client, tool, args)
+
+
+@pytest.mark.parametrize(
+    "members,bad_id,reason",
+    [
+        ([MEMBER], "stranger-1", "stranger-1 (not a member of this project)"),
+        ([MEMBER, GUEST], "guest-1", "project role 5 is below member"),
+        ([MEMBER, INACTIVE], "inactive-1", "project membership is inactive"),
+    ],
+    ids=["not-a-member", "below-member-role", "inactive-membership"],
+)
+def test_unassignable_assignee_is_rejected_before_the_write(monkeypatch, members, bad_id, reason):
+    """Each way Plane filters an id out must raise, name why, and leave the write unsent.
+
+    The error also has to name an id that would work — a caller cannot guess one.
+    """
+    client = MemberAwareClient(members)
+    with pytest.raises(ToolError) as exc:
+        _assign(monkeypatch, client, [bad_id])
+    assert reason in str(exc.value)
+    assert "member@example.com=member-1" in str(exc.value)
+    assert client.work_items.updated is None, "the destructive update must not be issued"
+
+
+@pytest.mark.parametrize("member", [MEMBER, AT_THRESHOLD], ids=["above-threshold", "exactly-at-threshold"])
+def test_assignable_member_passes_through(monkeypatch, member):
+    """The happy path is unchanged, including a role sitting exactly on the member floor.
+
+    Plane's cutoff is `role >= 15`, so role 15 is the value that decides whether the
+    comparison is inclusive — an off-by-one there would lock out every plain member.
+    """
+    client = MemberAwareClient([member])
+    _assign(monkeypatch, client, [member["id"]])
+    assert client.work_items.updated.assignees == [member["id"]]
+
+
+def test_members_without_role_are_accepted_on_membership_alone(monkeypatch):
+    """Some deployments omit role/is_active; an unreported role must not fail everyone."""
+    client = MemberAwareClient([BARE])
+    _assign(monkeypatch, client, ["bare-1"])
+    assert client.work_items.updated.assignees == ["bare-1"]
+
+
+def test_member_lookup_failure_does_not_block_the_write(monkeypatch):
+    """A pre-check must not become a new failure mode when the lookup itself breaks."""
+    client = MemberAwareClient([], error=RuntimeError("members endpoint unavailable"))
+    _assign(monkeypatch, client, ["member-1"])
+    assert client.work_items.updated.assignees == ["member-1"]
+
+
+def test_empty_assignees_skips_the_member_lookup(monkeypatch):
+    """Writes that carry no assignees must not pay for an extra request."""
+    client = MemberAwareClient([MEMBER])
+    _call(monkeypatch, client, "update_work_item", {"project_id": "p", "work_item_id": "w", "name": "Y"})
+    assert client.projects.calls == 0
+
+
+def test_create_work_item_rejects_unassignable_assignee(monkeypatch):
+    """Create cannot wipe anything, but the requested assignment still silently fails."""
+    client = MemberAwareClient([MEMBER])
+    with pytest.raises(ToolError) as exc:
+        _assign(monkeypatch, client, ["stranger-1"], tool="create_work_item")
+    assert "not a member of this project" in str(exc.value)
+    assert client.work_items.created is None
+
+
+def test_manage_assignee_rejects_unassignable_add(monkeypatch):
+    """The read-modify-write in manage_work_item_assignee is what makes this destructive."""
+    client = MemberAwareClient([MEMBER])
+    with pytest.raises(ToolError):
+        _call(
+            monkeypatch,
+            client,
+            "manage_work_item_assignee",
+            {"project_id": "p", "work_item_id": "w", "add_user_id": "stranger-1"},
+        )
+    assert client.work_items.updated is None, "existing-user must survive a rejected add"
+
+
+def test_manage_assignee_removal_is_not_blocked_by_a_stale_assignee(monkeypatch):
+    """Only the incoming id is checked, so losing access does not trap the existing list."""
+    client = MemberAwareClient([MEMBER])  # "existing-user" is no longer a member
+    _call(
+        monkeypatch,
+        client,
+        "manage_work_item_assignee",
+        {"project_id": "p", "work_item_id": "w", "remove_user_id": "existing-user"},
+    )
+    assert client.work_items.updated.assignees == []
+    assert client.projects.calls == 0, "a pure removal introduces no id to check"