fix(mcp): name chats in list_chats the way the UI does, not an 8-char id slice (#614)

.changeset/tidy-moons-invent.md

@@ -0,0 +1,22 @@
+---
+"@paddock/server": patch
+---
+
+`list_chats` now names a chat the way the web UI does, instead of falling through to an 8-character sessionId slice (#614).
+
+The MCP projection used `customName ?? autoName ?? sessionId.slice(0, 8)` and
+omitted the `preview` step the REST DTO has. Claude writes `ai-title` records
+rather than the `summary` records `autoName` is derived from, so `autoName` is
+almost never set and the chain went straight to the slice — **39 of 96 chats
+(41%)** on a live instance came back named after their own sessionId.
+
+That is worse than ugly: the stub *looks* like an id, and feeding it back to
+`read_chat` returns a successful empty result rather than an error, so a caller
+can conclude "empty chat" about a conversation it never opened.
+
+The preview-recovery logic (which unwraps a preload-polluted first message back
+to the user's real request) is now a shared `recoverPreview` helper used by both
+`chat-dto` and the MCP list, so the two surfaces can't drift on this again.
+
+Note this does not make `autoName` itself work — reading `ai-title` is the
+remaining half of #614 and lives in `@herdctl/core`.

packages/server/src/chat-dto.ts

@@ -285,6 +285,38 @@ export function toChatDto(
   };
 }
 
+/**
+ * Recover a usable preview when the raw one is a machine-prepended wrapper: the
+ * preload context block (#1/#62) and/or the composer-attachment block (#328).
+ * Either makes the first message a poor display name, so we read the untruncated
+ * first user message and strip the wrapper back to the user's actual request.
+ *
+ * Returns `undefined` when there is nothing better than `s.preview` — either the
+ * preview was never polluted, or nothing survived the strip. Only polluted chats
+ * pay for the extra (head-of-file) read; everything else returns without I/O.
+ *
+ * Shared by the REST DTO and the MCP `list_chats` projection (#614) so the two
+ * surfaces cannot drift on what a chat is called. They did: the MCP list omitted
+ * the preview step entirely and fell straight through to an 8-char sessionId.
+ */
+export async function recoverPreview(
+  projectDir: string,
+  s: DiscoveredSession,
+): Promise<string | undefined> {
+  const polluted =
+    !s.customName &&
+    !s.autoName &&
+    (s.preview?.startsWith(PRELOAD_CONTEXT_OPEN) || s.preview?.startsWith(ATTACHMENTS_OPEN));
+  if (!polluted) return undefined;
+
+  const full = await readFirstUserText(projectDir, s.sessionId).catch(() => undefined);
+  // Strip preload FIRST (it wraps the whole thing), then the attachment block
+  // nested inside it, leaving just the user's typed request.
+  const cleaned = stripAttachmentsWrapper(stripPreloadWrapper(full ?? s.preview ?? "")).trim();
+  if (!cleaned) return undefined; // couldn't recover
+  return cleaned.length > PREVIEW_MAX ? `${cleaned.slice(0, PREVIEW_MAX)}...` : cleaned;
+}
+
 /**
  * Build the chat DTOs for a PROJECT's sessions, cleaning names polluted by the
  * preload wrapper (issue #62). When a chat has no better name (no user rename,
@@ -318,25 +350,9 @@ export async function buildProjectChats(
       const starred = starredOf ? await starredOf(s).catch(() => false) : false;
       const unread = unreadOf ? await unreadOf(s).catch(() => false) : false;
       const parent = parentOf ? await parentOf(s).catch(() => null) : null;
-      // A preview polluted by a machine-prepended wrapper: the preload context
-      // block (#1) and/or the composer-attachment block (#328). Either makes the
-      // raw first message a poor display name, so recover the real request below.
-      const pollutedPreview =
-        !s.customName &&
-        !s.autoName &&
-        (s.preview?.startsWith(PRELOAD_CONTEXT_OPEN) || s.preview?.startsWith(ATTACHMENTS_OPEN));
-      if (!pollutedPreview)
-        return toChatDto(s, undefined, usage, archived, turnAt, lastSeen, provenance, trigger, starred, unread, parent);
-
-      const full = await readFirstUserText(projectDir, s.sessionId).catch(() => undefined);
-      // Strip preload FIRST (it wraps the whole thing), then the attachment block
-      // nested inside it, leaving just the user's typed request.
-      const cleaned = stripAttachmentsWrapper(stripPreloadWrapper(full ?? s.preview ?? "")).trim();
-      // couldn't recover
-      if (!cleaned)
-        return toChatDto(s, undefined, usage, archived, turnAt, lastSeen, provenance, trigger, starred, unread, parent);
-      const preview =
-        cleaned.length > PREVIEW_MAX ? `${cleaned.slice(0, PREVIEW_MAX)}...` : cleaned;
+      // A preview polluted by a machine-prepended wrapper is recovered back to
+      // the user's real request; anything else maps straight through.
+      const preview = await recoverPreview(projectDir, s);
       return toChatDto(s, preview, usage, archived, turnAt, lastSeen, provenance, trigger, starred, unread, parent);
     }),
   );

packages/server/src/management-ops.ts

@@ -34,6 +34,7 @@
  */
 import type { ChatHandlerContext } from "./ws-context.js";
 import { keeperAgentName } from "./herdctl.js";
+import { recoverPreview } from "./chat-dto.js";
 import { isKnownModel, isKnownDriveMode, type DriveMode } from "./models.js";
 import type {
   SelfMcpContext,
@@ -196,7 +197,17 @@ export function buildManagementOps(
           chats.push({
             project: p.slug,
             sessionId: s.sessionId,
-            name: s.customName ?? s.autoName ?? s.sessionId.slice(0, 8),
+            // #614: same fallback chain the web UI uses (`chat-dto.ts`), preview
+            // step included. Without it this dropped straight to an 8-char
+            // sessionId slice for any chat Claude hasn't titled — 41% of them on
+            // this instance — so the MCP list and the UI disagreed about what a
+            // chat is called, and callers got a stub that merely LOOKS like an id.
+            name:
+              s.customName ??
+              s.autoName ??
+              (await recoverPreview(p.dir, s)) ??
+              s.preview ??
+              s.sessionId.slice(0, 8),
             updatedAt: s.mtime,
             running: hub.isRunning(s.sessionId),
             // #489: the archived flag the web UI has always had (`chat-dto.ts`) but

packages/server/test/unit/management-ops-chat-name.test.ts

@@ -0,0 +1,119 @@
+/**
+ * #614 — `list_chats` must name a chat the way the web UI names it.
+ *
+ * The MCP projection used `customName ?? autoName ?? sessionId.slice(0, 8)`,
+ * omitting the `preview` step the REST DTO has (`chat-dto.ts`). Claude writes
+ * `ai-title` records rather than the `summary` records `autoName` is derived
+ * from, so `autoName` is almost never set in practice and the chain fell
+ * straight through to the slice: 39 of 96 chats (41%) on the live instance came
+ * back named after their own sessionId.
+ *
+ * That is worse than ugly. The stub LOOKS like an id, and feeding it back to
+ * `read_chat` returns a successful empty result rather than an error, so a
+ * caller can conclude "empty chat" about a conversation it never opened.
+ */
+import { describe, it, expect } from "vitest";
+import { buildManagementOps } from "../../src/management-ops.js";
+import { wrapPreload } from "../../src/preload.js";
+import type { ChatHandlerContext } from "../../src/ws-context.js";
+
+type StubSession = {
+  sessionId: string;
+  customName?: string | null;
+  autoName?: string | null;
+  preview?: string;
+  mtime?: string;
+};
+
+/** A context stubbed down to just what `listChats` touches. */
+function ctxWith(sessions: StubSession[]) {
+  const project = { slug: "alpha", name: "Alpha", dir: "/p/alpha", workingDir: "/p/alpha" };
+  return {
+    deps: {
+      projects: { get: async () => project, list: async () => [project] },
+      herdctl: { listSessions: async () => sessions },
+      archive: { isArchived: async () => false },
+    },
+    hub: { isRunning: () => false },
+  } as unknown as ChatHandlerContext;
+}
+
+const listChats = async (sessions: StubSession[]) => {
+  const ops = buildManagementOps(ctxWith(sessions), {
+    currentProjectSlug: "alpha",
+    currentSessionId: () => null,
+    includeWrite: false,
+    includeTriggers: false,
+    includeProjects: false,
+  } as Parameters<typeof buildManagementOps>[1]);
+  return ops.read.listChats(undefined);
+};
+
+describe("#614: list_chats names chats the way the UI does", () => {
+  it("prefers a user-set customName", async () => {
+    const [chat] = await listChats([
+      { sessionId: "aaaaaaaa-1111-2222-3333-444444444444", customName: "Manager", preview: "hi" },
+    ]);
+    expect(chat.name).toBe("Manager");
+  });
+
+  it("falls back to autoName before the preview", async () => {
+    const [chat] = await listChats([
+      {
+        sessionId: "bbbbbbbb-1111-2222-3333-444444444444",
+        autoName: "Fix the flaky suite",
+        preview: "some first message",
+      },
+    ]);
+    expect(chat.name).toBe("Fix the flaky suite");
+  });
+
+  // The regression this issue is about: an untitled chat used to become an
+  // 8-hex stub even though its first message was right there.
+  it("uses the preview when there is no stored title, NOT a sessionId slice", async () => {
+    const [chat] = await listChats([
+      {
+        sessionId: "cccccccc-1111-2222-3333-444444444444",
+        preview: "Audit the Night-Watch run against the JSONL",
+      },
+    ]);
+    expect(chat.name).toBe("Audit the Night-Watch run against the JSONL");
+    expect(chat.name).not.toBe("cccccccc");
+  });
+
+  it("still slices the sessionId when there is genuinely nothing to show", async () => {
+    const [chat] = await listChats([{ sessionId: "dddddddd-1111-2222-3333-444444444444" }]);
+    expect(chat.name).toBe("dddddddd");
+  });
+
+  // A preload-wrapped first message would otherwise name the chat after the
+  // injected OVERVIEW/CHANGELOG block (#62). Both surfaces run the same
+  // recovery, so the name is the user's actual request.
+  it("recovers the real request from a preload-wrapped preview", async () => {
+    const [chat] = await listChats([
+      {
+        sessionId: "eeeeeeee-1111-2222-3333-444444444444",
+        preview: wrapPreload("# OVERVIEW\n\nproject state", "please bump the deps"),
+      },
+    ]);
+    expect(chat.name).toBe("please bump the deps");
+  });
+
+  // Parity check, warts included: when the wrapper is truncated before the
+  // `My request:` marker and the untruncated message can't be read from disk,
+  // there is nothing to recover and `chat-dto` also falls through to the raw
+  // preview. Matching that is the point of #614 — the two surfaces must not
+  // disagree — so this pins the shared behaviour rather than quietly diverging.
+  it("matches the DTO's fallback when a truncated wrapper can't be recovered", async () => {
+    const truncated = "<project-context>\n# OVERVIEW\n\nproject state that got cut off";
+    const [chat] = await listChats([
+      { sessionId: "99999999-1111-2222-3333-444444444444", preview: truncated },
+    ]);
+    expect(chat.name).toBe(truncated);
+  });
+
+  it("returns the FULL sessionId regardless of what the name resolved to", async () => {
+    const [chat] = await listChats([{ sessionId: "ffffffff-1111-2222-3333-444444444444" }]);
+    expect(chat.sessionId).toBe("ffffffff-1111-2222-3333-444444444444");
+  });
+});