From 612c4e311a4a5d39541882642ee2e2d99bb8f65a Mon Sep 17 00:00:00 2001 From: HomeLab Agent Date: Sun, 2 Aug 2026 14:02:00 -0400 Subject: [PATCH] fix(mcp): name chats in list_chats the way the UI does, not an 8-char id slice (#614) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP projection resolved a chat's name as `customName ?? autoName ?? sessionId.slice(0, 8)`, omitting the `preview` step the REST DTO has (`chat-dto.ts:241`). So for the same chat the web UI showed prose and MCP showed a hash. `autoName` cannot cover the gap: core derives it only from `type:"summary"` records, and current Claude Code writes `ai-title` instead. Across the 1,704 transcripts on this instance, 1,649 carry an `ai-title` and 0 carry a `summary` — so the chain effectively ran `customName ?? slice(0,8)` and 39 of 96 listed chats (41%) came back named after their own sessionId. That is worse than ugly. The stub LOOKS like an id, and feeding one 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. It also silently broke a documented instruction: the Night-Watch reviewer is told to skip chats whose name contains "night-watch", and the prior run's actual title is "Night-Watch nightly quality review system" — the filter could not fire, two nights running, and it burned a read re-identifying the chat by content instead. - Extract the preload/attachment preview recovery out of `buildProjectChats` into a shared `recoverPreview`, so the REST DTO and the MCP list run the same logic and cannot drift on this again. - Use the full chain in `listChats`, including the raw-preview fallback, so MCP and the UI agree case for case — the truncated-wrapper wart included, which is pinned by a test rather than quietly "improved" on one surface. - Only polluted previews pay for the extra head-of-file read; everything else returns without I/O, so `list_chats` stays cheap. This is half of #614. The other half — teaching `autoName` to read `ai-title` — lives in `@herdctl/core` and needs a coordinated release, so it is deliberately not attempted here. Verified the three behavioural tests fail against the old one-liner rather than passing vacuously. Full server suite 1447 passed; typecheck clean. Co-Authored-By: Claude --- .changeset/tidy-moons-invent.md | 22 ++++ packages/server/src/chat-dto.ts | 54 +++++--- packages/server/src/management-ops.ts | 13 +- .../unit/management-ops-chat-name.test.ts | 119 ++++++++++++++++++ 4 files changed, 188 insertions(+), 20 deletions(-) create mode 100644 .changeset/tidy-moons-invent.md create mode 100644 packages/server/test/unit/management-ops-chat-name.test.ts diff --git a/.changeset/tidy-moons-invent.md b/.changeset/tidy-moons-invent.md new file mode 100644 index 00000000..08db1c09 --- /dev/null +++ b/.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`. diff --git a/packages/server/src/chat-dto.ts b/packages/server/src/chat-dto.ts index 22ae023f..bf5bcda1 100644 --- a/packages/server/src/chat-dto.ts +++ b/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 { + 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); }), ); diff --git a/packages/server/src/management-ops.ts b/packages/server/src/management-ops.ts index 78002352..178a96a5 100644 --- a/packages/server/src/management-ops.ts +++ b/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 diff --git a/packages/server/test/unit/management-ops-chat-name.test.ts b/packages/server/test/unit/management-ops-chat-name.test.ts new file mode 100644 index 00000000..d0b6e941 --- /dev/null +++ b/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[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 = "\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"); + }); +});