fix(chat): address review blockers on chat reply attachments (MUL-4287)

Two final-review blockers on PR #5164:

1. Mobile inline dedup only checked raw `url`, so an attachment referenced
   inline via `markdown_url` (exactly what the CLI snippet emits) rendered
   twice — once inline, once as a standalone card. Reuse the core
   `contentReferencesAttachment` helper so dedup covers every real reference
   form (stable /api/attachments/<id>/download path, url, download_url,
   markdown_url), matching web's AttachmentList. Extracted the filter into a
   pure `lib/attachment-dedup.ts` so it is unit-testable, and added a
   regression test covering `content` containing `attachment.markdown_url`
   (plus the other URL forms and same-identity sibling dedup).

2. CLI `attachment upload` emitted `![...]` image markdown for every file,
   producing a broken-image snippet for non-images. Emit image markdown only
   for image/* content types and a plain link otherwise, with a CLI contract
   test for both.

Approved scope otherwise unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>

apps/mobile/components/issue/comment-attachment-list.tsx

@@ -25,6 +25,7 @@ import { useMemo } from "react";
 import { Linking, Pressable, View } from "react-native";
 import { Ionicons } from "@expo/vector-icons";
 import type { Attachment } from "@multica/core/types";
+import { standaloneAttachments } from "@/lib/attachment-dedup";
 import { MarkdownImage } from "@/lib/markdown/markdown-image";
 import { resolveAttachmentUrl } from "@/lib/attachment-url";
 import { useColorScheme } from "@/lib/use-color-scheme";
@@ -45,30 +46,14 @@ export function CommentAttachmentList({ attachments, content }: Props) {
   const { colorScheme } = useColorScheme();
   const theme = THEME[colorScheme];
 
-  const standalone = useMemo(() => {
-    if (!attachments || attachments.length === 0) return [];
-    if (!content) return attachments;
-    return attachments.filter((a) => {
-      // Skip attachments whose URL is already referenced inline in the
-      // markdown — they'll render via MarkdownImage (images) or a markdown
-      // link (files), and we'd otherwise show them twice.
-      if (content.includes(a.url)) return false;
-      // Dedup: if another attachment with the same file identity (name,
-      // type, size) is already inline in the content, this one is a
-      // duplicate upload — skip it. Mirrors web's
-      // `comment-card.tsx:132-140` defense.
-      const hasSiblingInContent = attachments.some(
-        (other) =>
-          other.id !== a.id &&
-          other.filename === a.filename &&
-          other.content_type === a.content_type &&
-          other.size_bytes === a.size_bytes &&
-          content.includes(other.url),
-      );
-      if (hasSiblingInContent) return false;
-      return true;
-    });
-  }, [attachments, content]);
+  // Only render attachments not already referenced inline in the body. The
+  // dedup lives in a pure helper (lib/attachment-dedup) so it can be unit
+  // tested; it matches every real URL form the server emits (stable path /
+  // url / download_url / markdown_url), mirroring web's AttachmentList.
+  const standalone = useMemo(
+    () => standaloneAttachments(attachments, content),
+    [attachments, content],
+  );
 
   if (standalone.length === 0) return null;
 

apps/mobile/lib/attachment-dedup.test.ts

@@ -0,0 +1,67 @@
+import { describe, expect, it } from "vitest";
+import type { Attachment } from "@multica/core/types";
+import { standaloneAttachments } from "./attachment-dedup";
+
+function att(over: Partial<Attachment> = {}): Attachment {
+  return {
+    id: "att-1",
+    workspace_id: "ws-1",
+    issue_id: null,
+    comment_id: null,
+    chat_session_id: null,
+    chat_message_id: null,
+    uploader_type: "agent",
+    uploader_id: "agent-1",
+    filename: "chart.png",
+    url: "https://cdn.example/chart.png",
+    download_url: "https://signed.example/chart.png?sig=x",
+    markdown_url: "https://public.example/api/attachments/att-1/download",
+    content_type: "image/png",
+    size_bytes: 123,
+    created_at: "2026-07-09T00:00:00Z",
+    ...over,
+  };
+}
+
+describe("standaloneAttachments", () => {
+  it("excludes an attachment referenced inline via markdown_url (the CLI snippet form)", () => {
+    const a = att();
+    const content = `see the result\n\n![chart.png](${a.markdown_url})`;
+    expect(standaloneAttachments([a], content)).toEqual([]);
+  });
+
+  it("excludes an attachment referenced inline via the stable download path", () => {
+    const a = att();
+    const content = `![chart.png](/api/attachments/${a.id}/download)`;
+    expect(standaloneAttachments([a], content)).toEqual([]);
+  });
+
+  it("excludes an attachment referenced inline via raw url", () => {
+    const a = att();
+    expect(standaloneAttachments([a], `![x](${a.url})`)).toEqual([]);
+  });
+
+  it("excludes an attachment referenced inline via signed download_url", () => {
+    const a = att();
+    expect(standaloneAttachments([a], `![x](${a.download_url})`)).toEqual([]);
+  });
+
+  it("keeps an attachment that is not referenced anywhere in the body", () => {
+    const a = att();
+    expect(standaloneAttachments([a], "just some text, no image")).toEqual([a]);
+  });
+
+  it("renders every attachment when content is undefined (no body to reference them)", () => {
+    const a = att();
+    expect(standaloneAttachments([a], undefined)).toEqual([a]);
+  });
+
+  it("drops a duplicate upload whose same-identity sibling is inline via markdown_url", () => {
+    const inline = att({ id: "att-inline" });
+    const dup = att({ id: "att-dup", url: "https://cdn.example/other.png" });
+    // Only the sibling's markdown_url appears in the body; the dup shares
+    // filename/type/size, so it must be treated as the same file and dropped.
+    const content = `![chart.png](${inline.markdown_url})`;
+    expect(standaloneAttachments([inline, dup], content)).toEqual([]);
+  });
+});

apps/mobile/lib/attachment-dedup.ts

@@ -0,0 +1,42 @@
+import type { Attachment } from "@multica/core/types";
+import { contentReferencesAttachment } from "@multica/core/types";
+
+/**
+ * The attachments to render as standalone cards below a message / comment
+ * body: the ones NOT already referenced inline in `content`, minus duplicate
+ * uploads whose same-identity sibling is already inline.
+ *
+ * "Referenced inline" is decided by the core `contentReferencesAttachment`
+ * helper, so it matches every real URL form the server emits — the stable
+ * `/api/attachments/<id>/download` path, the raw storage `url`, the signed
+ * `download_url`, and the durable `markdown_url` that agents actually paste
+ * into a reply. Checking only raw `url` (the old behaviour) missed the
+ * `markdown_url` case and rendered the same image twice.
+ *
+ * Pass `content === undefined` (not "") to render every attachment — used when
+ * the body has no markdown that could reference them.
+ *
+ * Mirrors web's `AttachmentList` filter in
+ * `packages/views/issues/components/comment-card.tsx`.
+ */
+export function standaloneAttachments(
+  attachments: Attachment[] | undefined,
+  content: string | undefined,
+): Attachment[] {
+  if (!attachments || attachments.length === 0) return [];
+  if (!content) return attachments;
+  return attachments.filter((a) => {
+    if (contentReferencesAttachment(content, a)) return false;
+    // Dedup: if another attachment with the same file identity (name, type,
+    // size) is already inline, this one is a duplicate upload — skip it.
+    const hasSiblingInContent = attachments.some(
+      (other) =>
+        other.id !== a.id &&
+        other.filename === a.filename &&
+        other.content_type === a.content_type &&
+        other.size_bytes === a.size_bytes &&
+        contentReferencesAttachment(content, other),
+    );
+    return !hasSiblingInContent;
+  });
+}

server/cmd/multica/cmd_attachment.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -90,7 +91,13 @@ func runAttachmentUpload(cmd *cobra.Command, args []string) error {
 	}
 
 	filename := filepath.Base(path)
-	markdown := fmt.Sprintf("![%s](%s)", filename, att.MarkdownURL)
+	// Image content types get image markdown so they render inline in the
+	// reply; every other file gets a normal link so it shows as a proper file
+	// reference instead of a broken-image icon.
+	markdown := fmt.Sprintf("[%s](%s)", filename, att.MarkdownURL)
+	if strings.HasPrefix(att.ContentType, "image/") {
+		markdown = fmt.Sprintf("![%s](%s)", filename, att.MarkdownURL)
+	}
 	fmt.Fprintln(os.Stderr, "Uploaded:", filename)
 
 	return cli.PrintJSON(os.Stdout, map[string]any{

server/cmd/multica/cmd_attachment_test.go

@@ -99,6 +99,7 @@ func TestRunAttachmentUploadSendsTaskIDAndPrintsContract(t *testing.T) {
 		_ = json.NewEncoder(w).Encode(map[string]any{
 			"id":           "att-999",
 			"filename":     "chart.png",
+			"content_type": "image/png",
 			"url":          "https://cdn.example/chart.png",
 			"download_url": "https://signed.example/chart.png?sig=x",
 			"markdown_url": "https://public.example/api/attachments/att-999/download",
@@ -136,8 +137,50 @@ func TestRunAttachmentUploadSendsTaskIDAndPrintsContract(t *testing.T) {
 	if !strings.Contains(out, `"markdown_url": "https://public.example/api/attachments/att-999/download"`) {
 		t.Fatalf("stdout missing markdown_url: %q", out)
 	}
+	// Image content type → image markdown so it renders inline.
 	if !strings.Contains(out, `![chart.png](https://public.example/api/attachments/att-999/download)`) {
-		t.Fatalf("stdout missing markdown snippet: %q", out)
+		t.Fatalf("stdout missing image markdown snippet: %q", out)
+	}
+}
+
+// TestRunAttachmentUploadNonImageUsesLinkMarkdown covers the content-type
+// branch: a non-image file must emit a plain link, not `![...]`, which would
+// render as a broken image in the reply.
+func TestRunAttachmentUploadNonImageUsesLinkMarkdown(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if err := r.ParseMultipartForm(1 << 20); err != nil {
+			t.Fatalf("parse multipart: %v", err)
+		}
+		_ = json.NewEncoder(w).Encode(map[string]any{
+			"id":           "att-doc",
+			"filename":     "report.pdf",
+			"content_type": "application/pdf",
+			"url":          "https://cdn.example/report.pdf",
+			"markdown_url": "https://public.example/api/attachments/att-doc/download",
+		})
+	}))
+	defer srv.Close()
+	setCLITestServerEnv(t, srv.URL)
+	t.Setenv("MULTICA_TOKEN", "mat_test-token")
+
+	dir := t.TempDir()
+	docPath := filepath.Join(dir, "report.pdf")
+	if err := os.WriteFile(docPath, []byte("%PDF-1.4 bytes"), 0o644); err != nil {
+		t.Fatalf("write temp doc: %v", err)
+	}
+
+	cmd := newAttachmentUploadTestCmd()
+	_ = cmd.Flags().Set("task", "task-abc")
+
+	out, err := captureStdout(t, func() error { return runAttachmentUpload(cmd, []string{docPath}) })
+	if err != nil {
+		t.Fatalf("runAttachmentUpload: %v", err)
+	}
+	if !strings.Contains(out, `[report.pdf](https://public.example/api/attachments/att-doc/download)`) {
+		t.Fatalf("stdout missing link markdown: %q", out)
+	}
+	if strings.Contains(out, `![report.pdf]`) {
+		t.Fatalf("non-image must not use image markdown: %q", out)
 	}
 }