fix: WorkItemAttachments.create() returns wrapper response by volodchenkov · Pull Request #34 · makeplane/plane-python-sdk · GitHub

Fixes #33.

Summary

The Plane API returns a wrapper around the created attachment record,
including an S3 multipart-POST policy in upload_data for the file
upload. Previously WorkItemAttachments.create() validated this wrapper
as a flat WorkItemAttachment, raising ValidationError on the missing
asset field (the actual attachment is nested inside response["attachment"]).

This PR introduces WorkItemAttachmentCreateResponse capturing the full
wrapper shape:

attachment: WorkItemAttachment — the created record
upload_data: dict — S3 multipart-POST policy
asset_id: str | None
asset_url: str | None

WorkItemAttachments.create() is updated to return this new model.
Callers access the attachment via result.attachment and the upload
data via result.upload_data.

Test plan

Added tests/unit/test_work_item_attachments.py covering:
- list returns empty for new work item
- create returns wrapper with both attachment record and upload_data
- full lifecycle: create → list → retrieve → update is_uploaded → delete
ruff format clean on changed files
ruff check clean on changed files

Breaking change note

Return type of WorkItemAttachments.create() changes from
WorkItemAttachment to WorkItemAttachmentCreateResponse. Since the
previous return value crashed model_validate against any real Plane
response, no caller could have been depending on the old shape working.

Summary by CodeRabbit

New Features
- Attachment creation now returns the attachment plus multipart upload data (S3 policy/fields) so uploads can be performed immediately.
Improvements
- Attachment update flow now confirms and returns the latest attachment state after marking an upload as ready.
Tests
- Added end-to-end tests covering listing, creation (with upload data), update lifecycle, and deletion of attachments.

Warning

Rate limit exceeded

@volodchenkov has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 13 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 052acf07-7593-4cfc-80db-1574e59567f3

📥 Commits

Reviewing files that changed from the base of the PR and between 6df22d9 and c3c8fa9.

📒 Files selected for processing (3)

plane/api/work_items/attachments.py
plane/models/work_items.py
tests/unit/test_work_item_attachments.py

📝 Walkthrough

Walkthrough

Create now returns a wrapper model that includes the created attachment plus S3 multipart upload data; update performs a PATCH then re-lists attachments to return the updated record (raising if not found). Tests exercising list/create/update/delete were added.

Changes

WorkItem Attachments API & Model

Layer / File(s)	Summary
Data Shape plane/models/work_items.py	Adds WorkItemAttachmentCreateResponse with fields attachment: WorkItemAttachment, upload_data: dict[str, Any], optional asset_id, and asset_url.
Core Implementation plane/api/work_items/attachments.py	WorkItemAttachments.create() now validates/returns WorkItemAttachmentCreateResponse and documents the two-step flow (POST returns attachment + upload_data for S3 multipart). WorkItemAttachments.update() issues a PATCH (no response payload expected), then re-lists attachments and returns the matching attachment or raises ValueError if not found.
Tests tests/unit/test_work_item_attachments.py	New smoke tests: empty list for new work item, create returns wrapper with upload_data containing url and fields, and a full lifecycle test (create → list → get → update is_uploaded=True → delete).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client as Client\nrgba(66,133,244,0.5)
    participant API as Plane API\nrgba(52,168,83,0.5)
    participant S3 as S3 Multipart\nrgba(234,67,53,0.5)

    Client->>API: POST /work_items/{id}/attachments (create)
    API-->>Client: 201 { attachment, upload_data{url, fields}, asset_id?, asset_url? }
    Client->>S3: multipart upload using upload_data.url & fields + parts
    S3-->>Client: 200 OK (upload complete)
    Client->>API: PATCH /work_items/{id}/attachments/{attachment_id} (is_uploaded=True)
    API-->>Client: 204 No Content
    Client->>API: GET /work_items/{id}/attachments (list) -> find attachment by id
    API-->>Client: 200 [ ... attachment (is_uploaded=True) ... ]

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A tiny wrapper snug and bright,
Holds attachment and upload light,
I hop with fields and S3 cheer,
Two-step dance — the file is near,
Tests stamp paws and say: "All clear!"

🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fixing WorkItemAttachments.create() to return a wrapper response instead of a flat attachment object.
Linked Issues check	✅ Passed	The PR fully addresses issue #33 by introducing WorkItemAttachmentCreateResponse model and updating WorkItemAttachments.create() to return the wrapper response with nested attachment and upload_data.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the validation error in WorkItemAttachments.create(). The new model, updated method, and tests all address the issue with no out-of-scope additions.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches 🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/unit/test_work_item_attachments.py (1)

31-34: 💤 Low value

Bare except Exception: pass in three cleanup blocks — flagged by Ruff S110/BLE001.

All three teardown paths silently swallow any exception, making failure diagnosis difficult. A lightweight fix is to emit a warning instead of passing silently.

♻️ Proposed fix (shown for the fixture; apply the same pattern to the other two blocks)

+import warnings
 ...
         try:
             client.work_items.delete(workspace_slug, project.id, work_item.id)
-        except Exception:
-            pass
+        except Exception as exc:
+            warnings.warn(f"Cleanup failed: {exc}", stacklevel=2)

Also applies to: 61-66, 99-104

🤖 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/unit/test_work_item_attachments.py` around lines 31 - 34, The cleanup
blocks use bare "except Exception: pass" (e.g., around
client.work_items.delete(workspace_slug, project.id, work_item.id)); replace
each bare except with an explicit except Exception as e: that emits a warning or
log containing the exception details (for example via warnings.warn or process
logger) so failures during teardown are visible; also ensure warnings is
imported and apply the same change to the other two cleanup blocks that
currently swallow exceptions.

🤖 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 `@tests/unit/test_work_item_attachments.py`:
- Line 57: The test incorrectly asserts identity against False so it will fail
when WorkItemAttachment.is_uploaded is None; update the assertion in the test to
accept both False and None by replacing "assert result.attachment.is_uploaded is
False" with a check that allows either value (e.g., use "assert not
result.attachment.is_uploaded" or "assert result.attachment.is_uploaded in
(False, None)") so that result.attachment.is_uploaded (the
WorkItemAttachment.is_uploaded field) passes when the API returns null or false.

---

Nitpick comments:
In `@tests/unit/test_work_item_attachments.py`:
- Around line 31-34: The cleanup blocks use bare "except Exception: pass" (e.g.,
around client.work_items.delete(workspace_slug, project.id, work_item.id));
replace each bare except with an explicit except Exception as e: that emits a
warning or log containing the exception details (for example via warnings.warn
or process logger) so failures during teardown are visible; also ensure warnings
is imported and apply the same change to the other two cleanup blocks that
currently swallow exceptions.

🪄 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: fdd1533d-8dc3-4592-b0c3-0cc738021c49

📥 Commits

Reviewing files that changed from the base of the PR and between 6e5389f and d1457b7.

📒 Files selected for processing (3)

plane/api/work_items/attachments.py
plane/models/work_items.py
tests/unit/test_work_item_attachments.py

Actionable comments posted: 2

🧹 Nitpick comments (2)

tests/unit/test_work_item_attachments.py (1)
31-34: ⚡ Quick win

Blind except Exception: pass in teardown violates Ruff B (BLE001) — same pattern repeats at lines 65–66 and 103–104.

Use contextlib.suppress(Exception) instead, which is idiomatic, readable, and silences the Ruff warning without changing behavior.
♻️ Proposed fix (apply the same pattern to lines 65–66 and 103–104)
+from contextlib import suppress
 ...
     yield work_item.id
-    try:
+    with suppress(Exception):
         client.work_items.delete(workspace_slug, project.id, work_item.id)
-    except Exception:
-        pass
As per coding guidelines: "Apply Ruff rules: … B (bugbear)" — BLE001 is part of the bugbear ruleset.
🤖 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/unit/test_work_item_attachments.py` around lines 31 - 34, Replace the
blind try/except blocks that call client.work_items.delete(...) in the teardown
of tests/unit/test_work_item_attachments.py with contextlib.suppress(Exception);
specifically, remove the try: ... except Exception: pass patterns around the
client.work_items.delete(workspace_slug, project.id, work_item.id) calls (and
the two other identical occurrences) and wrap each delete call in a
contextlib.suppress(Exception) context manager so Ruff BLE001 is satisfied while
preserving behavior.
plane/api/work_items/attachments.py (1)
119-129: ⚖️ Poor tradeoff

update() should call retrieve() after PATCH, not list() + filter.

The docstring itself claims that the list API only returns is_uploaded=True records — yet test_full_lifecycle calls retrieve() before setting is_uploaded=True and expects it to return the attachment (lines 85–88 of the test file). If retrieve() works post-PATCH (which the test implies), using it here is strictly simpler, avoids loading all attachments, and removes the fragile assumption about is_uploaded filtering in list().
♻️ Proposed refactor
-        self._patch(
-            f"{workspace_slug}/projects/{project_id}/work-items/{work_item_id}/attachments/{attachment_id}",
-            data.model_dump(exclude_none=True),
-        )
-        for attachment in self.list(workspace_slug, project_id, work_item_id):
-            if attachment.id == attachment_id:
-                return attachment
-        raise ValueError(
-            f"Attachment {attachment_id} not found after update; "
-            "Plane only lists attachments with is_uploaded=True."
-        )
+        self._patch(
+            f"{workspace_slug}/projects/{project_id}/work-items/{work_item_id}/attachments/{attachment_id}",
+            data.model_dump(exclude_none=True),
+        )
+        return self.retrieve(workspace_slug, project_id, work_item_id, attachment_id)
Please verify whether GET /attachments/{id}/ on the live Plane API returns JSON attachment metadata or performs an S3 redirect. The update docstring claims the latter, but retrieve() exists and the lifecycle test calls it successfully, suggesting the former.
🤖 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/api/work_items/attachments.py` around lines 119 - 129, The update()
implementation currently calls self._patch(...) then iterates self.list(...) to
find the updated attachment which is inefficient and relies on is_uploaded
filtering; instead, after self._patch(...) call self.retrieve(workspace_slug,
project_id, work_item_id, attachment_id) and return its result (handle/propagate
any exceptions), removing the list/filter loop; also verify whether GET
/attachments/{id}/ returns JSON metadata or an S3 redirect and adjust retrieve()
behavior/docs accordingly if it follows a redirect flow.

🤖 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/api/work_items/attachments.py`:
- Line 120: The PATCH endpoint URL string in attachments.py currently lacks the
required trailing slash; update the URL used when constructing the PATCH call
(the f-string containing
"{workspace_slug}/projects/{project_id}/work-items/{work_item_id}/attachments/{attachment_id}")
to include a trailing "/" at the end so the endpoint becomes
".../attachments/{attachment_id}/" to comply with the API endpoint guideline.

In `@tests/unit/test_work_item_attachments.py`:
- Around line 82-83: The test currently asserts the new attachment appears in
client.work_items.attachments.list(...) immediately, but according to the
update() docstring list() only includes attachments with is_uploaded=True and
the newly created attachment is is_uploaded=False; update the test to reflect
that by removing or moving the assertion: first assert the attachment is NOT
present in the list (or skip the presence check) before calling
client.work_items.attachments.update(..., is_uploaded=True), then after update()
assert any(a.id == attachment_id for a in listed) to confirm it appears;
reference the methods client.work_items.attachments.list and
client.work_items.attachments.update and the attachment_id variable when
adjusting the assertions.

---

Nitpick comments:
In `@plane/api/work_items/attachments.py`:
- Around line 119-129: The update() implementation currently calls
self._patch(...) then iterates self.list(...) to find the updated attachment
which is inefficient and relies on is_uploaded filtering; instead, after
self._patch(...) call self.retrieve(workspace_slug, project_id, work_item_id,
attachment_id) and return its result (handle/propagate any exceptions), removing
the list/filter loop; also verify whether GET /attachments/{id}/ returns JSON
metadata or an S3 redirect and adjust retrieve() behavior/docs accordingly if it
follows a redirect flow.

In `@tests/unit/test_work_item_attachments.py`:
- Around line 31-34: Replace the blind try/except blocks that call
client.work_items.delete(...) in the teardown of
tests/unit/test_work_item_attachments.py with contextlib.suppress(Exception);
specifically, remove the try: ... except Exception: pass patterns around the
client.work_items.delete(workspace_slug, project.id, work_item.id) calls (and
the two other identical occurrences) and wrap each delete call in a
contextlib.suppress(Exception) context manager so Ruff BLE001 is satisfied while
preserving behavior.

🪄 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: 8d2a6414-ede1-4895-a14a-7af07b1dfd85

📥 Commits

Reviewing files that changed from the base of the PR and between d1457b7 and 6df22d9.

📒 Files selected for processing (3)

plane/api/work_items/attachments.py
plane/models/work_items.py
tests/unit/test_work_item_attachments.py

🚧 Files skipped from review as they are similar to previous changes (1)

plane/models/work_items.py

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.

@coderabbitai review

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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.

create(): The Plane API returns a wrapper around the created attachment record, including an S3 multipart-POST policy in 'upload_data' for the file upload. Previously WorkItemAttachments.create() validated this wrapper as a flat WorkItemAttachment, which raised ValidationError on the missing 'asset' field (the actual attachment is nested inside response['attachment']). This change introduces WorkItemAttachmentCreateResponse, a model that captures the full wrapper shape: - attachment: WorkItemAttachment (the created record) - upload_data: dict (S3 multipart-POST policy) - asset_id: str | None - asset_url: str | None WorkItemAttachments.create() is updated to return this new model. Callers can now access the attachment record via result.attachment and the upload data via result.upload_data to perform the actual file upload to object storage, then call update(is_uploaded=True) to mark the attachment ready. update(): The Plane API responds to attachment PATCH with 204 No Content (no body), and exposes no metadata-by-id endpoint (GET on a single attachment URL serves the file via S3 redirect). Previously update() called WorkItemAttachment.model_validate on the empty body, which raised ValidationError. Update now follows the PATCH with a list call and filters by id, returning the updated WorkItemAttachment per CRUD convention. Tests: Adds tests/unit/test_work_item_attachments.py covering list, create (asserting wrapper shape), and full lifecycle (create -> mark uploaded -> list -> retrieve -> delete). Closes makeplane#33

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.

Re: the suggestion to use retrieve() instead of list + filter in update() — declining this one.

GET /api/v1/workspaces/{ws}/projects/{p}/work-items/{wi}/attachments/{aid}/ does not return JSON metadata. It returns a 302 redirect to an S3 presigned download URL — the endpoint serves the file, not the record. Verified empirically against a live self-hosted Plane (status 302, location: https://storage.yandexcloud.net/plane-data-private/...).

So retrieve() itself is broken for metadata fetch (separate concern; will file an issue). The list + filter workaround in update() is the only approach that works against Plane's actual API today.

Updated the test for the previous reorder feedback (mark uploaded → list → delete) so the list+filter path is exercised end-to-end.

@coderabbitai review

Tip

For best results, initiate chat on the files or code changes.

@volodchenkov Thanks for the detailed empirical clarification — a 302 redirect to a presigned S3 URL is a completely valid reason to avoid retrieve() for metadata fetches, and the list + filter workaround in update() makes total sense given that constraint. I'll make sure not to flag this pattern in future reviews. I'm also noting the retrieve() breakage for separate tracking.

Kicking off the re-review now.

(⊙_⊙)？

[review]

✏️ Learnings added

Learnt from: volodchenkov
URL: https://github.com/makeplane/plane-python-sdk/pull/34

Timestamp: 2026-05-06T06:58:53.770Z
Learning: In the Plane API (plane-python-sdk), `GET /api/v1/workspaces/{ws}/projects/{p}/work-items/{wi}/attachments/{aid}/` does NOT return JSON metadata. It returns a 302 redirect to an S3 presigned download URL (verified empirically against a live self-hosted Plane instance). Therefore, `WorkItemAttachments.retrieve()` is broken for metadata fetches, and the `list + filter` workaround used in `WorkItemAttachments.update()` (list all attachments, then filter by `attachment_id`) is the only correct approach to obtain attachment metadata after an update.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

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.

coderabbitai Bot reviewed May 5, 2026

View reviewed changes

Comment thread tests/unit/test_work_item_attachments.py Outdated

volodchenkov force-pushed the fix/work-item-attachments-create-response branch 3 times, most recently from 2fa20f7 to 6df22d9 Compare May 5, 2026 12:56

coderabbitai Bot reviewed May 5, 2026

View reviewed changes

Comment thread plane/api/work_items/attachments.py Outdated

Comment thread tests/unit/test_work_item_attachments.py Outdated

volodchenkov mentioned this pull request May 5, 2026

feat: add work item attachment tools makeplane/plane-mcp-server#119

Open

3 tasks

volodchenkov force-pushed the fix/work-item-attachments-create-response branch from 6df22d9 to 4974557 Compare May 5, 2026 21:35

volodchenkov force-pushed the fix/work-item-attachments-create-response branch from 4974557 to 82278fe Compare May 5, 2026 21:37

volodchenkov force-pushed the fix/work-item-attachments-create-response branch from 82278fe to 0bb1112 Compare May 5, 2026 22:11

volodchenkov force-pushed the fix/work-item-attachments-create-response branch from 0bb1112 to c3c8fa9 Compare May 5, 2026 22:13

fix: WorkItemAttachments.create() returns wrapper response by volodchenkov · Pull Request #34 · makeplane/plane-python-sdk · GitHub

Uh oh!

fix: WorkItemAttachments.create() returns wrapper response - #34

fix: WorkItemAttachments.create() returns wrapper response#34
volodchenkov wants to merge 1 commit into
makeplane:mainfrom
volodchenkov:fix/work-item-attachments-create-response

volodchenkov commented May 5, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

coderabbitai Bot commented May 5, 2026 •
edited
Loading

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

volodchenkov commented May 5, 2026

Uh oh!

coderabbitai Bot commented May 5, 2026

Uh oh!

volodchenkov commented May 6, 2026

Uh oh!

coderabbitai Bot commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fix: WorkItemAttachments.create() returns wrapper response by volodchenkov · Pull Request #34 · makeplane/plane-python-sdk · GitHub

Uh oh!

fix: WorkItemAttachments.create() returns wrapper response - #34

Conversation

volodchenkov commented May 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Breaking change note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

volodchenkov commented May 5, 2026

Uh oh!

coderabbitai Bot commented May 5, 2026

Uh oh!

volodchenkov commented May 6, 2026

Uh oh!

coderabbitai Bot commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

volodchenkov commented May 5, 2026 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented May 5, 2026 •
edited
Loading