Feature: File uploads and attachments - #117
Open
CFDan wants to merge 3 commits into
Open
Conversation
Adds the ability to upload a file and attach it to a task, a comment or a message, plus a project's files area. Uploading is a single authenticated multipart request to POST /projects/api/v1/pendingfiles.json, which returns an opaque reference. The reference is then passed to whichever entity should own the file. The presigned S3 flow the public docs describe was not used: it needs three requests, bakes the exact content length into the signature, and signs the ACL header only outside staging, so an SDK would have to know which environment its installation is in. Tasks take the v3 attachments object, which is a sibling of the task in the request body, in the same way predecessors already are. Comments and messages take the v1 pendingFileAttachments list. The API accepts either a JSON array or a comma separated string there, so no custom encoding type was needed. attachmentOptions is deliberately not modelled. Its only field defaults to false, and on task update the API seeds the "keep these" list from the task's current attachments, so it removes nothing. Attaching is always additive. Notebooks, milestones, links and message replies have no attachment support in the API, and the v3 comment attachments route is broken server side, which is why comments continue to be written through v1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contents is now a byte slice rather than an io.Reader. A multipart body has to be assembled in full before it is sent, so a reader was being drained into a buffer anyway, and holding the bytes keeps the request re-executable: running the same request value twice used to upload an empty file the second time. It also removes the double buffering and the panic on a typed-nil reader. FileCreateResponse decodes the documented "id" instead of the "fileId" alias, matching every other v1 create response here, and FileCreateRequest validates its required fields the way PendingFileCreateRequest already did. Adds attachments_test.go, which drives HTTPRequest and asserts the encoded body. The integration tests only assert that a write was accepted, and the API answers 2xx whether or not it understood the attachment, so a wrong key or a field nested in the wrong place went unnoticed. It covers both multi-reference forms, attaching an existing file by identifier with a category, and the absence of the attachments key when none was asked for. These run without a configured engine. The attachment integration tests attached a pending file directly, which makes the API create a project file the test has no way to find, so every CI run left files behind in the shared test project. They now add the file to the project first and delete it in cleanup, which also covers attaching by identifier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Review pass done — pushed d48d296. Fixed
Checked against the API rather than changed Four findings assumed behaviour that turned out not to hold. I tested each on a live installation:
Not changed The |
FileCreateResponse exposes a single ID again; the fileId fallback is decoded through a local struct in HandleHTTPResponse rather than a second exported field that mirrors the first. Documents that the v1 message-update attachments field is additive, verified against a live installation, matching its pending-file sibling. Presizes the multipart upload buffer, since the file length is known. Adds a clarifying comment on the pending-file upload path, which is the only v1 route in the package under /projects/api/v1/. Guards the request-body test helper against a nil body and indexes the subset matcher's array path; TestFileDelete now provisions its file through the createFile helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Lets callers upload a file and attach it to a task (at creation or afterwards), a comment, or a message. This is the gap behind a customer request for attachment support in the MCP server — LLMs generate plans and specs and currently have no way to put them on a task.
API
Comments and messages take
PendingFileAttachments []PendingFileRef. Messages also takeAttachments LegacyNumericListfor files that already exist.FileCreate/FileDeletecover the project's files area, which is the only way to get an identifier reusable across several tasks, since a pending reference is consumed the first time it is attached.Notes for review
Upload is one multipart request, not the presigned S3 flow the docs describe.
POST /projects/api/v1/pendingfiles.jsonis authenticated, goes to the installation, and has explicit middleware carve-outs inprojectsapigo(the request timeout is disabled for that exact path). The presigned flow needs three requests, bakes the exact content length into the signature so the body cannot be streamed, and signsX-Amz-Aclonly outside staging — an SDK taking that route would have to know which environment its installation is in.attachmentOptionsis deliberately not modelled. Its only field defaults tofalse, and on task update the server seeds the "keep these" list from the task's current attachments, so it removes nothing. Attaching is always additive. Modelling it would advertise a capability that does not work.Comments stay on v1. v3 would drop four of the five parent types, break
CommentCreateResponse.IDfor existing callers, and the v3 comment attachments path panics server side (streamlinkQueryhas nocommentcase) — I will raise that separately.json:"-"plusomitzeroonTaskAttachments, so existing callers' payloads are unchanged rather than gaining an emptyattachments: {}.FileCreateRequesthasNotifyCurrentUserbut not the groupNotify. The API returns 201 for a bool,"ALL"and an ID list alike, but that only proves acceptance, not that notifications fire, so I left it unmodelled rather than guess at a notifier interface.Verification
Run live against a real installation, in a throwaway project since deleted: upload, task create with attachment, attach to an existing task, comment, message, project file create, reusing that file identifier on a second task, file delete, and an update with no attachments to confirm the key is omitted. All passed.
go build,go vet,go test ./...andgo generate ./...are clean, with no sparse fieldset drift.Note
gofmt/golangci-lintflag ~50 pre-existing files — the checkout is CRLF (core.autocrlf=true, no.gitattributes) while the tools want LF. That predates this branch and none of the files here are affected; worth a separate conversation.