From c35432ebfc8d12ed41a9c80118f02d3f56bd69cd Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <225728493+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Sun, 24 May 2026 16:52:46 +0000 Subject: [PATCH 1/3] refactor(assistant): daemon goes bilingual on conversationKey/conversationId wire fields (LUM-1890 Phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Daemon now accepts both `conversationKey` (legacy) and `conversationId` (canonical) on its client-facing inbound surfaces, and emits both on the outbound surfaces. When both inbound fields are sent, `conversationId` wins. This is the non-breaking, additive half of LUM-1890 — Phase 2 will migrate the web client off `conversationKey` once this lands. Per the investigation at notes/daemon-wire-format-investigation.md: `conversationKey` on the daemon is a first-class concept, not legacy naming — the internal `conversation_keys` table (many external opaque keys → one internal conversations.id) stays and continues to serve messaging adapters (Telegram, WhatsApp). Only the client-facing wire field name needs cleanup. Phase 4 (much later, after macOS migrates) drops `conversationKey` from the wire. Surfaces touched ---------------- 1. POST /v1/messages/ — `handleSendMessage` now reads `body.conversationId ?? body.conversationKey` for the resolved key passed to `getOrCreateConversation`. Identifier intentionally NOT destructured from body because nested closures inside this handler shadow `conversationId` with `const conversationId = mapping.conversationId`. 2. GET /v1/events/ — `handleSubscribeAssistantEvents` now accepts `?conversationId=` as a synonym for `?conversationKey=`. Empty-value rejection for both. OpenAPI queryParams schema lists both names. 3. POST /v1/conversations/ — `handleCreateConversation` accepts both fields on input (precedence: id > key > minted UUID) and emits both in the response carrying the same echoed value. Zod request/response schemas updated accordingly. 4. serializeConversationSummary — emits `conversationId` alongside the existing `id` field (both carry conversations.id). Phase 4 eventually drops `id`. `buildConversationDetailResponse` inherits this automatically via the same serializer. 5. SSE event payloads — confirmed no-op. The hub's `AssistantEvent` and `AssistantEventFilter` only ever carry `conversationId`. 6. deepLinkMetadata emitters — confirmed no-op. The only production construction site is `broadcaster.ts:235-243` (vellum channel), which always writes `conversationId`. `macos.ts:92` and `platform.ts:88` are pure pass-throughs. Tests added ----------- - conversation-serializer-bilingual.test.ts (pure-function, 2 tests): verifies id === conversationId on every serialized summary. - runtime-events-sse-bilingual.test.ts (4 tests): `?conversationId=` scopes identically; `conversationId` wins over `conversationKey` when both present; both empty-value rejections. - create-conversation-bilingual.test.ts (4 tests): all four input combinations (key only / id only / both / neither). Uses the real conversation-key store + DB rather than `mock.module` to avoid bleeding mocks into other route tests that share the bun-test process. - conversation-routes-disk-view.test.ts (existing file, 2 new tests): `body.conversationId` alone resolves through the key store; when both are sent, only the id materialises a mapping (key never does). Validation ---------- - `bun run typecheck` clean - `bun run lint` clean - Phase 1 tests + neighbors: 27/27 pass (events-sse, serializer, create-conversation, conversation-management-routes, disk-view) - Broader sweep: 31/31 pass (conversation-query-routes, http-conversation-lineage), 38/38 pass (playground), 54/54 pass (full events test family) Phase 2 will migrate the web client off `conversationKey` on the wire. Web's remaining `conversationKey` count stays at 37 after this PR (additive only); Phase 2 is where the count drops. --- .../conversation-routes-disk-view.test.ts | 78 ++++++++++ .../runtime-events-sse-bilingual.test.ts | 139 ++++++++++++++++++ .../create-conversation-bilingual.test.ts | 135 +++++++++++++++++ .../routes/conversation-management-routes.ts | 43 +++++- .../src/runtime/routes/conversation-routes.ts | 22 ++- assistant/src/runtime/routes/events-routes.ts | 40 ++++- .../conversation-serializer-bilingual.test.ts | 92 ++++++++++++ .../services/conversation-serializer.ts | 7 + 8 files changed, 539 insertions(+), 17 deletions(-) create mode 100644 assistant/src/__tests__/runtime-events-sse-bilingual.test.ts create mode 100644 assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts create mode 100644 assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts diff --git a/assistant/src/__tests__/conversation-routes-disk-view.test.ts b/assistant/src/__tests__/conversation-routes-disk-view.test.ts index 28fdddc884b..752d70706a5 100644 --- a/assistant/src/__tests__/conversation-routes-disk-view.test.ts +++ b/assistant/src/__tests__/conversation-routes-disk-view.test.ts @@ -498,6 +498,84 @@ describe("macOS browser backend fallback (no extension, no cdp-inspect)", () => }); }); +describe("LUM-1890 Phase 1 bilingual — POST /v1/messages accepts body.conversationId", () => { + // The handler accepts both `body.conversationKey` (legacy) and + // `body.conversationId` (canonical). When both are sent, `conversationId` + // wins. These tests are removable after Phase 4 deprecates `conversationKey` + // on the wire. + + async function sendMessage(body: Record): Promise { + return callHandler( + (args) => + handleSendMessage(args, { + sendMessageDeps: { + getOrCreateConversation: async (conversationId: string) => + getOrCreateFakeConversation(conversationId), + assistantEventHub: new AssistantEventHub(), + resolveAttachments: () => [], + }, + }), + new Request("http://localhost/v1/messages", { + method: "POST", + headers: { + "Content-Type": "application/json", + "x-vellum-principal-type": authContext.principalType, + }, + body: JSON.stringify(body), + }), + undefined, + 202, + ); + } + + test("body.conversationId alone resolves and persists like body.conversationKey", async () => { + const conversationId = `phase1-id-only-${crypto.randomUUID()}`; + const response = await sendMessage({ + conversationId, + content: "Bilingual surface 1 — conversationId only.", + sourceChannel: "vellum", + interface: "macos", + }); + + expect(response.status).toBe(202); + const body = (await response.json()) as { + accepted: boolean; + conversationId: string; + }; + expect(body.accepted).toBe(true); + + // The key store mapping must be created under the inbound value, so + // a subsequent send under either field name resolves to the same row. + const mapping = getOrCreateConversationMapping(conversationId); + expect(mapping.created).toBe(false); + expect(mapping.conversationId).toBe(body.conversationId); + }); + + test("body.conversationId wins over body.conversationKey when both are sent", async () => { + const idValue = `phase1-id-wins-${crypto.randomUUID()}`; + const keyValue = `phase1-key-loses-${crypto.randomUUID()}`; + + const response = await sendMessage({ + conversationId: idValue, + conversationKey: keyValue, + content: "Bilingual surface 1 — precedence check.", + sourceChannel: "vellum", + interface: "macos", + }); + + expect(response.status).toBe(202); + const body = (await response.json()) as { conversationId: string }; + + // The internal conversation id must trace back to `idValue`, NOT + // `keyValue`. Confirm via the key store: only `idValue` should have + // been materialised; `keyValue` should not exist. + const idMapping = getConversationByKey(idValue); + const keyMapping = getConversationByKey(keyValue); + expect(idMapping?.conversationId).toBe(body.conversationId); + expect(keyMapping).toBeNull(); + }); +}); + describe("conversationKey send path disk-view regression", () => { test("first send on a fresh conversationKey creates disk-view dir and writes user+assistant records", async () => { const conversationKey = `fresh-conv-key-${crypto.randomUUID()}`; diff --git a/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts b/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts new file mode 100644 index 00000000000..9b241dd3d35 --- /dev/null +++ b/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts @@ -0,0 +1,139 @@ +/** + * LUM-1890 Phase 1 — `GET /v1/events` (`handleSubscribeAssistantEvents`) + * accepts the canonical `?conversationId=` query parameter as a synonym + * for the legacy `?conversationKey=`. Both resolve to the same downstream + * conversation-scoped subscription filter. + * + * Companion to `runtime-events-sse.test.ts` which exercises the existing + * `?conversationKey=` path. This file is removable after Phase 4 + * deprecates `conversationKey` on the wire. + */ + +import { beforeEach, describe, expect, mock, test } from "bun:test"; + +mock.module("../util/logger.js", () => ({ + getLogger: () => + new Proxy({} as Record, { + get: () => () => {}, + }), +})); + +mock.module("../config/loader.js", () => ({ + getConfig: () => ({ + ui: {}, + model: "test", + provider: "test", + memory: { enabled: false }, + rateLimit: { maxRequestsPerMinute: 0 }, + secretDetection: { enabled: false }, + }), +})); + +import { getOrCreateConversation } from "../memory/conversation-key-store.js"; +import { getDb } from "../memory/db-connection.js"; +import { initializeDb } from "../memory/db-init.js"; +import { buildAssistantEvent } from "../runtime/assistant-event.js"; +import { AssistantEventHub } from "../runtime/assistant-event-hub.js"; +import { BadRequestError } from "../runtime/routes/errors.js"; +import { handleSubscribeAssistantEvents } from "../runtime/routes/events-routes.js"; + +initializeDb(); + +describe("GET /v1/events — Phase 1 bilingual query params", () => { + beforeEach(() => { + const db = getDb(); + db.run("DELETE FROM conversation_keys"); + db.run("DELETE FROM conversations"); + }); + + test("?conversationId= scopes the stream identically to ?conversationKey=", async () => { + // Materialise the conversation under the same key so both parameter + // names resolve to the same internal id. + const sharedKey = "sse-bilingual-shared"; + const { conversationId } = getOrCreateConversation(sharedKey); + + const ac = new AbortController(); + const testHub = new AssistantEventHub(); + + const stream = handleSubscribeAssistantEvents( + { + queryParams: { conversationId: sharedKey }, + abortSignal: ac.signal, + }, + { hub: testHub }, + ); + + const reader = stream.getReader(); + + // Consume the initial heartbeat. + const heartbeat = await reader.read(); + expect(new TextDecoder().decode(heartbeat.value)).toBe(": heartbeat\n\n"); + + // Publish an event scoped to the resolved conversation id — the + // bilingual filter should let it through. + const event = buildAssistantEvent({ type: "pong" }, conversationId); + await testHub.publish(event); + + const { value, done } = await reader.read(); + ac.abort(); + + expect(done).toBe(false); + const frame = new TextDecoder().decode(value); + expect(frame).toContain("event: assistant_event"); + expect(frame).toContain(`"conversationId":"${conversationId}"`); + }); + + test("conversationId wins over conversationKey when both are present", async () => { + // The canonical param should be the one we resolve. We assert this by + // materialising two distinct conversations and checking which one the + // filter scopes to. + const idKey = "sse-bilingual-id-wins"; + const keyKey = "sse-bilingual-key-loses"; + const { conversationId: idConv } = getOrCreateConversation(idKey); + const { conversationId: keyConv } = getOrCreateConversation(keyKey); + expect(idConv).not.toBe(keyConv); + + const ac = new AbortController(); + const testHub = new AssistantEventHub(); + + const stream = handleSubscribeAssistantEvents( + { + queryParams: { conversationId: idKey, conversationKey: keyKey }, + abortSignal: ac.signal, + }, + { hub: testHub }, + ); + const reader = stream.getReader(); + await reader.read(); // heartbeat + + // Publish on the "loser" first — should NOT be delivered. + await testHub.publish(buildAssistantEvent({ type: "pong" }, keyConv)); + // Publish on the "winner" — should be delivered. + await testHub.publish(buildAssistantEvent({ type: "pong" }, idConv)); + + const { value } = await reader.read(); + ac.abort(); + const frame = new TextDecoder().decode(value); + + expect(frame).toContain(`"conversationId":"${idConv}"`); + expect(frame).not.toContain(`"conversationId":"${keyConv}"`); + }); + + test("empty conversationId is rejected with BadRequestError", () => { + expect(() => + handleSubscribeAssistantEvents({ + queryParams: { conversationId: "" }, + abortSignal: new AbortController().signal, + }), + ).toThrow(BadRequestError); + }); + + test("empty conversationKey is still rejected (legacy parity)", () => { + expect(() => + handleSubscribeAssistantEvents({ + queryParams: { conversationKey: "" }, + abortSignal: new AbortController().signal, + }), + ).toThrow(BadRequestError); + }); +}); diff --git a/assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts b/assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts new file mode 100644 index 00000000000..7a706e22079 --- /dev/null +++ b/assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts @@ -0,0 +1,135 @@ +/** + * LUM-1890 Phase 1 — `POST /v1/conversations/` (`handleCreateConversation`) + * accepts both `body.conversationId` and `body.conversationKey` as the + * idempotency key, and emits both `conversationId` and `conversationKey` + * in the response carrying the same echoed value. + * + * Precedence rule: when both inbound fields are sent, `conversationId` + * wins. When neither is sent, the daemon mints a UUID. Outbound `id` + * remains the internal `conversations.id` returned by the + * `getOrCreateConversation` store call. + * + * Uses the real conversation-key store + DB. `mock.module` is intentionally + * avoided here so module mocks don't bleed into other route tests that + * run later in the same `bun test` process. + */ + +import { beforeEach, describe, expect, mock, test } from "bun:test"; + +mock.module("../../../util/logger.js", () => ({ + getLogger: () => + new Proxy({} as Record, { + get: () => () => {}, + }), +})); + +// Stub the event hub to avoid spinning up real SSE infrastructure. +mock.module("../../assistant-event-hub.js", () => ({ + assistantEventHub: { + publish: async () => {}, + subscribe: () => () => {}, + }, + broadcastMessage: () => {}, +})); + +mock.module("../../sync/resource-sync-events.js", () => ({ + publishConversationListAndMetadataChanged: () => {}, + publishConversationListChanged: () => {}, + publishConversationTitleChanged: () => {}, +})); + +import { getConversationByKey } from "../../../memory/conversation-key-store.js"; +import { getDb } from "../../../memory/db-connection.js"; +import { initializeDb } from "../../../memory/db-init.js"; +import { conversationKeys, conversations } from "../../../memory/schema.js"; +import { ROUTES as CONVERSATION_MANAGEMENT_ROUTES } from "../conversation-management-routes.js"; +import type { RouteDefinition, RouteHandlerArgs } from "../types.js"; + +initializeDb(); + +function findHandler(routes: RouteDefinition[], operationId: string) { + const route = routes.find((r) => r.operationId === operationId); + if (!route) throw new Error(`Route ${operationId} not found`); + return route.handler; +} + +const createHandler = findHandler( + CONVERSATION_MANAGEMENT_ROUTES, + "createConversation", +); + +async function callCreate( + body: Record, +): Promise> { + const args: RouteHandlerArgs = { + body, + headers: {}, + queryParams: {}, + pathParams: {}, + }; + return (await createHandler(args)) as Record; +} + +describe("POST /v1/conversations — Phase 1 bilingual", () => { + beforeEach(() => { + const db = getDb(); + db.delete(conversationKeys).run(); + db.delete(conversations).run(); + }); + + test("accepts body.conversationKey (legacy field) and echoes both names", async () => { + const requested = "client-key-aaa"; + const result = await callCreate({ conversationKey: requested }); + + // The key store must have a mapping under the requested key. + const mapping = getConversationByKey(requested); + expect(mapping).not.toBeNull(); + expect(mapping?.conversationId).toBe(result.id as string); + + expect(result.conversationKey).toBe(requested); + expect(result.conversationId).toBe(requested); + expect(result.created).toBe(true); + }); + + test("accepts body.conversationId (canonical field) and echoes both names", async () => { + const requested = "client-id-bbb"; + const result = await callCreate({ conversationId: requested }); + + const mapping = getConversationByKey(requested); + expect(mapping).not.toBeNull(); + expect(mapping?.conversationId).toBe(result.id as string); + + expect(result.conversationId).toBe(requested); + expect(result.conversationKey).toBe(requested); + }); + + test("prefers body.conversationId over body.conversationKey when both are sent", async () => { + const idValue = "wins-ccc"; + const keyValue = "loses-ddd"; + + const result = await callCreate({ + conversationId: idValue, + conversationKey: keyValue, + }); + + // The "winning" id materialised a key-store mapping; the "losing" one + // never did. + const idMapping = getConversationByKey(idValue); + const keyMapping = getConversationByKey(keyValue); + expect(idMapping?.conversationId).toBe(result.id as string); + expect(keyMapping).toBeNull(); + + expect(result.conversationId).toBe(idValue); + expect(result.conversationKey).toBe(idValue); + }); + + test("mints a UUID when neither field is sent", async () => { + const result = await callCreate({}); + + // UUIDs are 36 characters (8-4-4-4-12). + expect(typeof result.conversationId).toBe("string"); + expect((result.conversationId as string).length).toBe(36); + // Both response fields carry the same minted value. + expect(result.conversationKey).toBe(result.conversationId); + }); +}); diff --git a/assistant/src/runtime/routes/conversation-management-routes.ts b/assistant/src/runtime/routes/conversation-management-routes.ts index ffb313ce8fa..e721a8a9965 100644 --- a/assistant/src/runtime/routes/conversation-management-routes.ts +++ b/assistant/src/runtime/routes/conversation-management-routes.ts @@ -85,9 +85,14 @@ function cancelScheduleIfLast(conversationId: string): void { // --------------------------------------------------------------------------- function handleCreateConversation({ body = {}, headers }: RouteHandlerArgs) { - const conversationKey = - (body.conversationKey as string | undefined) ?? crypto.randomUUID(); - const result = getOrCreateConversation(conversationKey, { + // LUM-1890 Phase 1: bilingual acceptance for the idempotency key. Prefer + // the canonical `conversationId` when the client sends it; fall back to + // the legacy `conversationKey`. When neither is sent, mint a UUID. + const requestedConversationKey = + (body.conversationId as string | undefined) ?? + (body.conversationKey as string | undefined) ?? + crypto.randomUUID(); + const result = getOrCreateConversation(requestedConversationKey, { conversationType: "standard", }); if (result.created) { @@ -101,14 +106,20 @@ function handleCreateConversation({ body = {}, headers }: RouteHandlerArgs) { log.info( { conversationId: result.conversationId, - conversationKey, + conversationKey: requestedConversationKey, created: result.created, }, "Created conversation via POST", ); + // The response emits both `conversationId` (new canonical name) and + // `conversationKey` (legacy echo). Both carry the same value — whatever + // the client supplied (or the UUID we minted). The web client will + // migrate to reading `conversationId` in LUM-1890 Phase 2; for now the + // additive field is forward-compat only. return { id: result.conversationId, - conversationKey, + conversationId: requestedConversationKey, + conversationKey: requestedConversationKey, conversationType: normalizeConversationType(result.conversationType), created: result.created, }; @@ -426,9 +437,18 @@ export const ROUTES: RouteDefinition[] = [ description: "Create or get an existing conversation by key.", tags: ["conversations"], requestBody: z.object({ + conversationId: z + .string() + .optional() + .describe( + "Idempotency key for the conversation (canonical name; preferred over conversationKey).", + ), conversationKey: z .string() - .describe("Idempotency key for the conversation"), + .optional() + .describe( + "Legacy synonym for conversationId. When both are sent, conversationId wins.", + ), conversationType: z .literal("standard") .optional() @@ -436,7 +456,16 @@ export const ROUTES: RouteDefinition[] = [ }), responseBody: z.object({ id: z.string(), - conversationKey: z.string(), + conversationId: z + .string() + .describe( + "Echo of the requested conversationId/conversationKey (canonical name).", + ), + conversationKey: z + .string() + .describe( + "Legacy echo of the requested conversationId/conversationKey. Carries the same value as conversationId.", + ), conversationType: z.string(), created: z.boolean(), }), diff --git a/assistant/src/runtime/routes/conversation-routes.ts b/assistant/src/runtime/routes/conversation-routes.ts index 3a738da00a4..4d8c888ecf9 100644 --- a/assistant/src/runtime/routes/conversation-routes.ts +++ b/assistant/src/runtime/routes/conversation-routes.ts @@ -1223,6 +1223,13 @@ export async function handleSendMessage( ): Promise { const body = (rawBody ?? {}) as { conversationKey?: string; + // `conversationId` is the canonical client-facing name and is accepted + // as a synonym for `conversationKey` here. When both are present, + // `conversationId` wins. Both ultimately resolve to a value that + // `getOrCreateConversation` looks up in the `conversation_keys` table + // (either as an existing internal id or as an external key). See + // LUM-1890 Phase 1 for the bilingual rollout plan. + conversationId?: string; content?: string; attachmentIds?: string[]; sourceChannel?: string; @@ -1258,6 +1265,12 @@ export async function handleSendMessage( headers?.["x-vellum-client-id"]?.trim() || undefined; const { conversationKey, content, attachmentIds } = body; + // Bilingual acceptance: prefer the canonical `conversationId` when the + // client sends it; fall back to the legacy `conversationKey` field. + // Not destructured here because nested closures inside this handler + // already shadow the `conversationId` identifier with the resolved + // internal id (e.g. `const conversationId = mapping.conversationId`). + const inboundConversationKey = body.conversationId ?? conversationKey; const clientMessageId = typeof body.clientMessageId === "string" ? body.clientMessageId : undefined; const requestedInferenceProfile = @@ -1325,11 +1338,12 @@ export async function handleSendMessage( ? (canonicalizeTimeZone(body.clientTimezone) ?? undefined) : undefined; - // When conversationKey is omitted, derive a stable default from - // sourceChannel + sourceInterface so that repeated calls from the same - // channel/interface pair share a single conversation thread. + // When both conversationId and conversationKey are omitted, derive a + // stable default from sourceChannel + sourceInterface so that repeated + // calls from the same channel/interface pair share a single conversation + // thread. const resolvedConversationKey = - conversationKey ?? `default:${sourceChannel}:${sourceInterface}`; + inboundConversationKey ?? `default:${sourceChannel}:${sourceInterface}`; // Reject non-string content values (numbers, objects, etc.) if (content != null && typeof content !== "string") { diff --git a/assistant/src/runtime/routes/events-routes.ts b/assistant/src/runtime/routes/events-routes.ts index 1e3986673fe..f14a682595b 100644 --- a/assistant/src/runtime/routes/events-routes.ts +++ b/assistant/src/runtime/routes/events-routes.ts @@ -220,9 +220,14 @@ const defaultSseShedReporter: SseShedReporter = (reason, inst) => { * Stream assistant events as Server-Sent Events. * * Query params: - * conversationKey -- optional; when provided, scopes the stream to one - * conversation. When omitted, the stream delivers events - * from ALL conversations for this assistant. + * conversationId -- optional; canonical client-facing name for the scope + * identifier. When provided, scopes the stream to one + * conversation. + * conversationKey -- optional; legacy synonym for `conversationId`. When + * both are sent, `conversationId` wins. When both are + * omitted, the stream delivers events from ALL + * conversations for this assistant. See LUM-1890 Phase 1 + * for the bilingual rollout. * * Headers (optional): * X-Vellum-Client-Id -- stable per-install UUID identifying this client. @@ -248,10 +253,27 @@ export function handleSubscribeAssistantEvents( ): ReadableStream { const { queryParams, headers, abortSignal } = args; - const conversationKey = queryParams?.conversationKey; - if ("conversationKey" in (queryParams ?? {}) && !conversationKey?.trim()) { + // LUM-1890 Phase 1: accept the canonical `conversationId` query param + // as a synonym for the legacy `conversationKey`. When both are present, + // `conversationId` wins. Internally we keep the variable named + // `conversationKey` because downstream call sites (the SSE instrumentation + // record + the `getOrCreateConversation` lookup) treat this value as the + // external key in the `conversation_keys` table. + const rawConversationId = queryParams?.conversationId; + const rawConversationKey = queryParams?.conversationKey; + if ( + "conversationId" in (queryParams ?? {}) && + !rawConversationId?.trim() + ) { + throw new BadRequestError("conversationId must not be empty"); + } + if ( + "conversationKey" in (queryParams ?? {}) && + !rawConversationKey?.trim() + ) { throw new BadRequestError("conversationKey must not be empty"); } + const conversationKey = rawConversationId ?? rawConversationKey; // ── Client identity from headers ────────────────────────────────────── const rawClientId = headers?.["x-vellum-client-id"]; @@ -470,9 +492,15 @@ export const ROUTES: RouteDefinition[] = [ description: "Stream assistant events as Server-Sent Events (SSE).", tags: ["events"], queryParams: [ + { + name: "conversationId", + description: + "Scope to a single conversation (canonical name; preferred when sending fresh requests).", + }, { name: "conversationKey", - description: "Scope to a single conversation", + description: + "Scope to a single conversation (legacy synonym for conversationId).", }, ], responseHeaders: { diff --git a/assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts b/assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts new file mode 100644 index 00000000000..9f86770a2ed --- /dev/null +++ b/assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts @@ -0,0 +1,92 @@ +/** + * LUM-1890 Phase 1 — `serializeConversationSummary` emits the canonical + * `conversationId` alongside the legacy `id` field. + * + * Both fields carry the same value (the daemon's internal `conversations.id`). + * The `conversationId` field is forward-compat: it lets clients that prefer + * the canonical name read it directly without re-mapping, enabling the + * Phase 2 web migration to drop the `id` → `conversationId` mapping in + * `parseConversation`. + * + * Removable from the wire when Phase 4 deprecates `id` (after macOS catches + * up). Until then, both fields are emitted. + */ + +import { describe, expect, mock, test } from "bun:test"; + +mock.module("../../../util/logger.js", () => ({ + getLogger: () => + new Proxy({} as Record, { + get: () => () => {}, + }), +})); + +import type { ConversationRow } from "../../../memory/conversation-crud.js"; +import { serializeConversationSummary } from "../conversation-serializer.js"; + +function buildConversationRow( + overrides: Partial = {}, +): ConversationRow { + const now = Date.now(); + return { + id: "conv-internal-id-123", + title: "Test conversation", + createdAt: now, + updatedAt: now, + totalInputTokens: 0, + totalOutputTokens: 0, + totalEstimatedCost: 0, + contextSummary: null, + contextCompactedMessageCount: 0, + contextCompactedAt: null, + historyStrippedAt: null, + slackContextCompactionWatermarkTs: null, + slackContextCompactionWatermarkAt: null, + conversationType: "standard", + source: "user", + memoryScopeId: "scope-1", + originChannel: null, + originInterface: null, + forkParentConversationId: null, + forkParentMessageId: null, + ...overrides, + } as ConversationRow; +} + +describe("serializeConversationSummary — Phase 1 bilingual emission", () => { + test("emits both `id` and `conversationId` with the same internal id value", () => { + const conversation = buildConversationRow({ id: "conv-bilingual-abc" }); + const out = serializeConversationSummary({ + conversation, + parentCache: new Map(), + }); + + expect(out.id).toBe("conv-bilingual-abc"); + expect(out.conversationId).toBe("conv-bilingual-abc"); + // Forward-compat guarantee: both fields always carry the same value + // until Phase 4 drops `id`. Any divergence is a contract bug. + expect(out.id).toBe(out.conversationId); + }); + + test("conversationId tracks id even when binding/attention/displayMeta are set", () => { + const conversation = buildConversationRow({ id: "conv-with-meta-xyz" }); + const out = serializeConversationSummary({ + conversation, + displayMeta: { + displayOrder: 5, + isPinned: true, + groupId: "group-7", + }, + parentCache: new Map(), + }); + + // The serializer return type is a discriminated union over the + // display-meta variants; reach in via Record for + // the optional fields rather than narrowing. + const outAsRecord = out as Record; + expect(out.id).toBe("conv-with-meta-xyz"); + expect(out.conversationId).toBe("conv-with-meta-xyz"); + expect(outAsRecord.isPinned).toBe(true); + expect(outAsRecord.displayOrder).toBe(5); + }); +}); diff --git a/assistant/src/runtime/services/conversation-serializer.ts b/assistant/src/runtime/services/conversation-serializer.ts index c002f73a32e..fb676bf3dcf 100644 --- a/assistant/src/runtime/services/conversation-serializer.ts +++ b/assistant/src/runtime/services/conversation-serializer.ts @@ -171,6 +171,13 @@ export function serializeConversationSummary(params: { return { id: conversation.id, + // LUM-1890 Phase 1: emit `conversationId` alongside `id` as a + // forward-compat field so the web's `parseConversation` (and any + // other client that prefers the canonical name) can read it directly + // without re-mapping. Both carry the same internal conversations.id + // value. Phase 2 will migrate web off `id`/`conversationKey` onto + // `conversationId`; Phase 4 eventually drops `id`. + conversationId: conversation.id, title: conversation.title ?? "Untitled", createdAt: conversation.createdAt, updatedAt: conversation.updatedAt, From 5ea97a8959e3f939e2098d370235d5de17924b8f Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <225728493+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Sun, 24 May 2026 17:39:24 +0000 Subject: [PATCH 2/3] chore(assistant): regenerate openapi.yaml for LUM-1890 Phase 1 bilingual MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picks up the additive conversationId fields in three places: - POST /v1/conversations request body — conversationId/conversationKey both optional; conversationKey loses required status. - POST /v1/conversations response body — adds conversationId to the required echo set alongside conversationKey/id. - GET /v1/events queryParams — adds conversationId param alongside the legacy conversationKey one, with updated descriptions. CI `OpenAPI Spec Check` was failing on the stale file. --- assistant/openapi.yaml | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/assistant/openapi.yaml b/assistant/openapi.yaml index 7648c3f59be..ed823fefdfc 100644 --- a/assistant/openapi.yaml +++ b/assistant/openapi.yaml @@ -4616,14 +4616,19 @@ paths: properties: id: type: string + conversationId: + type: string + description: Echo of the requested conversationId/conversationKey (canonical name). conversationKey: type: string + description: Legacy echo of the requested conversationId/conversationKey. Carries the same value as conversationId. conversationType: type: string created: type: boolean required: - id + - conversationId - conversationKey - conversationType - created @@ -4635,15 +4640,16 @@ paths: schema: type: object properties: + conversationId: + description: Idempotency key for the conversation (canonical name; preferred over conversationKey). + type: string conversationKey: + description: Legacy synonym for conversationId. When both are sent, conversationId wins. type: string - description: Idempotency key for the conversation conversationType: description: Only standard conversations are created by this endpoint type: string const: standard - required: - - conversationKey additionalProperties: false /v1/conversations/{conversationId}/slack-channel/resolve: post: @@ -7651,12 +7657,18 @@ paths: "200": description: Successful response parameters: + - name: conversationId + in: query + required: false + schema: + type: string + description: Scope to a single conversation (canonical name; preferred when sending fresh requests). - name: conversationKey in: query required: false schema: type: string - description: Scope to a single conversation + description: Scope to a single conversation (legacy synonym for conversationId). /v1/events/emit: post: operationId: events_emit_post From 3d95ad17ca04d5cb325d0c84143fec84ad3b1521 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <225728493+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Sun, 24 May 2026 18:15:27 +0000 Subject: [PATCH 3/3] =?UTF-8?q?refactor(assistant):=20apply=20Phase=201=20?= =?UTF-8?q?review=20feedback=20=E2=80=94=20strict=20dual-lookup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acts on Vargas's review on PR #31922: 1. POST /v1/conversations no longer accepts `conversationId` on input. Conversation id is assistant-controlled, not client-controlled — the optimistic-UI flow is a future frontend concern. Response also drops the additive `conversationId` echo (redundant with `id`). `conversationKey` stays in the response: it's the external key for non-vellum channels (Telegram, WhatsApp, etc.). 2. `serializeConversationSummary` no longer emits `conversationId` alongside `id` — same redundancy. Conversation records carry their identifier as `id`; non-Conversation records (events, messages) use `conversationId`. The frontend principle stays consistent. 3. POST /v1/messages and GET /v1/events now use strict dual-lookup instead of `conversationId ?? conversationKey`: - `conversationId` (when supplied) is the assistant-minted internal id. Looked up directly via `getConversation`. Missing row = 404. Never materializes a new row. - `conversationKey` (when supplied) is an external key. Existing `getOrCreateConversation` flow, materializes on first use. - When both are sent, `conversationId` wins and `conversationKey` is ignored — no combination, no fallback, no empty-string footgun (also resolves Codex's P1/P2). 4. Stripped Linear ticket references from all code comments (prod and tests). Saved for git/PR history. 5. Removed the verbose body-type comment on handleSendMessage that Vargas flagged on line 1231. Tests - Deleted `create-conversation-bilingual.test.ts` and `conversation-serializer-bilingual.test.ts` (they covered reverted behavior). - Rewrote the events SSE bilingual test for the new strict-lookup semantics: id-scopes-stream / id-not-found-throws / id-wins-when-both / empty-id-rejected / empty-key-rejected (5 tests). - Rewrote the disk-view send-message bilingual tests: id-routes-to- existing-conversation / id-not-found-404 / id-wins-when-both (3 tests). - `bun run typecheck` clean. `bun run lint` clean. - 23/23 across Phase 1 surfaces + neighbors. 64/64 across the full events test family. OpenAPI regenerated: drops the additive `conversationId` from the create-conversation request/response shapes and updates the events queryParam descriptions to reflect strict semantics. --- assistant/openapi.yaml | 21 ++- .../conversation-routes-disk-view.test.ts | 91 ++++++++---- .../runtime-events-sse-bilingual.test.ts | 77 ++++++---- .../create-conversation-bilingual.test.ts | 135 ------------------ .../routes/conversation-management-routes.ts | 43 +++--- .../src/runtime/routes/conversation-routes.ts | 67 ++++++--- assistant/src/runtime/routes/events-routes.ts | 58 +++++--- .../conversation-serializer-bilingual.test.ts | 92 ------------ .../services/conversation-serializer.ts | 7 - 9 files changed, 214 insertions(+), 377 deletions(-) delete mode 100644 assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts delete mode 100644 assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts diff --git a/assistant/openapi.yaml b/assistant/openapi.yaml index ed823fefdfc..0c4a03c7dc8 100644 --- a/assistant/openapi.yaml +++ b/assistant/openapi.yaml @@ -4616,19 +4616,16 @@ paths: properties: id: type: string - conversationId: - type: string - description: Echo of the requested conversationId/conversationKey (canonical name). + description: Assistant-minted internal conversation id. The authoritative identifier for the conversation. conversationKey: type: string - description: Legacy echo of the requested conversationId/conversationKey. Carries the same value as conversationId. + description: Echo of the optional external key supplied by the client (or the value the daemon minted when omitted). conversationType: type: string created: type: boolean required: - id - - conversationId - conversationKey - conversationType - created @@ -4640,11 +4637,11 @@ paths: schema: type: object properties: - conversationId: - description: Idempotency key for the conversation (canonical name; preferred over conversationKey). - type: string conversationKey: - description: Legacy synonym for conversationId. When both are sent, conversationId wins. + description: + Optional external key. Echoed back in the response. Non-vellum channels (Telegram, WhatsApp) use this to + scope to a logical channel thread; vellum-web clients can omit it and rely on the assistant-minted + `id`. type: string conversationType: description: Only standard conversations are created by this endpoint @@ -7662,13 +7659,15 @@ paths: required: false schema: type: string - description: Scope to a single conversation (canonical name; preferred when sending fresh requests). + description: Scope to a single conversation by its assistant-minted internal id. 404s if no such conversation exists. - name: conversationKey in: query required: false schema: type: string - description: Scope to a single conversation (legacy synonym for conversationId). + description: + Scope to a single conversation by an external key (non-vellum channels) or the web idempotency key. + Materializes a row on first use. Ignored when conversationId is also provided. /v1/events/emit: post: operationId: events_emit_post diff --git a/assistant/src/__tests__/conversation-routes-disk-view.test.ts b/assistant/src/__tests__/conversation-routes-disk-view.test.ts index 752d70706a5..8716ab1d5c9 100644 --- a/assistant/src/__tests__/conversation-routes-disk-view.test.ts +++ b/assistant/src/__tests__/conversation-routes-disk-view.test.ts @@ -498,13 +498,23 @@ describe("macOS browser backend fallback (no extension, no cdp-inspect)", () => }); }); -describe("LUM-1890 Phase 1 bilingual — POST /v1/messages accepts body.conversationId", () => { - // The handler accepts both `body.conversationKey` (legacy) and - // `body.conversationId` (canonical). When both are sent, `conversationId` - // wins. These tests are removable after Phase 4 deprecates `conversationKey` - // on the wire. - - async function sendMessage(body: Record): Promise { +describe("POST /v1/messages — body.conversationId direct id lookup", () => { + // The handler accepts two scope inputs with distinct semantics: + // + // - `body.conversationId` is the assistant-minted internal id and is + // looked up directly. A missing row is a 404 — clients must obtain + // the id from a prior daemon response. + // - `body.conversationKey` is an external key (non-vellum channels / + // web idempotency); resolved via the conversation_keys table and + // materialised on first use. + // + // When both are sent, `conversationId` wins and `conversationKey` is + // ignored. (Don't combine — fetch by one and then the other.) + + async function sendMessage( + body: Record, + successStatus = 202, + ): Promise { return callHandler( (args) => handleSendMessage(args, { @@ -524,15 +534,19 @@ describe("LUM-1890 Phase 1 bilingual — POST /v1/messages accepts body.conversa body: JSON.stringify(body), }), undefined, - 202, + successStatus, ); } - test("body.conversationId alone resolves and persists like body.conversationKey", async () => { - const conversationId = `phase1-id-only-${crypto.randomUUID()}`; + test("body.conversationId= scopes the send to that conversation", async () => { + // Pre-materialise a conversation via the key path, then send a message + // by its assistant-minted internal id. + const externalKey = `pre-materialised-${crypto.randomUUID()}`; + const seeded = getOrCreateConversationMapping(externalKey); + const response = await sendMessage({ - conversationId, - content: "Bilingual surface 1 — conversationId only.", + conversationId: seeded.conversationId, + content: "Direct id lookup — should reuse the existing conversation.", sourceChannel: "vellum", interface: "macos", }); @@ -543,36 +557,53 @@ describe("LUM-1890 Phase 1 bilingual — POST /v1/messages accepts body.conversa conversationId: string; }; expect(body.accepted).toBe(true); + expect(body.conversationId).toBe(seeded.conversationId); - // The key store mapping must be created under the inbound value, so - // a subsequent send under either field name resolves to the same row. - const mapping = getOrCreateConversationMapping(conversationId); - expect(mapping.created).toBe(false); - expect(mapping.conversationId).toBe(body.conversationId); + // No new external-key row should be materialised under the internal id. + expect(getConversationByKey(seeded.conversationId)).toBeNull(); }); - test("body.conversationId wins over body.conversationKey when both are sent", async () => { - const idValue = `phase1-id-wins-${crypto.randomUUID()}`; - const keyValue = `phase1-key-loses-${crypto.randomUUID()}`; + test("body.conversationId= returns 404", async () => { + const response = await sendMessage( + { + conversationId: `does-not-exist-${crypto.randomUUID()}`, + content: "Should 404 — unknown internal id.", + sourceChannel: "vellum", + interface: "macos", + }, + 404, + ); + expect(response.status).toBe(404); + const body = (await response.json()) as { + error?: { code?: string; message?: string }; + }; + expect(body.error?.code).toBe("NOT_FOUND"); + expect(body.error?.message).toMatch(/not found/i); + }); + + test("body.conversationId is honored and body.conversationKey is ignored when both are sent", async () => { + // Seed a conversation for the id we'll send. Also seed a separate + // conversation under a key the client will pass alongside — but the + // handler must scope to the id, NOT the key. + const idSeed = getOrCreateConversationMapping( + `id-honored-${crypto.randomUUID()}`, + ); + const keyValue = `key-ignored-${crypto.randomUUID()}`; + const keySeed = getOrCreateConversationMapping(keyValue); + expect(idSeed.conversationId).not.toBe(keySeed.conversationId); const response = await sendMessage({ - conversationId: idValue, + conversationId: idSeed.conversationId, conversationKey: keyValue, - content: "Bilingual surface 1 — precedence check.", + content: "Both fields sent — id should win.", sourceChannel: "vellum", interface: "macos", }); expect(response.status).toBe(202); const body = (await response.json()) as { conversationId: string }; - - // The internal conversation id must trace back to `idValue`, NOT - // `keyValue`. Confirm via the key store: only `idValue` should have - // been materialised; `keyValue` should not exist. - const idMapping = getConversationByKey(idValue); - const keyMapping = getConversationByKey(keyValue); - expect(idMapping?.conversationId).toBe(body.conversationId); - expect(keyMapping).toBeNull(); + expect(body.conversationId).toBe(idSeed.conversationId); + expect(body.conversationId).not.toBe(keySeed.conversationId); }); }); diff --git a/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts b/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts index 9b241dd3d35..5e2a6714bd5 100644 --- a/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts +++ b/assistant/src/__tests__/runtime-events-sse-bilingual.test.ts @@ -1,12 +1,16 @@ /** - * LUM-1890 Phase 1 — `GET /v1/events` (`handleSubscribeAssistantEvents`) - * accepts the canonical `?conversationId=` query parameter as a synonym - * for the legacy `?conversationKey=`. Both resolve to the same downstream - * conversation-scoped subscription filter. + * `GET /v1/events` (`handleSubscribeAssistantEvents`) — bilingual scope + * resolution. Two query params are accepted, with distinct semantics: * - * Companion to `runtime-events-sse.test.ts` which exercises the existing - * `?conversationKey=` path. This file is removable after Phase 4 - * deprecates `conversationKey` on the wire. + * - `?conversationId=` — looks up the conversation row + * directly by its assistant-minted id. 404 if not found. Does NOT + * materialise a new row. + * - `?conversationKey=` — resolves via the + * `conversation_keys` table; materialises on first use. Ignored when + * `conversationId` is also supplied. + * + * Companion to `runtime-events-sse.test.ts`, which exercises the broader + * `?conversationKey=` happy/error path. */ import { beforeEach, describe, expect, mock, test } from "bun:test"; @@ -34,45 +38,44 @@ import { getDb } from "../memory/db-connection.js"; import { initializeDb } from "../memory/db-init.js"; import { buildAssistantEvent } from "../runtime/assistant-event.js"; import { AssistantEventHub } from "../runtime/assistant-event-hub.js"; -import { BadRequestError } from "../runtime/routes/errors.js"; +import { + BadRequestError, + NotFoundError, +} from "../runtime/routes/errors.js"; import { handleSubscribeAssistantEvents } from "../runtime/routes/events-routes.js"; initializeDb(); -describe("GET /v1/events — Phase 1 bilingual query params", () => { +describe("GET /v1/events — bilingual scope query params", () => { beforeEach(() => { const db = getDb(); db.run("DELETE FROM conversation_keys"); db.run("DELETE FROM conversations"); }); - test("?conversationId= scopes the stream identically to ?conversationKey=", async () => { - // Materialise the conversation under the same key so both parameter - // names resolve to the same internal id. - const sharedKey = "sse-bilingual-shared"; - const { conversationId } = getOrCreateConversation(sharedKey); + test("?conversationId= scopes the stream to that conversation", async () => { + // Materialise a conversation via the key path, then subscribe to it + // directly by its internal id. + const { conversationId } = getOrCreateConversation("sse-id-scope-source"); const ac = new AbortController(); const testHub = new AssistantEventHub(); const stream = handleSubscribeAssistantEvents( { - queryParams: { conversationId: sharedKey }, + queryParams: { conversationId }, abortSignal: ac.signal, }, { hub: testHub }, ); const reader = stream.getReader(); - // Consume the initial heartbeat. const heartbeat = await reader.read(); expect(new TextDecoder().decode(heartbeat.value)).toBe(": heartbeat\n\n"); - // Publish an event scoped to the resolved conversation id — the - // bilingual filter should let it through. - const event = buildAssistantEvent({ type: "pong" }, conversationId); - await testHub.publish(event); + // Publish an event scoped to that conversation — should be delivered. + await testHub.publish(buildAssistantEvent({ type: "pong" }, conversationId)); const { value, done } = await reader.read(); ac.abort(); @@ -83,14 +86,22 @@ describe("GET /v1/events — Phase 1 bilingual query params", () => { expect(frame).toContain(`"conversationId":"${conversationId}"`); }); - test("conversationId wins over conversationKey when both are present", async () => { - // The canonical param should be the one we resolve. We assert this by - // materialising two distinct conversations and checking which one the - // filter scopes to. - const idKey = "sse-bilingual-id-wins"; - const keyKey = "sse-bilingual-key-loses"; - const { conversationId: idConv } = getOrCreateConversation(idKey); - const { conversationId: keyConv } = getOrCreateConversation(keyKey); + test("?conversationId= throws NotFoundError", () => { + expect(() => + handleSubscribeAssistantEvents({ + queryParams: { conversationId: "does-not-exist" }, + abortSignal: new AbortController().signal, + }), + ).toThrow(NotFoundError); + }); + + test("?conversationId is honored and ?conversationKey is ignored when both are present", async () => { + // Materialise two distinct conversations: one we'll subscribe to by id, + // one we'll publish to via the ignored key. + const { conversationId: idConv } = getOrCreateConversation("sse-id-wins"); + const { conversationId: keyConv } = getOrCreateConversation( + "sse-key-ignored", + ); expect(idConv).not.toBe(keyConv); const ac = new AbortController(); @@ -98,7 +109,10 @@ describe("GET /v1/events — Phase 1 bilingual query params", () => { const stream = handleSubscribeAssistantEvents( { - queryParams: { conversationId: idKey, conversationKey: keyKey }, + queryParams: { + conversationId: idConv, + conversationKey: "sse-key-ignored", + }, abortSignal: ac.signal, }, { hub: testHub }, @@ -106,9 +120,10 @@ describe("GET /v1/events — Phase 1 bilingual query params", () => { const reader = stream.getReader(); await reader.read(); // heartbeat - // Publish on the "loser" first — should NOT be delivered. + // Publish on the "key" conversation — should NOT be delivered (filter + // is locked to idConv because conversationId wins). await testHub.publish(buildAssistantEvent({ type: "pong" }, keyConv)); - // Publish on the "winner" — should be delivered. + // Publish on the "id" conversation — should be delivered. await testHub.publish(buildAssistantEvent({ type: "pong" }, idConv)); const { value } = await reader.read(); diff --git a/assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts b/assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts deleted file mode 100644 index 7a706e22079..00000000000 --- a/assistant/src/runtime/routes/__tests__/create-conversation-bilingual.test.ts +++ /dev/null @@ -1,135 +0,0 @@ -/** - * LUM-1890 Phase 1 — `POST /v1/conversations/` (`handleCreateConversation`) - * accepts both `body.conversationId` and `body.conversationKey` as the - * idempotency key, and emits both `conversationId` and `conversationKey` - * in the response carrying the same echoed value. - * - * Precedence rule: when both inbound fields are sent, `conversationId` - * wins. When neither is sent, the daemon mints a UUID. Outbound `id` - * remains the internal `conversations.id` returned by the - * `getOrCreateConversation` store call. - * - * Uses the real conversation-key store + DB. `mock.module` is intentionally - * avoided here so module mocks don't bleed into other route tests that - * run later in the same `bun test` process. - */ - -import { beforeEach, describe, expect, mock, test } from "bun:test"; - -mock.module("../../../util/logger.js", () => ({ - getLogger: () => - new Proxy({} as Record, { - get: () => () => {}, - }), -})); - -// Stub the event hub to avoid spinning up real SSE infrastructure. -mock.module("../../assistant-event-hub.js", () => ({ - assistantEventHub: { - publish: async () => {}, - subscribe: () => () => {}, - }, - broadcastMessage: () => {}, -})); - -mock.module("../../sync/resource-sync-events.js", () => ({ - publishConversationListAndMetadataChanged: () => {}, - publishConversationListChanged: () => {}, - publishConversationTitleChanged: () => {}, -})); - -import { getConversationByKey } from "../../../memory/conversation-key-store.js"; -import { getDb } from "../../../memory/db-connection.js"; -import { initializeDb } from "../../../memory/db-init.js"; -import { conversationKeys, conversations } from "../../../memory/schema.js"; -import { ROUTES as CONVERSATION_MANAGEMENT_ROUTES } from "../conversation-management-routes.js"; -import type { RouteDefinition, RouteHandlerArgs } from "../types.js"; - -initializeDb(); - -function findHandler(routes: RouteDefinition[], operationId: string) { - const route = routes.find((r) => r.operationId === operationId); - if (!route) throw new Error(`Route ${operationId} not found`); - return route.handler; -} - -const createHandler = findHandler( - CONVERSATION_MANAGEMENT_ROUTES, - "createConversation", -); - -async function callCreate( - body: Record, -): Promise> { - const args: RouteHandlerArgs = { - body, - headers: {}, - queryParams: {}, - pathParams: {}, - }; - return (await createHandler(args)) as Record; -} - -describe("POST /v1/conversations — Phase 1 bilingual", () => { - beforeEach(() => { - const db = getDb(); - db.delete(conversationKeys).run(); - db.delete(conversations).run(); - }); - - test("accepts body.conversationKey (legacy field) and echoes both names", async () => { - const requested = "client-key-aaa"; - const result = await callCreate({ conversationKey: requested }); - - // The key store must have a mapping under the requested key. - const mapping = getConversationByKey(requested); - expect(mapping).not.toBeNull(); - expect(mapping?.conversationId).toBe(result.id as string); - - expect(result.conversationKey).toBe(requested); - expect(result.conversationId).toBe(requested); - expect(result.created).toBe(true); - }); - - test("accepts body.conversationId (canonical field) and echoes both names", async () => { - const requested = "client-id-bbb"; - const result = await callCreate({ conversationId: requested }); - - const mapping = getConversationByKey(requested); - expect(mapping).not.toBeNull(); - expect(mapping?.conversationId).toBe(result.id as string); - - expect(result.conversationId).toBe(requested); - expect(result.conversationKey).toBe(requested); - }); - - test("prefers body.conversationId over body.conversationKey when both are sent", async () => { - const idValue = "wins-ccc"; - const keyValue = "loses-ddd"; - - const result = await callCreate({ - conversationId: idValue, - conversationKey: keyValue, - }); - - // The "winning" id materialised a key-store mapping; the "losing" one - // never did. - const idMapping = getConversationByKey(idValue); - const keyMapping = getConversationByKey(keyValue); - expect(idMapping?.conversationId).toBe(result.id as string); - expect(keyMapping).toBeNull(); - - expect(result.conversationId).toBe(idValue); - expect(result.conversationKey).toBe(idValue); - }); - - test("mints a UUID when neither field is sent", async () => { - const result = await callCreate({}); - - // UUIDs are 36 characters (8-4-4-4-12). - expect(typeof result.conversationId).toBe("string"); - expect((result.conversationId as string).length).toBe(36); - // Both response fields carry the same minted value. - expect(result.conversationKey).toBe(result.conversationId); - }); -}); diff --git a/assistant/src/runtime/routes/conversation-management-routes.ts b/assistant/src/runtime/routes/conversation-management-routes.ts index e721a8a9965..fc2cc3a27bc 100644 --- a/assistant/src/runtime/routes/conversation-management-routes.ts +++ b/assistant/src/runtime/routes/conversation-management-routes.ts @@ -85,14 +85,9 @@ function cancelScheduleIfLast(conversationId: string): void { // --------------------------------------------------------------------------- function handleCreateConversation({ body = {}, headers }: RouteHandlerArgs) { - // LUM-1890 Phase 1: bilingual acceptance for the idempotency key. Prefer - // the canonical `conversationId` when the client sends it; fall back to - // the legacy `conversationKey`. When neither is sent, mint a UUID. - const requestedConversationKey = - (body.conversationId as string | undefined) ?? - (body.conversationKey as string | undefined) ?? - crypto.randomUUID(); - const result = getOrCreateConversation(requestedConversationKey, { + const conversationKey = + (body.conversationKey as string | undefined) ?? crypto.randomUUID(); + const result = getOrCreateConversation(conversationKey, { conversationType: "standard", }); if (result.created) { @@ -106,20 +101,21 @@ function handleCreateConversation({ body = {}, headers }: RouteHandlerArgs) { log.info( { conversationId: result.conversationId, - conversationKey: requestedConversationKey, + conversationKey, created: result.created, }, "Created conversation via POST", ); - // The response emits both `conversationId` (new canonical name) and - // `conversationKey` (legacy echo). Both carry the same value — whatever - // the client supplied (or the UUID we minted). The web client will - // migrate to reading `conversationId` in LUM-1890 Phase 2; for now the - // additive field is forward-compat only. + // `id` is the assistant-minted internal `conversations.id` — the + // authoritative identifier for this conversation. `conversationKey` + // echoes the optional external key supplied by the client (or the + // UUID we minted) and is the identifier non-vellum channel adapters + // (Telegram, WhatsApp, etc.) use to scope to a logical channel + // thread. Vellum-web clients can ignore `conversationKey` and use + // `id` directly. return { id: result.conversationId, - conversationId: requestedConversationKey, - conversationKey: requestedConversationKey, + conversationKey, conversationType: normalizeConversationType(result.conversationType), created: result.created, }; @@ -437,17 +433,11 @@ export const ROUTES: RouteDefinition[] = [ description: "Create or get an existing conversation by key.", tags: ["conversations"], requestBody: z.object({ - conversationId: z - .string() - .optional() - .describe( - "Idempotency key for the conversation (canonical name; preferred over conversationKey).", - ), conversationKey: z .string() .optional() .describe( - "Legacy synonym for conversationId. When both are sent, conversationId wins.", + "Optional external key. Echoed back in the response. Non-vellum channels (Telegram, WhatsApp) use this to scope to a logical channel thread; vellum-web clients can omit it and rely on the assistant-minted `id`.", ), conversationType: z .literal("standard") @@ -455,16 +445,15 @@ export const ROUTES: RouteDefinition[] = [ .describe("Only standard conversations are created by this endpoint"), }), responseBody: z.object({ - id: z.string(), - conversationId: z + id: z .string() .describe( - "Echo of the requested conversationId/conversationKey (canonical name).", + "Assistant-minted internal conversation id. The authoritative identifier for the conversation.", ), conversationKey: z .string() .describe( - "Legacy echo of the requested conversationId/conversationKey. Carries the same value as conversationId.", + "Echo of the optional external key supplied by the client (or the value the daemon minted when omitted).", ), conversationType: z.string(), created: z.boolean(), diff --git a/assistant/src/runtime/routes/conversation-routes.ts b/assistant/src/runtime/routes/conversation-routes.ts index 4d8c888ecf9..d56631a22d8 100644 --- a/assistant/src/runtime/routes/conversation-routes.ts +++ b/assistant/src/runtime/routes/conversation-routes.ts @@ -121,7 +121,12 @@ import { resolveTrustContext, withSourceChannel, } from "../trust-context-resolver.js"; -import { BadRequestError, InternalError, RouteError } from "./errors.js"; +import { + BadRequestError, + InternalError, + NotFoundError, + RouteError, +} from "./errors.js"; import type { RouteDefinition, RouteHandlerArgs } from "./types.js"; import { RouteResponse } from "./types.js"; @@ -1223,12 +1228,6 @@ export async function handleSendMessage( ): Promise { const body = (rawBody ?? {}) as { conversationKey?: string; - // `conversationId` is the canonical client-facing name and is accepted - // as a synonym for `conversationKey` here. When both are present, - // `conversationId` wins. Both ultimately resolve to a value that - // `getOrCreateConversation` looks up in the `conversation_keys` table - // (either as an existing internal id or as an external key). See - // LUM-1890 Phase 1 for the bilingual rollout plan. conversationId?: string; content?: string; attachmentIds?: string[]; @@ -1265,12 +1264,10 @@ export async function handleSendMessage( headers?.["x-vellum-client-id"]?.trim() || undefined; const { conversationKey, content, attachmentIds } = body; - // Bilingual acceptance: prefer the canonical `conversationId` when the - // client sends it; fall back to the legacy `conversationKey` field. - // Not destructured here because nested closures inside this handler - // already shadow the `conversationId` identifier with the resolved - // internal id (e.g. `const conversationId = mapping.conversationId`). - const inboundConversationKey = body.conversationId ?? conversationKey; + const inboundConversationId = + typeof body.conversationId === "string" && body.conversationId.length > 0 + ? body.conversationId + : undefined; const clientMessageId = typeof body.clientMessageId === "string" ? body.clientMessageId : undefined; const requestedInferenceProfile = @@ -1338,13 +1335,6 @@ export async function handleSendMessage( ? (canonicalizeTimeZone(body.clientTimezone) ?? undefined) : undefined; - // When both conversationId and conversationKey are omitted, derive a - // stable default from sourceChannel + sourceInterface so that repeated - // calls from the same channel/interface pair share a single conversation - // thread. - const resolvedConversationKey = - inboundConversationKey ?? `default:${sourceChannel}:${sourceInterface}`; - // Reject non-string content values (numbers, objects, etc.) if (content != null && typeof content !== "string") { throw new BadRequestError("content must be a string"); @@ -1408,9 +1398,40 @@ export async function handleSendMessage( // timer so the next heartbeat is a full interval after this interaction. HeartbeatService.getInstance()?.resetTimer(); - const mapping = getOrCreateConversation(resolvedConversationKey, { - conversationType: "standard", - }); + // Resolve the target conversation. Fetch by `conversationId` (the + // assistant-minted internal id) when the client supplies it — clients + // must obtain this id from a prior daemon response, so a missing row + // is a 404. Otherwise fall through to the external-key path: the + // client-supplied `conversationKey` (used by non-vellum channels and + // the web idempotency flow) or, when neither is provided, a stable + // default keyed on sourceChannel + sourceInterface so repeated calls + // from the same channel/interface share a single thread. + let mapping: { + conversationId: string; + conversationType: string; + created: boolean; + }; + if (inboundConversationId !== undefined) { + const existing = getConversation(inboundConversationId); + if (!existing) { + throw new NotFoundError( + `Conversation ${inboundConversationId} not found`, + ); + } + mapping = { + conversationId: existing.id, + conversationType: existing.conversationType, + created: false, + }; + } else { + const resolvedConversationKey = + conversationKey && conversationKey.length > 0 + ? conversationKey + : `default:${sourceChannel}:${sourceInterface}`; + mapping = getOrCreateConversation(resolvedConversationKey, { + conversationType: "standard", + }); + } if (requestedRiskThreshold !== undefined) { const result = await ipcCall("set_conversation_threshold", { diff --git a/assistant/src/runtime/routes/events-routes.ts b/assistant/src/runtime/routes/events-routes.ts index f14a682595b..4a2fec318a7 100644 --- a/assistant/src/runtime/routes/events-routes.ts +++ b/assistant/src/runtime/routes/events-routes.ts @@ -26,6 +26,7 @@ import { z } from "zod"; import type { HostProxyCapability } from "../../channels/types.js"; import { parseInterfaceId, supportsHostProxy } from "../../channels/types.js"; import { emitContactChange } from "../../contacts/contact-events.js"; +import { getConversation } from "../../memory/conversation-crud.js"; import { getOrCreateConversation } from "../../memory/conversation-key-store.js"; import { getLogger } from "../../util/logger.js"; import { formatSseFrame, formatSseHeartbeat } from "../assistant-event.js"; @@ -39,7 +40,11 @@ import { assistantEventHub, } from "../assistant-event-hub.js"; import { resolveActorPrincipalIdForLocalGuardian } from "../local-actor-identity.js"; -import { BadRequestError, ServiceUnavailableError } from "./errors.js"; +import { + BadRequestError, + NotFoundError, + ServiceUnavailableError, +} from "./errors.js"; import type { RouteDefinition, RouteHandlerArgs } from "./types.js"; const log = getLogger("events-routes"); @@ -220,14 +225,17 @@ const defaultSseShedReporter: SseShedReporter = (reason, inst) => { * Stream assistant events as Server-Sent Events. * * Query params: - * conversationId -- optional; canonical client-facing name for the scope - * identifier. When provided, scopes the stream to one - * conversation. - * conversationKey -- optional; legacy synonym for `conversationId`. When - * both are sent, `conversationId` wins. When both are - * omitted, the stream delivers events from ALL - * conversations for this assistant. See LUM-1890 Phase 1 - * for the bilingual rollout. + * conversationId -- optional; assistant-minted internal conversation id. + * When provided, the stream is scoped to that one + * conversation; the daemon 404s if no such conversation + * exists (clients must obtain the id from a prior + * response). + * conversationKey -- optional; external key (non-vellum channels) or the + * web idempotency key. Resolved via the conversation + * keys table; materializes a row on first use. + * Ignored when `conversationId` is also provided. + * When both are omitted, the stream delivers events from ALL + * conversations for this assistant. * * Headers (optional): * X-Vellum-Client-Id -- stable per-install UUID identifying this client. @@ -253,12 +261,6 @@ export function handleSubscribeAssistantEvents( ): ReadableStream { const { queryParams, headers, abortSignal } = args; - // LUM-1890 Phase 1: accept the canonical `conversationId` query param - // as a synonym for the legacy `conversationKey`. When both are present, - // `conversationId` wins. Internally we keep the variable named - // `conversationKey` because downstream call sites (the SSE instrumentation - // record + the `getOrCreateConversation` lookup) treat this value as the - // external key in the `conversation_keys` table. const rawConversationId = queryParams?.conversationId; const rawConversationKey = queryParams?.conversationKey; if ( @@ -273,7 +275,6 @@ export function handleSubscribeAssistantEvents( ) { throw new BadRequestError("conversationKey must not be empty"); } - const conversationKey = rawConversationId ?? rawConversationKey; // ── Client identity from headers ────────────────────────────────────── const rawClientId = headers?.["x-vellum-client-id"]; @@ -315,10 +316,25 @@ export function handleSubscribeAssistantEvents( "host_browser", ]; + // Resolve the scope. `conversationId` (when supplied) is the + // assistant-minted internal id — looked up directly; 404 if absent. + // Otherwise fall through to `conversationKey`, which is treated as an + // external key and resolved via the conversation_keys table + // (materialized on first use, preserving the existing subscribe-time + // create behavior for the web idempotency flow). const filter: AssistantEventFilter = {}; - if (conversationKey) { - const mapping = getOrCreateConversation(conversationKey); + let scopeConversationKey: string | null = null; + if (rawConversationId) { + const existing = getConversation(rawConversationId); + if (!existing) { + throw new NotFoundError(`Conversation ${rawConversationId} not found`); + } + filter.conversationId = existing.id; + scopeConversationKey = existing.id; + } else if (rawConversationKey) { + const mapping = getOrCreateConversation(rawConversationKey); filter.conversationId = mapping.conversationId; + scopeConversationKey = rawConversationKey; } const encoder = new TextEncoder(); @@ -338,7 +354,7 @@ export function handleSubscribeAssistantEvents( heartbeatsSent: 0, clientId, interfaceId, - conversationKey: conversationKey ?? null, + conversationKey: scopeConversationKey, }; ensureEventLoopDelayMonitorStarted(); @@ -495,12 +511,12 @@ export const ROUTES: RouteDefinition[] = [ { name: "conversationId", description: - "Scope to a single conversation (canonical name; preferred when sending fresh requests).", + "Scope to a single conversation by its assistant-minted internal id. 404s if no such conversation exists.", }, { name: "conversationKey", description: - "Scope to a single conversation (legacy synonym for conversationId).", + "Scope to a single conversation by an external key (non-vellum channels) or the web idempotency key. Materializes a row on first use. Ignored when conversationId is also provided.", }, ], responseHeaders: { diff --git a/assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts b/assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts deleted file mode 100644 index 9f86770a2ed..00000000000 --- a/assistant/src/runtime/services/__tests__/conversation-serializer-bilingual.test.ts +++ /dev/null @@ -1,92 +0,0 @@ -/** - * LUM-1890 Phase 1 — `serializeConversationSummary` emits the canonical - * `conversationId` alongside the legacy `id` field. - * - * Both fields carry the same value (the daemon's internal `conversations.id`). - * The `conversationId` field is forward-compat: it lets clients that prefer - * the canonical name read it directly without re-mapping, enabling the - * Phase 2 web migration to drop the `id` → `conversationId` mapping in - * `parseConversation`. - * - * Removable from the wire when Phase 4 deprecates `id` (after macOS catches - * up). Until then, both fields are emitted. - */ - -import { describe, expect, mock, test } from "bun:test"; - -mock.module("../../../util/logger.js", () => ({ - getLogger: () => - new Proxy({} as Record, { - get: () => () => {}, - }), -})); - -import type { ConversationRow } from "../../../memory/conversation-crud.js"; -import { serializeConversationSummary } from "../conversation-serializer.js"; - -function buildConversationRow( - overrides: Partial = {}, -): ConversationRow { - const now = Date.now(); - return { - id: "conv-internal-id-123", - title: "Test conversation", - createdAt: now, - updatedAt: now, - totalInputTokens: 0, - totalOutputTokens: 0, - totalEstimatedCost: 0, - contextSummary: null, - contextCompactedMessageCount: 0, - contextCompactedAt: null, - historyStrippedAt: null, - slackContextCompactionWatermarkTs: null, - slackContextCompactionWatermarkAt: null, - conversationType: "standard", - source: "user", - memoryScopeId: "scope-1", - originChannel: null, - originInterface: null, - forkParentConversationId: null, - forkParentMessageId: null, - ...overrides, - } as ConversationRow; -} - -describe("serializeConversationSummary — Phase 1 bilingual emission", () => { - test("emits both `id` and `conversationId` with the same internal id value", () => { - const conversation = buildConversationRow({ id: "conv-bilingual-abc" }); - const out = serializeConversationSummary({ - conversation, - parentCache: new Map(), - }); - - expect(out.id).toBe("conv-bilingual-abc"); - expect(out.conversationId).toBe("conv-bilingual-abc"); - // Forward-compat guarantee: both fields always carry the same value - // until Phase 4 drops `id`. Any divergence is a contract bug. - expect(out.id).toBe(out.conversationId); - }); - - test("conversationId tracks id even when binding/attention/displayMeta are set", () => { - const conversation = buildConversationRow({ id: "conv-with-meta-xyz" }); - const out = serializeConversationSummary({ - conversation, - displayMeta: { - displayOrder: 5, - isPinned: true, - groupId: "group-7", - }, - parentCache: new Map(), - }); - - // The serializer return type is a discriminated union over the - // display-meta variants; reach in via Record for - // the optional fields rather than narrowing. - const outAsRecord = out as Record; - expect(out.id).toBe("conv-with-meta-xyz"); - expect(out.conversationId).toBe("conv-with-meta-xyz"); - expect(outAsRecord.isPinned).toBe(true); - expect(outAsRecord.displayOrder).toBe(5); - }); -}); diff --git a/assistant/src/runtime/services/conversation-serializer.ts b/assistant/src/runtime/services/conversation-serializer.ts index fb676bf3dcf..c002f73a32e 100644 --- a/assistant/src/runtime/services/conversation-serializer.ts +++ b/assistant/src/runtime/services/conversation-serializer.ts @@ -171,13 +171,6 @@ export function serializeConversationSummary(params: { return { id: conversation.id, - // LUM-1890 Phase 1: emit `conversationId` alongside `id` as a - // forward-compat field so the web's `parseConversation` (and any - // other client that prefers the canonical name) can read it directly - // without re-mapping. Both carry the same internal conversations.id - // value. Phase 2 will migrate web off `id`/`conversationKey` onto - // `conversationId`; Phase 4 eventually drops `id`. - conversationId: conversation.id, title: conversation.title ?? "Untitled", createdAt: conversation.createdAt, updatedAt: conversation.updatedAt,