fix: refuse an assignee Plane will not accept, rather than lose the field - #204
Open
Semih702 wants to merge 1 commit into
Open
fix: refuse an assignee Plane will not accept, rather than lose the field#204Semih702 wants to merge 1 commit into
Semih702 wants to merge 1 commit into
Conversation
…ield 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.
|
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 (3)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughWork-item create, update, and assignee-management additions now validate assignees against active project membership and role requirements before writes. Validation skips inconclusive member lookups, while removals remain unrestricted. Tests cover these paths. ChangesWork-item assignee validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change rejects assignees that Plane would silently discard, preventing unintended loss of existing assignments while preserving valid assignment and removal behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WorkItemTool
participant ProjectMembersAPI
participant WorkItemAPI
WorkItemTool->>ProjectMembersAPI: Request project members
ProjectMembersAPI-->>WorkItemTool: Return members or lookup failure
WorkItemTool->>WorkItemTool: Validate active role and requested IDs
WorkItemTool->>WorkItemAPI: Send write when permitted
🚥 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
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. A write naming one therefore both fails to apply and clears the field, under a
200— nothing in the response distinguishes it from a successful assignment.Measured against self-managed 1.2.0, through v0.3.0's own
workitemtool, wherealiceis a member of the project andbobis a workspace member who isn't:update assignees=[bob][]— alice gone, bob never addedupdate assignees=[alice, bob][alice]— bob silently missingmanage_assignee add_user_id=bobOnly the first destroys data, and it is the one that bites in practice: an agent told "assign these twelve to bob", where bob isn't in that project, clears the assignee on twelve items and reports twelve successes.
Three ways to land there, none of them visible to the caller: the user is a workspace member but not a member of this project; their project role is guest, below Plane's
role__gte=15floor; or the membership isis_active=False. Both halves are onmakeplane/planepreview— the filter that drops the id and the unconditional delete before the write.It is the same failure
tools/README.mdalready rules out for enum parameters — "dropping it writes the record without the field and reports success" — one layer out, where the value is a UUID and the API does the dropping.What changed
_unassignable()reads the project's members and names the rejected ids before the write. Wired intocreate,updateandmanage_assignee:Notes:
HttpError) or an empty one skips the check, and the write proceeds exactly as it does today.role./project-membersanswers with identity fields only there, so an absentroleoris_activecannot disqualify anyone — membership alone decides. Where both are present, guest and inactive are caught too.projects.get_members()is the non-lite endpoint, which self-managed serves;project-members-lite404s there, 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.manage_assigneevalidates only the incoming id rather than the merged list, so a member who has since lost project access can still be removed.Type of Change
Test Scenarios
Unit — 7 tests, 11 cases in
tests/tools/test_dispatch.py, beside the enum-dropping test they are a variation of:roleabsentmanage_assigneerefusing an add, and a removal not being blockedChecked against the pre-fix tree: 4 of the 11 fail without the guard, and the other 7 are the "must keep working" half that should pass either way.
1092 passed, 25 skipped.ruff checkandruff format --checkclean. One existing test needed a line —test_coercion.py's recorder stops at the first SDK call, and the preflight is now that call, so it answers the member lookup rather than being consumed by it.End to end — 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 driven over JSON-RPC against self-managed 1.2.0. 14/14:manage_assigneeadd refused with the list intact; adding a real member still works; removal not blocked; a write carrying no assignees unaffected;manage_labelnot caught by the assignee check.update_work_itemandmanage_work_item_assigneeare guarded the same way;create_work_itemandretrieve_work_itemare unaffected.I verified the non-member case live. Guest and inactive membership go through the same filter, so I would expect them to behave identically, but testing those means reshuffling memberships on a live workspace — they are covered by unit tests only.
References
Fixes #193. Replaces #194, which patched
plane_mcp/tools/work_items.py— a file this refactor removed.Summary by CodeRabbit