refactor(assistant): daemon goes bilingual on conversationKey/conversationId wire fields (LUM-1890 Phase 1)#31922
Conversation
…ationId wire fields (LUM-1890 Phase 1) 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c35432ebfc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 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; |
There was a problem hiding this comment.
When both fields are present, conversationId is chosen with ??, so an empty string ("") still wins over a valid conversationKey. In that case resolvedConversationKey becomes empty and getOrCreateConversation("") is used, which can route unrelated requests into the same empty-key thread instead of the caller’s intended conversation. This is a regression for mixed clients that send both fields during migration and occasionally emit blank conversationId values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved as a side effect of switching to the dual-lookup pattern (commit 3d95ad1). Empty conversationId is now rejected with BadRequestError before it can ever shadow a valid conversationKey.
| const requestedConversationKey = | ||
| (body.conversationId as string | undefined) ?? | ||
| (body.conversationKey as string | undefined) ?? | ||
| crypto.randomUUID(); |
There was a problem hiding this comment.
The new precedence logic also uses conversationId ?? conversationKey, so conversationId: "" overrides a non-empty conversationKey. That causes POST /v1/conversations to create/lookup mappings under the empty key and echo the empty value back, which can silently collapse multiple creates into the same mapped conversation key. The bilingual precedence should only prefer conversationId after non-empty validation (or fall back to conversationKey).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Moot — POST /v1/conversations no longer accepts conversationId on input (commit 3d95ad1, per maintainer review).
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.
| const conversationKey = | ||
| (body.conversationKey as string | undefined) ?? crypto.randomUUID(); | ||
| const result = getOrCreateConversation(conversationKey, { | ||
| // LUM-1890 Phase 1: bilingual acceptance for the idempotency key. Prefer |
There was a problem hiding this comment.
Don't mention linear tickets in code comments
There was a problem hiding this comment.
Stripped all LUM-1890 mentions from prod and test code in commit 3d95ad1. Will keep them to commit messages / PR descriptions / notes.
| const requestedConversationKey = | ||
| (body.conversationId as string | undefined) ?? |
There was a problem hiding this comment.
The create conversation endpoint must not support conversationId. Conversation id is assistant controlled, not client controlled. The frontend will support a some optimistic ui flow when we get to it
There was a problem hiding this comment.
Reverted in 3d95ad1. The create endpoint now only accepts conversationKey again. Will plan the optimistic-UI flow on the frontend side when we get there.
| responseBody: z.object({ | ||
| id: z.string(), | ||
| conversationKey: z.string(), | ||
| conversationId: z |
There was a problem hiding this comment.
This is redundant with id, the frontend should start cutting over to conversation.id when the record is a Conversation record and conversationId when the record is something else
There was a problem hiding this comment.
Reverted in 3d95ad1. serializeConversationSummary and the create response both stopped emitting the additive conversationId field. The frontend principle (record.id for Conversation records / record.conversationId for non-Conversation records) is recorded for the Phase 2 web migration.
| conversationKey: z | ||
| .string() | ||
| .describe( | ||
| "Legacy echo of the requested conversationId/conversationKey. Carries the same value as conversationId.", |
There was a problem hiding this comment.
This should still represent the optional external conversation key for non vellum channels
There was a problem hiding this comment.
Kept in 3d95ad1. The response still emits conversationKey and the description now spells out that it represents the optional external key for non-vellum channels.
| // `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. |
There was a problem hiding this comment.
Delete this comment
| // 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; |
There was a problem hiding this comment.
Don't combine these fields, instead fetch by one and then the other
There was a problem hiding this comment.
Restructured in 3d95ad1 to use the same dual-lookup pattern as handleListMessages further up this file. If conversationId is supplied, looked up directly via getConversation (404 if missing). Otherwise falls through to the existing conversationKey path. Applied the same shape to handleSubscribeAssistantEvents. No more ?? between them.
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.
| // `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. |
There was a problem hiding this comment.
Delete this comment
There was a problem hiding this comment.
Queued for the next assistant-touching PR — tracked at notes/pending-followups.md with the exact patch ready to apply.
| conversationKey: z | ||
| .string() | ||
| .describe( | ||
| "Echo of the optional external key supplied by the client (or the value the daemon minted when omitted).", |
There was a problem hiding this comment.
Remove the parenthesis
There was a problem hiding this comment.
Queued for the next assistant-touching PR — tracked at notes/pending-followups.md with the exact patch ready to apply.
…es + GET /v1/events Phase 2 of the LUM-1890 wire-format migration. The web client now sends the canonical `conversationId` field to daemons that support it, and falls back to the legacy `conversationKey` for daemons that don't. The version gate adopts the `lib/backwards-compat/` pattern established in #31932: apps/web/src/lib/backwards-compat/conversation-id-wire-field.ts - MIN_VERSION = "0.8.5" - pickConversationIdWireField(): "conversationId" | "conversationKey" The helper reads the version snapshot via `useAssistantIdentityStore.getState()` rather than the `use.version()` hook selector, so it's safe to call from the async `postChatMessage` and `subscribeChatEvents` paths. Semver parsing reuses the shared `@/utils/semver.js` utilities (so behavior matches the existing `useAssistantSupports` hook): pre-release suffixes count as the full patch version, `v` prefix is stripped, unparseable / missing versions fall back to the legacy field. ## Why the gate The web client and the macOS daemon ship on independent release cadences. Users update the daemon by installing a fresh macOS app build; the web is whatever is currently deployed at app.vellum.ai. The web is usually ahead of the local daemon. When we introduce a new wire field we must keep speaking the old field name to daemons that don't know the new one yet. Cutover threshold: daemon >= 0.8.5. Phase 1 (#31922) will ride in 0.8.5; current main is 0.8.4. ## Wire sites updated Two outbound sites in apps/web: - `postChatMessage` (`apps/web/src/domains/chat/api/messages.ts`) — `POST /v1/messages` body. - `subscribeChatEvents` (`apps/web/src/domains/chat/api/stream.ts`) — `GET /v1/events` query. Both now do `[pickConversationIdWireField()]: conversationId` to pick the right wire-field name on each call. ## Tests - `lib/backwards-compat/conversation-id-wire-field.test.ts` — 5 tests covering: unknown version → conversationKey, 0.8.4 / 0.7.0 → conversationKey, 0.8.5 / 0.8.6 / 0.9.0 / 1.0.0 → conversationId, RC builds (0.8.5-rc.1, 0.8.5-beta) → conversationId, unparseable versions → conversationKey. (Exhaustive semver-edge tests already live in `utils.test.ts`.) - `post-chat-message.test.ts` — adds a `postChatMessage wire-field bilingual cutover` describe block (4 tests) verifying the matrix at the integration level (asserting against the actual outbound request body). - `stream.test.ts` — adds 3 bilingual tests mirroring the same matrix for the SSE subscribe query param, and broadens the existing `omits ... query when subscribing to all assistant events` test to confirm both wire fields are absent (was only checking `conversationKey`). - Existing tests get `clearIdentity()` in `beforeEach`/`afterEach` so the version-gate defaults to the conservative legacy path. ## Out of scope (this PR) - `event-parser.ts` reads inbound `conversationKey` from event payloads; daemon still emits this in deep-link metadata. - `routes.tsx` URL-redirect that translates legacy `?conversationKey=` URLs in-app to `?conversationId=` is a URL semantics shim, not a wire-format concern. - Inspector / event-bus / type-definition references are inbound or client-internal, not wire-out. ## Out of scope (later phases) - Phase 3: macOS Swift client cutover. - Phase 4: daemon drops legacy `conversationKey` from the wire. Ships after macOS catches up. The version gate above means newer web clients gracefully downshift against older daemons; the breaking direction (older web talking to a Phase-4 daemon) we control via the deploy gate. Refs LUM-1890.
…rmat migration (#31960) Phase 1 (#31922) made the daemon bilingual; Phase 2 (#31944) cut web over to conversationId with a 0.8.5+ version gate. Phase 3 (macOS Swift) and Phase 4 (drop conversationKey) are explicitly out of scope — desktop is moving to Electron-of-web and the bilingual routes will continue to support both fields indefinitely for non-vellum channel adapters (Telegram, WhatsApp, integrations). Docs: - docs/internal-reference.md: GET /v1/assistants/:id/messages and GET /v1/events now document conversationId as the preferred field and conversationKey as the legacy alias for external channel adapters. Curl example flipped to ?conversationId. - apps/web/docs/CONVENTIONS.md: rewrote Rule 1 to point at lib/backwards-compat/conversation-id-wire-field.ts and remove the 'incremental cleanup' framing — wire-format work is locked in. Rule 3 reframed: conversationKey is retained for external channel adapters, web uses conversationId. - apps/web/docs/EVENT_BUS.md: bus consumers filter on payload.conversationId, not payload.conversationKey (events emit conversationId at the envelope level). - assistant/ARCHITECTURE.md: mermaid SSE_ROUTE node lists both query params. Code (queued follow-ups from #31922 review): - conversation-management-routes.ts: dropped a 7-line narration block inside handleCreateConversation explaining id vs conversationKey (already documented in the Zod schema). - conversation-management-routes.ts: tightened the conversationKey Zod response description from 'Echo of the optional external key supplied by the client (or the value the daemon minted when omitted).' to 'Echo of the optional external key supplied by the client.' OpenAPI regenerated (single-line description delta). Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
1 participant
What
Daemon accepts a new
conversationIdinput alongside the existingconversationKeyon its client-facing surfaces, using strict dual-lookup semantics (not synonyms). Non-breaking, additive. Phase 2 migrates the web client ontoconversationId.Why this isn't a simple rename
Per the investigation at
notes/daemon-wire-format-investigation.md:conversationKeyon the daemon is a first-class concept, not legacy naming. The internalconversation_keystable (many external opaque keys → one internalconversations.id) stays and continues to serve messaging adapters (Telegram, WhatsApp, etc.).The wire-field cleanup only applies to client-facing surfaces where the web client previously had to send its locally-minted UUID as
conversationKey. After Phase 1, web (and any future client that knows the internal id) can sendconversationIddirectly without materializing an external key mapping.Dual-lookup contract
On
POST /v1/messagesandGET /v1/events:conversationId(when supplied) is the assistant-minted internalconversations.id. Looked up directly viagetConversation. Returns 404 if no such conversation exists — clients must obtain the id from a prior daemon response.conversationKey(when supplied) is an external key (non-vellum channels) or the web idempotency key. Resolved via theconversation_keystable and materialised on first use.conversationIdwins andconversationKeyis ignored. (Don't combine — fetch by one and then the other.)This matches the existing dual-lookup pattern already used by
handleListMessagesinconversation-routes.ts.Surfaces touched
POST /v1/messages—handleSendMessagenow branches on whetherbody.conversationIdis supplied. Internal-id path goes throughgetConversation(404 on miss). Key path retains the existinggetOrCreateConversationmaterialise-on-first-use behavior.GET /v1/events—handleSubscribeAssistantEventsadds?conversationId=with the same strict-lookup contract, retains the existing?conversationKey=path unchanged.POST /v1/conversations— left as-is. Conversation id is assistant-controlled; clients can't supply it on create. The optimistic-UI flow is a future frontend concern.Conversation serializer / list payloads — unchanged. The response field stays
id. The frontend principle (userecord.idfor Conversation records /record.conversationIdfor non-Conversation records like events/messages) is recorded for the Phase 2 web migration.SSE hub + deepLinkMetadata — no-op audit. Internal types already use
conversationId.Validation
bun run typecheckclean.bun run lintclean.bun run generate:openapi -- --checkup to date.Phase plan
conversationKeyon the wire for vellum-channel sends. Readsrecord.idfor Conversation records /record.conversationIdfor non-Conversation records.conversationKeyfrom the wire (breaking; ships after macOS catches up). Theconversation_keystable and the messaging-channel adapter usage stay forever.