feat: add work item attachment tools#119
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new work item attachment feature: a Pydantic wrapper model and four MCP tools (list, create, update, delete) plus wiring to register them; integration tests updated to exercise the attachment lifecycle and expect the new tools. ChangesWork Item Attachment Lifecycle
Sequence Diagram(s)sequenceDiagram
participant MCP as MCP tool
participant Plane as Plane API client
participant Storage as S3 / Upload service
MCP->>Plane: create work item attachment (upload request)
Plane-->>MCP: returns wrapper + upload_data (S3 multipart info)
MCP->>Storage: (client uploads parts using upload_data)
MCP->>Plane: update attachment (is_uploaded=True)
Plane-->>MCP: acknowledges update
MCP->>Plane: list/delete attachment(s)
Plane-->>MCP: returns list / confirms deletion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_integration.py (1)
163-173: ⚡ Quick winAdd an assertion that the created attachment is actually listed.
Right now this segment only prints IDs; it doesn’t verify behavior. A direct assertion here would make the lifecycle test meaningful.
Suggested fix
attachments_list = extract_result(attachments_list_result) - print(f"Attachments on work item 1: {[a['id'] for a in attachments_list if isinstance(a, dict) and 'id' in a]}") + attachment_ids = [a["id"] for a in attachments_list if isinstance(a, dict) and "id" in a] + print(f"Attachments on work item 1: {attachment_ids}") + assert attachment_id in attachment_ids, "Created attachment was not returned by list_work_item_attachments"🤖 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_integration.py` around lines 163 - 173, Add an assertion that the attachment you created is present in the returned list: after calling client.call_tool("list_work_item_attachments") and extracting attachments_list via extract_result(attachments_list_result), assert that the expected attachment id (the id returned when the attachment was created earlier, e.g. stored in a variable like created_attachment_id or similar) appears in [a['id'] for a in attachments_list if isinstance(a, dict) and 'id' in a]; if the creation step used a different variable name, reference that (e.g. created_attachment, created_attachment_result) and assert its 'id' is contained in attachments_list to fail the test when the attachment is not listed.
🤖 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.
Inline comments:
In `@plane_mcp/tools/work_item_attachments.py`:
- Around line 127-170: The update_work_item_attachment function currently
accepts is_uploaded=None/False which permits sending a PATCH via
client.work_items.attachments._patch but then the subsequent list call
(client.work_items.attachments.list) only returns attachments with
is_uploaded=True, causing false "not found" errors; change the function to only
allow transitioning to is_uploaded=True (validate that is_uploaded is True at
the start and raise a ValueError if it is None or False) so the PATCH and
subsequent list/filter by attachment_id are consistent with Plane's behavior.
---
Nitpick comments:
In `@tests/test_integration.py`:
- Around line 163-173: Add an assertion that the attachment you created is
present in the returned list: after calling
client.call_tool("list_work_item_attachments") and extracting attachments_list
via extract_result(attachments_list_result), assert that the expected attachment
id (the id returned when the attachment was created earlier, e.g. stored in a
variable like created_attachment_id or similar) appears in [a['id'] for a in
attachments_list if isinstance(a, dict) and 'id' in a]; if the creation step
used a different variable name, reference that (e.g. created_attachment,
created_attachment_result) and assert its 'id' is contained in attachments_list
to fail the test when the attachment is not listed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9450b8c4-bab6-4548-a901-2a843b91f3da
📒 Files selected for processing (3)
plane_mcp/tools/__init__.pyplane_mcp/tools/work_item_attachments.pytests/test_integration.py
volodchenkov
force-pushed
the
feat/work-item-attachments
branch
from
4c36fd3 to
8552aa5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
volodchenkov
force-pushed
the
feat/work-item-attachments
branch
2 times, most recently
from
18d2c61 to
2ce059e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plane_mcp/tools/work_item_attachments.py (1)
183-190: ⚡ Quick winAdd the missing
Returnssection to the delete tool docstring.Lines 183-190 are the only tool docstring in this module without a
Returnsblock, so this one falls out of step with the rest of the tool surface.As per coding guidelines "Tool docstrings must include Args and Returns sections".
🤖 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_item_attachments.py` around lines 183 - 190, Update the tool docstring that begins "Delete an attachment from a work item." to include a Returns section that matches the style of the other tools: state the return type (e.g., None or bool) and a short description of what is returned on success/failure (for the function that implements the delete attachment tool — the function whose docstring starts "Delete an attachment from a work item."). Ensure the Returns block follows the Args block formatting used elsewhere in this module.
🤖 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.
Inline comments:
In `@plane_mcp/tools/work_item_attachments.py`:
- Around line 63-71: Rename the parameter named type in the
create_work_item_attachment function to mime_type to avoid shadowing Python's
builtin; update the function signature and all internal uses to mime_type, and
when constructing the MCP DTO (WorkItemAttachmentUploadRequest) pass the value
as type=mime_type so the external field name remains unchanged (e.g.,
WorkItemAttachmentUploadRequest(type=mime_type, ...)). Also update any local
variable names and argument forwarding in create_work_item_attachment to use
mime_type to keep naming consistent.
In `@tests/test_integration.py`:
- Around line 132-160: The test creates an attachment via
create_work_item_attachment but never actually uploads the file bytes using
attachment_wrapper["upload_data"], so the presigned/form upload step is skipped;
modify the test to POST a multipart/form-data to
attachment_wrapper["upload_data"]["url"] using the returned fields
(attachment_wrapper["upload_data"]["fields"] or similar) and include the file
body (e.g., 12 bytes or the expected content) before calling
update_work_item_attachment to set is_uploaded=True, keeping the existing
client.call_tool calls (create_work_item_attachment and
update_work_item_attachment) and using an async HTTP client compatible with the
test framework to perform the upload.
---
Nitpick comments:
In `@plane_mcp/tools/work_item_attachments.py`:
- Around line 183-190: Update the tool docstring that begins "Delete an
attachment from a work item." to include a Returns section that matches the
style of the other tools: state the return type (e.g., None or bool) and a short
description of what is returned on success/failure (for the function that
implements the delete attachment tool — the function whose docstring starts
"Delete an attachment from a work item."). Ensure the Returns block follows the
Args block formatting used elsewhere in this module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09c1c967-4834-42c8-9df3-60210182bd08
📒 Files selected for processing (3)
plane_mcp/tools/__init__.pyplane_mcp/tools/work_item_attachments.pytests/test_integration.py
| # Work item attachment lifecycle (create metadata → list → retrieve → mark uploaded → delete) | ||
| print("Creating work item attachment...") | ||
| attachment_result = await client.call_tool( | ||
| "create_work_item_attachment", | ||
| { | ||
| "project_id": project_id, | ||
| "work_item_id": work_item_1_id, | ||
| "name": f"test-{unique_id}.txt", | ||
| "size": 12, | ||
| "type": "text/plain", | ||
| }, | ||
| ) | ||
| attachment_wrapper = extract_result(attachment_result) | ||
| # create_work_item_attachment returns WorkItemAttachmentCreated wrapper: | ||
| # {attachment, upload_data, asset_id, asset_url} | ||
| attachment_id = attachment_wrapper["attachment"]["id"] | ||
| print(f"Created attachment: {attachment_id}") | ||
|
|
||
| # Plane filters list/retrieve to is_uploaded=True only — mark first. | ||
| print("Marking attachment as uploaded...") | ||
| await client.call_tool( | ||
| "update_work_item_attachment", | ||
| { | ||
| "project_id": project_id, | ||
| "work_item_id": work_item_1_id, | ||
| "attachment_id": attachment_id, | ||
| "is_uploaded": True, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Actually upload the file before flipping is_uploaded=True.
Lines 144-160 never use attachment_wrapper["upload_data"], so this test still passes if the presigned form policy or upload endpoint is broken. That misses the main failure mode in the new two-step attachment flow; POST a tiny multipart body to upload_data["url"] with the returned fields before calling update_work_item_attachment.
🤖 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_integration.py` around lines 132 - 160, The test creates an
attachment via create_work_item_attachment but never actually uploads the file
bytes using attachment_wrapper["upload_data"], so the presigned/form upload step
is skipped; modify the test to POST a multipart/form-data to
attachment_wrapper["upload_data"]["url"] using the returned fields
(attachment_wrapper["upload_data"]["fields"] or similar) and include the file
body (e.g., 12 bytes or the expected content) before calling
update_work_item_attachment to set is_uploaded=True, keeping the existing
client.call_tool calls (create_work_item_attachment and
update_work_item_attachment) and using an async HTTP client compatible with the
test framework to perform the upload.
Adds four MCP tools wrapping the Plane work-item attachment API: - list_work_item_attachments - create_work_item_attachment - update_work_item_attachment - delete_work_item_attachment The tools follow the existing pattern in plane_mcp/tools/work_item_links.py (flat params at the MCP boundary, Pydantic models from plane-sdk). Closes makeplane#118. Two-step upload flow Plane attachments use a two-step asset flow: 1. POST metadata (name, size, type) -> server returns an attachment record plus an S3 multipart-POST policy in 'upload_data'. 2. Caller posts the file as multipart/form-data to upload_data['url'] using upload_data['fields'] plus a 'file' part. 3. PATCH is_uploaded=true on the attachment to mark it ready. create_work_item_attachment returns a WorkItemAttachmentCreated wrapper exposing both the attachment record and the upload_data so the agent can perform the upload itself, then call update_work_item_attachment with is_uploaded=True. Workarounds for plane-sdk bugs (see makeplane/plane-python-sdk#34) The current pinned plane-sdk (0.2.10) has two attachment bugs we work around in this module: - WorkItemAttachments.create() validates the API wrapper response as a flat WorkItemAttachment, raising ValidationError. We bypass via the inherited _post and validate against our local WorkItemAttachmentCreated wrapper. - WorkItemAttachments.update() calls model_validate on the API's 204 No Content response. We bypass via _patch and then list + filter by id to return the updated record. Both workarounds are documented inline. When the upstream SDK fix lands (PR makeplane/plane-python-sdk#34) and is bumped here, the workarounds can collapse to direct SDK calls. retrieve_work_item_attachment is intentionally not exposed: Plane has no metadata-by-id endpoint for attachments (the GET on a single attachment URL serves a download redirect to S3). Agents that need metadata can use list_work_item_attachments and filter. Tests Extends run_integration_test in tests/test_integration.py with the attachment lifecycle (create -> mark uploaded -> list -> delete) on work_item_1, before the epic step. EXPECTED_TOOLS is updated to include the four attachment tools. Verified end-to-end against a live self-hosted Plane instance: all four attachment operations pass.
|
Last round of polish pushed (
Re: the suggestion to actually POST file bytes to the presigned URL in the integration test — declining for this PR. It would add an httpx dependency and tie test reliability to S3 reachability. Plane accepts @coderabbitai review |
volodchenkov
force-pushed
the
feat/work-item-attachments
branch
from
2ce059e to
09c9064
Compare
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
1 participant
Closes #118.
Summary
Adds four MCP tools wrapping Plane's work-item attachment API:
list_work_item_attachmentscreate_work_item_attachmentupdate_work_item_attachmentdelete_work_item_attachmentPatterned after
plane_mcp/tools/work_item_links.py(flat params at the MCP boundary, Pydantic models from plane-sdk).Two-step upload flow
Plane attachments use a two-step asset flow.
create_work_item_attachmentreturns aWorkItemAttachmentCreatedwrapper exposing both the attachment record and an S3 multipart-POST policy inupload_data. The agent posts the file toupload_data["url"]itself, then callsupdate_work_item_attachmentwithis_uploaded=Trueto mark it ready.retrieve_work_item_attachmentis intentionally not exposed: Plane has no metadata-by-id endpoint for attachments (GET on a single attachment URL serves a download redirect to S3). Agents that need metadata can uselist_work_item_attachmentsand filter.Workarounds for plane-sdk bugs
The current pinned
plane-sdk==0.2.10has two attachment bugs we work around in this module — both being fixed upstream in makeplane/plane-python-sdk#34:WorkItemAttachments.create()validates the API wrapper response as a flatWorkItemAttachment, raisingValidationError. We bypass via the inherited_postand validate against our localWorkItemAttachmentCreatedwrapper.WorkItemAttachments.update()callsmodel_validateon Plane's204 No Contentresponse. We bypass via_patchand thenlist + filter by id.Both workarounds are documented inline. When the SDK fix lands and the pin is bumped, the workarounds collapse to direct SDK calls.
Test plan
tests/test_integration.py::run_integration_testwith attachment lifecycle (create → mark uploaded → list → delete) onwork_item_1EXPECTED_TOOLSupdated with the four attachment tool namesNotes
The original issue proposed three tools (list / upload / delete). Expanded to four to match the lifecycle pattern in
work_item_links.py. Easy to trim if preferred.Summary by CodeRabbit
New Features
Tests