Attach images returned by MCP tools #2

Merged

mohlec merged 6 commits from slash/mcp-image-attachments into ncw/main 2026-08-10 01:06:13 +00:00

Conversation 0 Commits 6 Files changed 17 +322 -39

mohlec commented 2026-08-09 20:06:56 +00:00

Owner

Copy link

Deployed to CT#137 as 2.2.0+c126a1d3 and confirmed working end to end.

The problem

The MCP client extracted only TextContent, so image blocks were dropped and the agent posted an unreachable markdown URL instead of the picture.

The design that matters

Images are posted from the MCP tool resolver — the single point every tool call passes through — not handed to the conversation layer.

The first version did the latter, and it was wrong. Tools execute from at least six places (mention, DM, approval, tool follow-up, regeneration, and the non-post runner), each needing to remember to consume a stash. Three rounds of fixes each uncovered another path that did not, and every miss failed silently: tool succeeded, model described the picture, nothing posted. Diagnostics on the running server caught the last one directly — an image captured with no consumer.

Posting from the resolver means a new caller cannot miss it. llm.Context.ResponseRootID carries the thread; a path that forgets to set it posts to the channel rather than losing the image, so the failure mode is visible instead of silent.

Also fixed along the way

• 30s ceiling on every tool call. MCP used the shared untrusted HTTP client (httpservice.RequestTimeout). SDXL takes ~40s warm, so generate_image failed every time. MCP now has its own client with a 10 minute ceiling — still MakeClient(false), so AllowedUntrustedInternalConnections is still enforced.
• .jfif. mime.ExtensionsByType returns alphabetically, so image/jpeg yielded an extension Mattermost will not preview. Types are mapped explicitly, with a test pinning it.

Review notes

• The 10 minute MCP timeout is the riskiest change — a ceiling, not a target, but a wedged tool server can hold a request longer than before.
• Images now arrive before the model text, since the tool completes first.
• mmapi.Client gains UploadFile; mocks regenerated with mockery v3.2.5.

Operational gotchas (in slash-deploy RUNBOOK)

• "Failed to submit tool decisions" is usually a lie: approval runs the tool synchronously, so slow tools outlive the browser.
• Vision must be OFF for a text-only model, or images in thread history hard-fail every later turn.
• A small model may claim it generated an image without calling the tool at all. No MCP log line = no tool call.

**Deployed to CT#137 as `2.2.0+c126a1d3` and confirmed working end to end.** ### The problem The MCP client extracted only `TextContent`, so image blocks were dropped and the agent posted an unreachable markdown URL instead of the picture. ### The design that matters Images are posted **from the MCP tool resolver** — the single point every tool call passes through — not handed to the conversation layer. The first version did the latter, and it was wrong. Tools execute from at least six places (mention, DM, approval, tool follow-up, regeneration, and the non-post runner), each needing to remember to consume a stash. Three rounds of fixes each uncovered another path that did not, and **every miss failed silently**: tool succeeded, model described the picture, nothing posted. Diagnostics on the running server caught the last one directly — an image captured with no consumer. Posting from the resolver means a new caller cannot miss it. `llm.Context.ResponseRootID` carries the thread; a path that forgets to set it posts to the channel rather than losing the image, so the failure mode is visible instead of silent. ### Also fixed along the way - **30s ceiling on every tool call.** MCP used the shared untrusted HTTP client (`httpservice.RequestTimeout`). SDXL takes ~40s warm, so `generate_image` failed *every* time. MCP now has its own client with a 10 minute ceiling — still `MakeClient(false)`, so `AllowedUntrustedInternalConnections` is still enforced. - **`.jfif`.** `mime.ExtensionsByType` returns alphabetically, so `image/jpeg` yielded an extension Mattermost will not preview. Types are mapped explicitly, with a test pinning it. ### Review notes - **The 10 minute MCP timeout is the riskiest change** — a ceiling, not a target, but a wedged tool server can hold a request longer than before. - Images now arrive *before* the model text, since the tool completes first. - `mmapi.Client` gains `UploadFile`; mocks regenerated with mockery v3.2.5. ### Operational gotchas (in slash-deploy RUNBOOK) - "Failed to submit tool decisions" is usually a lie: approval runs the tool synchronously, so slow tools outlive the browser. - Vision must be OFF for a text-only model, or images in thread history hard-fail every later turn. - A small model may claim it generated an image without calling the tool at all. No MCP log line = no tool call.

mohlec added 4 commits 2026-08-09 20:06:56 +00:00

Attach images returned by MCP tools instead of dropping them ... 28d7bcd207

The MCP client extracted only TextContent from tool results, so image blocks
were silently discarded. Skynet's generate_image returns both an inline image
and a markdown URL; the agent therefore posted only the URL, which points at a
tailnet address no user's browser can reach — every generated image rendered
broken.

Images cannot ride back through a tool result, because that value is text fed to
the model. They also cannot be attached to the streamed reply: that post is
created empty before streaming starts, and Mattermost does not allow attachments
to be added to an existing post on update. So images are carried out-of-band and
posted as a threaded follow-up.

- mcp/client.go: collect ImageContent alongside text. An image-only result is no
  longer an error ("no text content found"), since that is a legitimate response.
- mcp/tool_images.go: stash images on llm.Context.Parameters, mirroring the
  existing web-search pattern. Unlike that one this is a true consume — the stash
  is cleared so a second read cannot post the same picture twice.
- mcp/user_clients.go: stash from the tool resolver; when a tool returns only an
  image, give the model a short text note so it knows what happened.
- conversations/handle_messages.go: after streaming, upload the images and post
  them in the thread. Upload failures are logged, not returned — the text answer
  has already been delivered and losing a picture should not fail the reply.
- mmapi: add UploadFile (thin wrapper over pluginAPI.File.Upload); mocks
  regenerated with mockery v3.2.5.

Not yet verified end to end against a real generate_image call.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Give MCP tool calls their own timeout instead of httpservice's 30s ... a4f0b491a3

MCP tool calls went through the shared untrusted HTTP client, whose
httpservice.RequestTimeout is 30 seconds. That is fine for a web request and
wrong for a tool call: generate_image on SDXL takes ~40s warm and minutes from
cold, so it failed every time with "request terminated without response" while
ComfyUI went on to finish the picture nobody received.

Use a dedicated client for the MCP client manager with a 10 minute ceiling,
matching what llmUpstreamHTTPClient already does for slow LLM requests. It is
still MakeClient(false), so MCP servers remain untrusted and
AllowedUntrustedInternalConnections is still enforced; only the deadline changes.
OAuth keeps the short-timeout client.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Post tool images from the approval and regeneration paths too ... cff582b26e

The first cut only consumed stashed images in handle_messages, which covers
auto-run tools. Tools set to "Ask Every Time" execute in tool_approval.go
instead, and that path builds its own llm.Context, so a generated image was
stashed on a context nobody read: the tool call succeeded, the model described
the picture, and no attachment appeared.

Consume and post in tool_approval.go after the approved tool calls resolve, and
in regeneration.go before the regenerated stream is returned. Verified against
the real server that skynet-mcp returns both blocks, so capture was never the
problem:
  BLOCK image | mime: image/jpeg | base64 len: 75620
  BLOCK text  | ![generated image](...)

conversations.go has a fourth tool runner used for non-post flows, where there
is no post to attach to; left alone deliberately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Name JPEG attachments .jpg so Mattermost previews them inline ... 9b7791d558

toolImageFileName asked mime.ExtensionsByType for an extension and took the
first result. That list is alphabetical, so image/jpeg produced ".jfif" -- a
valid JPEG extension that Mattermost does not recognise as previewable, so a
generated picture arrived as a download card instead of rendering in the thread.

Map the common image types explicitly and keep the lookup only as a fallback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mohlec added 1 commit 2026-08-09 20:11:15 +00:00

Post tool images from the follow-up and DM paths as well ... c536c10445

Auto-run tools produced no attachment: the tool succeeded, the model described
the picture, nothing was posted. The image was stashed in streamToolFollowUp --
the continuation round that runs after a tool result, which builds its own
llm.Context and was the one execution site still unpatched. Confirmed against
the database: exactly one uploaded file existed across the whole session, from
the approval path, and none from any auto-run attempt.

That is the fifth such site. Also patching handleDMViaConversation, which had
the same hole and would have dropped images in DMs.

Every path that runs tools and owns a post now consumes:
  handle_messages.go     mention responses, DM responses
  tool_approval.go       approved tool calls, follow-up continuation rounds
  regeneration.go        regenerated responses
conversations.go keeps the sixth runner for non-post flows, where there is no
post to attach to.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mohlec added 1 commit 2026-08-10 00:55:46 +00:00

Post tool images from the resolver, not from each caller ... c126a1d3c8

Handing images to the conversation layer to post was the wrong design. Tools
execute from at least six places -- mention, DM, approval, tool follow-up,
regeneration, and the non-post runner -- and every one had to remember to
consume the stash. Three rounds of fixes each found another path that did not,
and each miss failed silently: the tool succeeded, the model described the
picture, and nothing was posted. Diagnostics on the running server caught the
last one directly, an image captured with no consumer:

  DIAG mcp tool returned  tool=generate_image images=1 ctx_nil=false
  (no postToolImages entered)

Post from the tool resolver instead. Every tool call passes through it, so a new
caller cannot miss it, and the stash/consume plumbing disappears entirely.

llm.Context gains ResponseRootID so images thread with the reply. A path that
forgets to set it posts to the channel instead of losing the image -- the
failure mode is now visible rather than silent.

UserClients takes the plugin API (ClientManager already held it). Removes
ConsumeToolImages/StashToolImages, the five consume call sites, and the
conversation-layer helpers. Tests now cover the file naming, including that
image/jpeg must not become .jfif.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mohlec merged commit f544bffa61 into ncw/main 2026-08-10 01:06:13 +00:00

mohlec deleted branch slash/mcp-image-attachments 2026-08-10 01:06:14 +00:00

mohlec referenced this pull request from a commit 2026-08-10 01:06:15 +00:00

Merge pull request 'Attach images returned by MCP tools' (#2) from slash/mcp-image-attachments into ncw/main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

slash/slash-agents-plugin!2

No description provided.