fix: reject unassignable assignees before Plane silently clears the field - #194
fix: reject unassignable assignees before Plane silently clears the field#194Semih702 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds project-member assignability pre-validation for work item creation, updates, and assignee additions. Invalid assignments raise before writes. Lookup failures and removals preserve existing behavior. Tests cover role, activity, incomplete payloads, and lookup scenarios. ChangesAssignee validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WorkItemTool
participant ProjectMembers
participant AssigneeValidator
participant WorkItemAPI
WorkItemTool->>AssigneeValidator: submit assignee IDs
AssigneeValidator->>ProjectMembers: get_members(project_id)
ProjectMembers-->>AssigneeValidator: member payloads
AssigneeValidator-->>WorkItemTool: accept IDs or raise ToolError
WorkItemTool->>WorkItemAPI: write work item with validated assignees
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_work_items.py (1)
115-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBoundary role value (
_MEMBER_ROLE = 15) isn't directly exercised.
MEMBERuses role 20 (admin-tier) rather than exactly 15 (member-tier). Since_is_assignableusesrole >= _MEMBER_ROLE, a member with role exactly 15 is the boundary case that most needs coverage; the current fixtures skip it entirely.✅ Suggested addition
MEMBER = {"id": "member-1", "email": "member@example.com", "role": 20, "is_active": True} +MEMBER_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}Then add a small test asserting
MEMBER_AT_THRESHOLD's id passes through unchanged.Also applies to: 149-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_work_items.py` around lines 115 - 118, Update the work-item test fixtures and related test around _is_assignable to add a member with role exactly _MEMBER_ROLE (15), such as MEMBER_AT_THRESHOLD, and assert that its id passes through unchanged. Keep the existing higher-role MEMBER fixture and other coverage intact.plane_mcp/tools/work_items.py (1)
70-131: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNarrow the pre-check fallback or mark the blind except as intentional.
except Exceptionis too broad for the assignment pre-check: it can hide bugs like anAttributeErrorfrom the member payload, while Ruff reportsBLE001. Catch the SDK/network errors you expect plus unexpected shape failures such asAttributeError,KeyError, orTypeError; alternatively add# noqa: BLE001with a concise comment referencing the deliberate fallback behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plane_mcp/tools/work_items.py` around lines 70 - 131, The broad exception handler in _assert_assignable should not silently mask arbitrary programming errors. Narrow the except clause to expected SDK/network failures plus payload-shape errors such as AttributeError, KeyError, and TypeError, preserving the existing warning-and-continue fallback; alternatively, retain Exception only with a concise BLE001 suppression comment documenting the intentional fallback.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plane_mcp/tools/work_items.py`:
- Around line 70-131: The broad exception handler in _assert_assignable should
not silently mask arbitrary programming errors. Narrow the except clause to
expected SDK/network failures plus payload-shape errors such as AttributeError,
KeyError, and TypeError, preserving the existing warning-and-continue fallback;
alternatively, retain Exception only with a concise BLE001 suppression comment
documenting the intentional fallback.
In `@tests/test_work_items.py`:
- Around line 115-118: Update the work-item test fixtures and related test
around _is_assignable to add a member with role exactly _MEMBER_ROLE (15), such
as MEMBER_AT_THRESHOLD, and assert that its id passes through unchanged. Keep
the existing higher-role MEMBER fixture and other coverage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c9e144e-d528-484b-aa2f-817f3b6c413d
📒 Files selected for processing (2)
plane_mcp/tools/work_items.pytests/test_work_items.py
…ield Plane filters ids it will not accept out of `assignees` during validation instead of 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 — the caller sees a successful write that both failed and destroyed data. Three ways to trip it, none of them visible in the response: the user is a workspace member but not a member of this project, their project role is guest (below the member floor), or the project membership is inactive. `manage_work_item_assignee` is the worst case, because it reads the current assignees, appends one id and writes the whole list back — so a single bad id discards the list it just read, which is exactly what that tool exists to avoid. Check the project's members before the write and raise instead, naming both the rejected ids and the ids that would work so the caller can recover. Verified against self-managed Plane 1.2.0; the filtering lives in the shared issue serializer, so Cloud may behave the same way, but that is unverified. The check degrades safely. Deployments whose member payload omits `role` and `is_active` fall back to a membership-only check, so an unreported role does not disqualify every member. If the member lookup fails or answers in an unexpected shape, the write proceeds exactly as it did before — a pre-check must not become a new failure mode. Writes that carry no assignees issue no extra request.
f9bfb66 to
95cbc82
Compare
|
Small update on the open question from the description — whether this is specific to self-managed or not. I still can't test Plane Cloud directly, so I went looking at the source instead. The same two pieces are on
So it isn't something that only exists in the 1.2.0 I tested against; it's still there at the current head. That said, I don't know what Cloud actually runs, so I'm not claiming it's affected — someone with a Cloud workspace could confirm in about a minute: assign a work item to a user who's in the workspace but not in that project, then re-read the work item and see whether the previous assignee survived. Also worth noting the same reasoning covers the guest case, which is easier to hit than the non-member one: a guest is a real project member, so nothing about them looks wrong to a caller, but I've also since verified this build end-to-end through the Docker image ( |
|
@coderabbitai review (pushed docstrings for the helpers the coverage check flagged — re-running so the summary reflects the current commit) |
|
✅ Action performedReview finished.
|
Plane's cutoff is `role >= 15`, but the fixtures only carried role 20 and role 5 — nothing sat on 15 itself. An off-by-one in that comparison would have locked out every plain member while the suite stayed green. Parametrizes the pass-through test over both an above-threshold role and one exactly at it. Flipping `>=` to `>` now fails the boundary case and nothing else.
|
Thanks — both looked at. Boundary role value — good catch, that was a real gap. The fixtures had role 20 and role 5, so nothing sat on 15 itself and an off-by-one in Broad On the substance: catching Happy to narrow it if a maintainer would rather have the pre-check fail loudly on unexpected shapes — that's a judgement call about which risk is worse, and I don't feel strongly enough to argue past the person who'll maintain it. |
1 participant
Fixes #193.
What's going on
Plane filters assignee ids it won't accept out of the payload during validation instead of rejecting them, and an update deletes the work item's existing assignees before writing that filtered list. So one unassignable id clears the assignees and still answers
200— the caller sees a write that both failed and destroyed data.There are three ways to land there and none of them show up in the response: the user is a workspace member but not a member of this project, their project role is guest (below the
role >= 15floor), or the project membership is inactive.manage_work_item_assigneeis the worst of the three call sites, because it reads the current assignees, appends one id, and writes the whole list back — so a single bad id discards the list it just read, which is exactly what that tool exists to prevent.What this changes
_assert_assignable()inplane_mcp/tools/work_items.pylooks up the project's members and raises before the write if any requested id isn't assignable. The message names both what was rejected and what would work, so a caller can recover without guessing:It's wired into
create_work_item,update_work_itemandmanage_work_item_assignee. The last one checks only the incomingadd_user_idrather than the merged list — validating the merged list would let an already-assigned member who has since lost project access block a legitimate removal, and such a member gets dropped by Plane on any write anyway, so that isn't a loss this change introduces.A few things I was deliberate about:
projects.get_members()(non-lite), which self-managed instances serve, so it works even on deployments where theget_project_memberstool still 404s per list_projects 404s on self-hosted Plane CE — list_lite calls /projects-lite which CE doesn't expose #172/list_projects and get_workspace_members call Cloud-only *-lite endpoints → HTTP 404 on self-hosted Community Edition #188. This doesn't touch the same lines as fix: fall back to full list endpoints when -lite routes 404 on self-hosted CE #173 and doesn't depend on it.project-membersreturns a bare user record with noroleoris_active, so an unreported role falls back to a membership-only check rather than disqualifying everyone. Membership alone still catches the common case.Scope is one source file plus tests. No return types, tool signatures or existing behaviour change.
Testing
Added 8 tests to
tests/test_work_items.py(10 cases): the three rejection reasons parametrized, a valid member passing through,role/is_activeabsent, the member lookup failing, no assignees meaning no extra request, thecreate_work_itemandmanage_work_item_assigneecall sites, and a removal not being blocked by a stale assignee. I checked these against the pre-fix tree to make sure they weren't vacuous — 5 of the cases fail without the change.ruff checkandruff format --checkare clean on both files and the repo-wide lint count is unchanged. Full non-integration suite: 66 passed (56 before, 10 new cases).For an end-to-end check I built the wheel, installed it into a clean venv with no editable link to the source tree, and ran
plane-mcp-server stdioas a real subprocess with a client talking JSON-RPC over its pipes. Against self-managed Plane 1.2.0: create with a non-member refused,update_work_itemwith a non-member refused and the original assignee still intact,manage_work_item_assigneewith a bad id refused with the list preserved, a real member applied correctly, removals unaffected, non-assignee writes unaffected.I repeated the core scenario over the HTTP header-auth transport (
/http/api-key/mcp) as well, since that path builds the Plane client from the request's API key rather than from the environment.I've since built the Docker image from the repo Dockerfile and run the server inside the container as well, in both
stdioand the defaulthttpmode — same scenario, same results. (An earlier version of this description said the Docker path was unverified; it no longer is.)One open question
I could only verify against self-managed 1.2.0, so I'm not claiming Plane Cloud is affected. What I can say is that it isn't specific to that version: both halves are still on
makeplane/planepreviewat3985693— the filter that drops unassignable ids and the unconditional delete before the write.If someone with a Cloud workspace can check, it takes about a minute: assign a work item to a user who's in the workspace but not in that project, then re-read it and see whether the previous assignee survived. If Cloud behaves the same, this stops being a self-hosting fix and becomes a general one.
🤖 Generated with Claude Code
Summary by CodeRabbit