Repository navigation
Add generic attach_data_to_card tool - #81
Conversation
attach_image_data_to_card already accepts any binary data — its implementation just buffers bytes and posts them as a multipart attachment — but its name, title, description, and PNG/screenshot defaults all signal "images only." MCP clients reading tool listings reasonably conclude that markdown, PDFs, and other non-image content need a different mechanism (e.g., external asset hosting), even though the tool would handle them fine. Add a sibling tool `attach_data_to_card` whose name and documentation match its actual capability: - generic title and description naming common content types - `data` parameter (vs. `imageData`) - defaults to `application/octet-stream` and `attachment-<timestamp>` rather than PNG/screenshot defaults - routes to a new `attachDataToCard` client method `attach_image_data_to_card` is preserved unchanged in behavior — its defaults are still useful for the screenshot-paste case. It now delegates to `attachDataToCard` after applying its image-flavored defaults, and its description points callers to the generic tool for non-image content. No breaking changes for existing clients.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new attach_data_to_card tool and refactors attach_image_data_to_card into a convenience wrapper to support arbitrary binary data uploads. The review feedback highlights the need to refactor large files to meet the 500-line limit, add automated tests for the new functionality, and enhance MIME type inference by considering file extensions.
|
|
||
| // Attach image data to card (for base64/data URL uploads) | ||
| // Attach arbitrary binary data to a card (base64 or data URL) | ||
| this.server.registerTool( |
There was a problem hiding this comment.
The src/index.ts file currently has 1625 lines, which violates the repository style guide requiring files to be under 500 lines. While this PR adds necessary functionality, consider refactoring the tool registrations into separate modules to improve maintainability and adhere to the modular design principles.
References
- Modular Design: Files under 500 lines (link)
There was a problem hiding this comment.
This file was already 1625 lines before this PR (the change adds ~55). Splitting tool registrations into separate modules is a worthwhile refactor, but it would balloon the diff well beyond this feature. Happy to file a follow-up issue tracking the split rather than couple it here.
| // Attach image data to card (for base64/data URL uploads) | ||
| // Attach arbitrary binary data to a card (base64 or data URL) | ||
| this.server.registerTool( | ||
| 'attach_data_to_card', |
There was a problem hiding this comment.
The PR introduces a new tool attach_data_to_card but does not include corresponding automated tests. Following the 'Test-First' principle in the style guide, please add unit or integration tests to verify the new tool's functionality, especially for non-image content types and edge cases like invalid base64 data.
References
- Test-First: Write tests before implementation (link)
There was a problem hiding this comment.
Added unit tests in 3609001 covering both touched methods — explicit mime type, data URL extraction, filename-extension inference, octet-stream fallback, explicit override of a data URL mime, malformed data URL rejection, and the image-flavored defaults for attachImageDataToCard. Scoped to the methods this PR touches; happy to file a follow-up if you'd like coverage extended to attachFileToCard / attachImageToCard, which currently lack unit tests as well.
| .string() | ||
| .optional() | ||
| .describe( | ||
| 'MIME type of the data (e.g. "text/markdown", "application/pdf", "image/png"). Required for correct rendering in Trello; defaults to "application/octet-stream" if omitted and not derivable from a data URL.' |
There was a problem hiding this comment.
Update the description to reflect that the MIME type can also be inferred from the filename extension if provided.
| 'MIME type of the data (e.g. "text/markdown", "application/pdf", "image/png"). Required for correct rendering in Trello; defaults to "application/octet-stream" if omitted and not derivable from a data URL.' | |
| 'MIME type of the data (e.g. "text/markdown", "application/pdf", "image/png"). Required for correct rendering in Trello; defaults to "application/octet-stream" if omitted and not derivable from a data URL or filename extension.' |
There was a problem hiding this comment.
Done in 88c0ba3 — attach_data_to_card now falls back to filename-extension lookup via MIME_TYPES when mimeType is omitted (matching attachFileToCard). The tool description has been updated accordingly.
| } | ||
|
|
||
| async attachImageDataToCard( | ||
| async attachDataToCard( |
There was a problem hiding this comment.
The src/trello-client.ts file currently has 1334 lines, exceeding the 500-line limit specified in the repository style guide. Consider splitting the client logic into smaller, more focused classes or utility modules to improve code organization.
References
- Modular Design: Files under 500 lines (link)
There was a problem hiding this comment.
Same as the index.ts comment above — trello-client.ts was already 1334 lines before this PR. Suggest tracking a focused split as a separate refactor rather than expanding this PR's diff.
| // Convert base64 or data URL to buffer | ||
| let buffer: Buffer; | ||
| let effectiveMimeType = mimeType || 'image/png'; | ||
| let effectiveMimeType = mimeType || 'application/octet-stream'; |
There was a problem hiding this comment.
For better consistency with attachFileToCard, consider inferring the MIME type from the filename extension if the mimeType parameter is not provided. This improves the user experience when uploading common file types like PDFs or Markdown without explicitly specifying the MIME type.
let effectiveMimeType = mimeType;
if (!effectiveMimeType && name) {
const ext = path.extname(name).toLowerCase();
effectiveMimeType = MIME_TYPES[ext];
}
effectiveMimeType = effectiveMimeType || 'application/octet-stream';There was a problem hiding this comment.
Done in 88c0ba3 — added the filename-extension fallback via MIME_TYPES and let an explicit mimeType override one parsed from a data URL prefix (so callers can correct an inaccurate prefix).
When mimeType is not provided, fall back to looking up the extension of the attachment name in MIME_TYPES (matching attachFileToCard's behavior) before defaulting to application/octet-stream. Also lets an explicit mimeType argument override one parsed from a data URL prefix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds unit tests for the two methods touched in this PR, covering: - explicit mime type + filename - mime type extraction from a data URL - mime type inference from filename extension - application/octet-stream fallback when no hints exist - explicit mimeType overriding a data URL's mime type - malformed data URL rejection without an upload - attachImageDataToCard's image/png + screenshot-<ts>.png defaults - attachImageDataToCard respecting caller-supplied overrides Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Woohoo glad to pitch in! |
2 participants
Summary
attach_image_data_to_cardalready accepts any binary data — under the hood it just buffers bytes and posts a multipart attachment to Trello. But every signal an MCP client sees says "images only": the tool name, title, description ("for screenshot uploads"), theimageDataparameter, and the PNG/screenshot defaults. Reasonable clients conclude that markdown, PDFs, or other non-image content need to live somewhere else (external hosting, links in comments, etc.), even though the existing implementation handles them fine.This PR adds a sibling tool,
attach_data_to_card, whose surface matches its actual capability — and leavesattach_image_data_to_carduntouched in behavior for callers that depend on its image-flavored defaults.What changes
New tool:
attach_data_to_carddatainstead ofimageDataapplication/octet-streamandattachment-<timestamp>— no assumed PNGattachDataToCardExisting tool:
attach_image_data_to_card— unchanged behaviorscreenshot-<timestamp>.png)attach_data_to_cardfor non-image contentattachDataToCardafter applying its image defaultsNo breaking changes.
Why two tools instead of generalizing the existing one
Renaming the tool or its parameter would break every client currently calling
attach_image_data_to_card. Changing the defaults would silently change behavior for screenshot pasting — the dominant use case the existing tool was designed around. Two first-class tools, each named honestly for what it does, lets callers pick by intent without forcing a migration.Test plan
bun buildsucceedsupdateChecklistItemexists onmainalready)attach_data_to_cardand confirm Trello renders it correctlyattach_image_data_to_cardstill works for screenshot pasting with no behavior change