feat(mcp): expose attachment upload and send#10564

🟠 High attachment/tools.ts:37

Following the PUT instruction in this description reaches no handler because attachmentUploadRouteLayer only registers POST for the returned upload URL. The staged file is therefore never uploaded, so the subsequent t3_thread_send_attachments call cannot send it; register a PUT handler or change the contract to match the route.

Fixed in bf8195852e913b0cefd21d9e2510d942de8066c7. The tool now says POST, matching attachmentUploadRouteLayer in apps/server/src/http.ts:402-404. The existing route consumes the raw request stream and returns 204 on successful storage; no new route or service behavior was needed. Targeted formatting/diff check, the shared core MCP tests (3/3), and canonical server typecheck pass.

claimPendingAttachments fails only with AttachmentClaimError, whose messages are caller-actionable validation results ("attachment not found", "stored size does not match", "attachment type does not match the upload"). Mapping them through unavailable collapses all of them into orchestration_error / "The operation could not be completed.", so the calling agent cannot tell a bad upload reference from a server fault. Consider mapping where the context is known, as readThread already does for thread-not-found:

^{Posted via Macroscope — Effect Service Conventions}

Fixed in dd7eca05adffd26e53763cd3ea083320b38f49c2 by retaining AttachmentClaimError.message but never its cause. The production claim service constructs these messages from bounded caller filenames and fixed explanations. I kept orchestration_error rather than mapping every error to invalid_request: copyFile failures and operational stat failures share this same typed channel, so that blanket classification would be inaccurate. This now gives the agent the actionable message without inventing a new service taxonomy. Shared tests/typecheck/lint pass.

🟠 High attachment/handlers.ts:38

Already-owned attachments can be dispatched with forged metadata, causing the provider to resolve an existing <id>.png as <id>.jpg (or deliver incorrect MIME metadata) and fail to read it. The check at line 38 retains only IDs and forwards the caller-supplied attachment unchanged; look up the matching attachment in projection and use its canonical fields, or require an exact metadata match.

No longer relevant as of 8a1460937c86baa3c41b0c036e1b2a92d107218d

Fixed in 8a1460937c. Already-owned IDs now resolve to the exact attachment object stored on the target projection before claim/send; caller-supplied MIME/name/size no longer replace the stored fields. Pending uploads keep the existing claim-service validation. A shared MCP boundary regression proves forged JPEG metadata returns the stored PNG attachment and an unknown ID rejects. Shared core 5/5, final core + real MCP integration 7/7, server typecheck and targeted lint/fmt/diff pass. No service implementation changed.

This ownership guard is new backend behavior (it is what stops an agent from re-sending another thread's attachment id) and nothing exercises it: no test invokes t3_thread_send_attachments, and the only test touched by this PR asserts tool-name uniqueness. Consider a focused handler test that drives the toolkit through the production registration — the same shape as toolkits/core.test.ts — with test layers for FileSystem/ThreadManagementService only, covering the pending-upload path, the already-owned path, and the rejected foreign-id path.

^{Posted via Macroscope — Effect Service Conventions}

The ownership boundary now has a focused shared MCP regression in 8a1460937c: it resolves stored canonical metadata and rejects an unknown/foreign attachment ID. Pending-upload validation remains in AttachmentClaims, whose behavior is unchanged. The final shared core + real MCP integration run passed 7/7. Per the maintainer's explicit direction, I am not adding a separate mocked filesystem/send suite for every forwarding tool; the new mapping itself is covered and existing service tests retain responsibility for upload/claim execution.

🟠 High attachment/handlers.ts:56

Sending attachments to an archived thread claims the pending uploads but leaves the claimed copies behind when sendToThread rejects the target. Repeating the request creates another UUID-named non-pending file each time, bypassing the pending sweep and eventually exhausting attachment storage; release the claims on this error path or reject archived threads before claimPendingAttachments.

Fixed in 8c58757ba6: the wrapper rejects an already-archived target immediately after the scoped target read, before resolving or claiming any attachments. This closes the reported repeated archived-target allocation path. Shared MCP core 5/5, server typecheck, targeted lint/fmt/diff pass. A later concurrent archive can still make dispatch fail; blanket cleanup is intentionally not used because send may have durably committed before a later observation fails. Precise accepted-versus-unaccepted cleanup belongs in the shared service, separately from this wrapper PR.

🟠 High attachment/handlers.ts:20

Pending attachment IDs are accepted with client-supplied type, contentType, and size, so a file upload can be dispatched as an image even when its upload authorization was for a different type. Because the pending branch returns attachment unchanged, resolve the ID against the server-side upload metadata and use that canonical metadata when constructing the claim.

This branch keeps pending uploads on the existing RPC claim path; it does not introduce a separate upload-type policy. The upload HTTP endpoint verifies the signed token and size, while AttachmentClaims validates the stored size and requires the path derived from the supplied type/name/MIME to match the actual staged path before copying. A different extension fails that check. There is no persisted upload-metadata record to resolve here: the metadata lives in the signed upload token. Image/file classification is not a separate MCP authorization capability, and RPC accepts the same references. Adding a persistent metadata record or a stricter MIME/content policy would be a shared upload-service change, not an MCP-only check. Keeping this extraction aligned with RPC; no new bypass was introduced by the shared intake change.