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

merged6
agents/mcp-thin/projects
agents/mcp-thin/attachments
openedSep 7, 2026byJJulius Marminge
View on GitHubTwo-way sync enabledPR last imported 3d ago
6 unresolved threads · Jump to file
Selected thread: apps/server/src/mcp/toolkits/attachment/handlers.ts:56 — 🟠 **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`. <details> <summary>🚀 Reply "<strong>fix it for me</strong>" or copy this <strong>AI Prompt</strong> for your agent:</summary> ```text In file @apps/server/src/mcp/toolkits/attachment/handlers.ts around line 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`. ``` </details> <!-- macroscope-meta: {"kind":"code_review","variant":"195358a06841b2d4d03feb0d212d1ca7"} -->
0 / 5 viewed5 files changedLine totals unavailable

apps/server/src/mcp/toolkits/attachment/handlers.ts · Earlier revision 8a146093 · right 56

Original diff
@@ -0,0 +1,89 @@
+import { type ChatAttachment, MessageId, OrchestratorMcpFailure } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+import * as Upload from "../../../assets/AttachmentUpload.ts";
+import * as Claims from "../../../orchestration-v2/AttachmentClaims.ts";
+import {
+  newCommandId,
+  readMutationCaller,
+  readWritableThread,
+  unavailable,
+} from "../../threadAccess.ts";
+import { AttachmentToolkit } from "./tools.ts";
+
+export function resolveAttachmentReferences(
+  requested: ReadonlyArray<ChatAttachment>,
+  stored: ReadonlyArray<ChatAttachment>,
+) {
+  const owned = new Map(stored.map((attachment) => [attachment.id, attachment]));
+  return Effect.forEach(requested, (attachment) => {
+    const canonical = Claims.attachmentIsPendingUpload(attachment)
+      ? attachment
+      : owned.get(attachment.id);
+    return canonical === undefined
+      ? Effect.fail(
+          new OrchestratorMcpFailure({
+            code: "invalid_request",
+            message: "Attachments must be pending uploads or already belong to the target thread.",
+          }),
+        )
+      : Effect.succeed(canonical);
+  });
+}
+
+export const AttachmentHandlersLive = AttachmentToolkit.toLayer({
+  t3_attachment_prepare_upload: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      return yield* Upload.issueAttachmentUploadUrl(input.upload).pipe(
+        Effect.mapError(unavailable),
+      );
+    }),
+  t3_attachment_discard: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      yield* Upload.deletePendingAttachment(input.attachmentId);
+      return {};
+    }),
+  t3_thread_send_attachments: (input) =>
+    Effect.gen(function* () {
+      const { caller, threads, projection } = yield* readWritableThread(input.threadId);
+      const attachments = yield* resolveAttachmentReferences(
+        input.attachments,
+        projection.messages.flatMap((message) => message.attachments),
+      );
+      const commandId = yield* newCommandId();
+      const messageId = MessageId.make(commandId);
+      const claimed = yield* Claims.claimPendingAttachments({
M
macroscopeappBotSep 7, 2026

🟠 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.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
text
1In file @apps/server/src/mcp/toolkits/attachment/handlers.ts around line 56:
2
3Sending 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`.
J
Julius MarmingeSep 7, 2026

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.

apps/server/src/mcp/toolkits/attachment/handlers.ts · Earlier revision dd7eca05 · right 38

Original diff
@@ -0,0 +1,80 @@
+import { MessageId, OrchestratorMcpFailure } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+import * as Upload from "../../../assets/AttachmentUpload.ts";
+import * as Claims from "../../../orchestration-v2/AttachmentClaims.ts";
+import {
+  newCommandId,
+  readMutationCaller,
+  readWritableThread,
+  unavailable,
+} from "../../threadAccess.ts";
+import { AttachmentToolkit } from "./tools.ts";
+
+export const AttachmentHandlersLive = AttachmentToolkit.toLayer({
+  t3_attachment_prepare_upload: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      return yield* Upload.issueAttachmentUploadUrl(input.upload).pipe(
+        Effect.mapError(unavailable),
+      );
+    }),
+  t3_attachment_discard: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      yield* Upload.deletePendingAttachment(input.attachmentId);
+      return {};
+    }),
+  t3_thread_send_attachments: (input) =>
+    Effect.gen(function* () {
+      const { caller, threads, projection } = yield* readWritableThread(input.threadId);
+      const owned = new Set(
+        projection.messages.flatMap((message) =>
+          message.attachments.map((attachment) => attachment.id),
+        ),
+      );
+      if (
+        input.attachments.some(
+          (attachment) =>
+            !Claims.attachmentIsPendingUpload(attachment) && !owned.has(attachment.id),
M
macroscopeappBotSep 7, 2026

🟠 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

J
Julius MarmingeSep 7, 2026

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.

apps/server/src/mcp/toolkits/attachment/handlers.ts · Earlier revision 11253422 · right 20

Original diff
@@ -0,0 +1,86 @@
+import { type ChatAttachment, MessageId, OrchestratorMcpFailure } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+import * as Upload from "../../../assets/AttachmentUpload.ts";
+import * as Claims from "../../../orchestration-v2/AttachmentClaims.ts";
+import * as ThreadMessageIntake from "../../../orchestration-v2/ThreadMessageIntake.ts";
+import {
+  newCommandId,
+  readMutationCaller,
+  readWritableThread,
+  unavailable,
+} from "../../threadAccess.ts";
+import { AttachmentToolkit } from "./tools.ts";
+
+export function resolveAttachmentReferences(
+  requested: ReadonlyArray<ChatAttachment>,
+  stored: ReadonlyArray<ChatAttachment>,
+) {
+  const owned = new Map(stored.map((attachment) => [attachment.id, attachment]));
+  return Effect.forEach(requested, (attachment) => {
+    const canonical = Claims.attachmentIsPendingUpload(attachment)
M
macroscopeappBotSep 7, 2026

🟠 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.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
text
1In file @apps/server/src/mcp/toolkits/attachment/handlers.ts around line 20:
2
3Pending 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.
J
Julius MarmingeSep 7, 2026

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.

apps/server/src/mcp/toolkits/attachment/tools.ts · Earlier revision 0024ed0c · right 37

Original diff
@@ -0,0 +1,73 @@
+import {
+  AttachmentCreateUploadUrlInput,
+  AttachmentCreateUploadUrlResult,
+  AttachmentDeleteInput,
+  ChatImageAttachment,
+  ChatFileAttachment,
+  MessageId,
+  RunId,
+  ThreadId,
+  OrchestrationV2RunStatus,
+  OrchestratorMcpFailure,
+} from "@t3tools/contracts";
+import * as Crypto from "effect/Crypto";
+import * as FileSystem from "effect/FileSystem";
+import * as Schema from "effect/Schema";
+import { Tool, Toolkit } from "effect/unstable/ai";
+import { ServerSecretStore } from "../../../auth/ServerSecretStore.ts";
+import { ServerConfig } from "../../../config.ts";
+import { ThreadManagementService } from "../../../orchestration-v2/ThreadManagementService.ts";
+import { McpInvocationContext } from "../../McpInvocationContext.ts";
+
+const shared = {
+  failure: OrchestratorMcpFailure,
+  failureMode: "return" as const,
+  dependencies: [
+    McpInvocationContext,
+    ThreadManagementService,
+    ServerConfig,
+    ServerSecretStore,
+    FileSystem.FileSystem,
+    Crypto.Crypto,
+  ],
+};
+export const AttachmentUploadTool = Tool.make("t3_attachment_prepare_upload", {
+  ...shared,
+  description:
+    "Create the app's signed upload URL. PUT the exact bytes to relativeUrl on this MCP server's HTTP origin, then pass the attachment metadata and returned ID to t3_thread_send_attachments. Upload is separate from sending; provider attachment support is decided by its adapter.",
M
macroscopeappBotSep 7, 2026

🟠 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.

Also found in 1 other location(s)

apps/server/src/mcp/McpHttpServer.ts:277

Registering AttachmentRegistrationLive exposes t3_attachment_prepare_upload, whose tool description directs clients to PUT the upload bytes, but attachmentUploadRouteLayer is registered only for POST at that URL. A client following the newly exposed tool contract therefore cannot upload the staged file and cannot subsequently send the attachment.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
text
1In file @apps/server/src/mcp/toolkits/attachment/tools.ts around line 37:
2
3Following 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.
4
5Also found in 1 other location(s):
6- apps/server/src/mcp/McpHttpServer.ts:277 -- Registering `AttachmentRegistrationLive` exposes `t3_attachment_prepare_upload`, whose tool description directs clients to `PUT` the upload bytes, but `attachmentUploadRouteLayer` is registered only for `POST` at that URL. A client following the newly exposed tool contract therefore cannot upload the staged file and cannot subsequently send the attachment.
J
Julius MarmingeSep 7, 2026

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.

apps/server/src/mcp/toolkits/attachment/handlers.ts · Earlier revision dd7eca05 · right 35–44

Original diff
@@ -0,0 +1,80 @@
+import { MessageId, OrchestratorMcpFailure } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+import * as Upload from "../../../assets/AttachmentUpload.ts";
+import * as Claims from "../../../orchestration-v2/AttachmentClaims.ts";
+import {
+  newCommandId,
+  readMutationCaller,
+  readWritableThread,
+  unavailable,
+} from "../../threadAccess.ts";
+import { AttachmentToolkit } from "./tools.ts";
+
+export const AttachmentHandlersLive = AttachmentToolkit.toLayer({
+  t3_attachment_prepare_upload: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      return yield* Upload.issueAttachmentUploadUrl(input.upload).pipe(
+        Effect.mapError(unavailable),
+      );
+    }),
+  t3_attachment_discard: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      yield* Upload.deletePendingAttachment(input.attachmentId);
+      return {};
+    }),
+  t3_thread_send_attachments: (input) =>
+    Effect.gen(function* () {
+      const { caller, threads, projection } = yield* readWritableThread(input.threadId);
+      const owned = new Set(
+        projection.messages.flatMap((message) =>
+          message.attachments.map((attachment) => attachment.id),
+        ),
+      );
+      if (
+        input.attachments.some(
+          (attachment) =>
+            !Claims.attachmentIsPendingUpload(attachment) && !owned.has(attachment.id),
+        )
+      )
+        return yield* new OrchestratorMcpFailure({
+          code: "invalid_request",
+          message: "Attachments must be pending uploads or already belong to the target thread.",
+        });

apps/server/src/mcp/toolkits/attachment/handlers.ts · right 35–44

M
macroscopeappBotSep 7, 2026

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

J
Julius MarmingeSep 7, 2026

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.

apps/server/src/mcp/toolkits/attachment/handlers.ts · Earlier revision bf819585 · right 47–50

Original diff
@@ -0,0 +1,72 @@
+import { MessageId, OrchestratorMcpFailure } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+import * as Upload from "../../../assets/AttachmentUpload.ts";
+import * as Claims from "../../../orchestration-v2/AttachmentClaims.ts";
+import {
+  newCommandId,
+  readMutationCaller,
+  readWritableThread,
+  unavailable,
+} from "../../threadAccess.ts";
+import { AttachmentToolkit } from "./tools.ts";
+
+export const AttachmentHandlersLive = AttachmentToolkit.toLayer({
+  t3_attachment_prepare_upload: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      return yield* Upload.issueAttachmentUploadUrl(input.upload).pipe(
+        Effect.mapError(unavailable),
+      );
+    }),
+  t3_attachment_discard: (input) =>
+    Effect.gen(function* () {
+      yield* readMutationCaller();
+      yield* Upload.deletePendingAttachment(input.attachmentId);
+      return {};
+    }),
+  t3_thread_send_attachments: (input) =>
+    Effect.gen(function* () {
+      const { caller, threads, projection } = yield* readWritableThread(input.threadId);
+      const owned = new Set(
+        projection.messages.flatMap((message) =>
+          message.attachments.map((attachment) => attachment.id),
+        ),
+      );
+      if (
+        input.attachments.some(
+          (attachment) =>
+            !Claims.attachmentIsPendingUpload(attachment) && !owned.has(attachment.id),
+        )
+      )
+        return yield* new OrchestratorMcpFailure({
+          code: "invalid_request",
+          message: "Attachments must be pending uploads or already belong to the target thread.",
+        });
+      const commandId = yield* newCommandId();
+      const messageId = MessageId.make(commandId);
+      const claimed = yield* Claims.claimPendingAttachments({
+        threadId: projection.thread.id,
+        attachments: input.attachments,
+      }).pipe(Effect.mapError(unavailable));

apps/server/src/mcp/toolkits/attachment/handlers.ts · right 47–50

M
macroscopeappBotSep 7, 2026

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:

suggestion
1 const claimed = yield* Claims.claimPendingAttachments({
2 threadId: projection.thread.id,
3 attachments: input.attachments,
4 }).pipe(
5 Effect.catchTags({
6 AttachmentClaimError: (error) =>
7 Effect.fail(
8 new OrchestratorMcpFailure({ code: "invalid_request", message: error.message }),
9 ),
10 }),
11 );

Posted via Macroscope — Effect Service Conventions

J
Julius MarmingeSep 7, 2026

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.