feat: server-mint vellum conversations and gate web client to opt in#32234
Conversation
postChatMessage(conversationId: null) omits both wire fields so the daemon mints a fresh row on empty-handed sourceChannel="vellum" sends. The response's conversationId becomes resolvedConversationId, and the existing draft-key-resolution path in sendMessage swaps optimistic state and navigates to the canonical URL. Sender side (use-send-message.ts): - sendMessageViaStream now takes isDraft and gates on supportsServerMintedConversation() to pick null vs. requestConversationId. - effectiveConversationId narrowed defensively — postChatMessage's success contract guarantees a non-null id (the early-return on server-mint without a daemon-echoed id surfaces 422 first). API side (messages.ts): - PostMessageResult.conversationId: string | null on success arms. - Body composition omits both wire fields when conversationId is null; pickConversationIdWireField stays in charge of the non-null path. - 422 early-return when server-mint was requested but the daemon accepted the message without echoing a conversation id back. Tests (post-chat-message.test.ts): - Server-mint flow describe block: omit both fields, return minted id as resolvedConversationId, 422 on broken daemon contract, and a guard that the legacy non-null path is unchanged. Queue path (line 519) left unchanged — it requires an existing conversation by construction. The tight race where a draft's second send fires before the first's resolveDraft swap is a known corner case the existing wire-field gate also hits; will revisit if it surfaces.
![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: 21dcd46002
ℹ️ 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".
| const useServerMint = isDraft && supportsServerMintedConversation(); | ||
| const postResult = await postChatMessage( | ||
| requestAssistantId, | ||
| requestConversationId, | ||
| useServerMint ? null : requestConversationId, |
There was a problem hiding this comment.
When this server-minted path is used for a fresh draft, activeConversationId remains the client-only draft key until the first POST returns and resolveDraftKey runs. If the user submits a second message during that window, the sendMessage queue branch still posts with activeConversationId; on assistants that pass this gate, pickConversationIdWireField() serializes that value as conversationId, and the backend's strict lookup 404s because the server minted a different ID. The second message is reverted instead of being queued, so the server-minted flow needs to hold/route queued sends until the resolved ID is available (or keep using the create-or-lookup key for that pending draft).
Useful? React with 👍 / 👎.
| conversationId: string; | ||
| /** Caller's input id, echoed back. `null` when the caller asked the | ||
| * assistant to mint a fresh conversation (server-minted flow). */ | ||
| conversationId: string | null; |
There was a problem hiding this comment.
no, the PostMessageResult should always return a conversationId. it's the authoritative conversation id that the server side now uses, regardless of whether the frontend passes one in or not.
| return Array.isArray(data?.messages) ? data.messages : []; | ||
| } | ||
|
|
||
| export type PostMessageResult = |
There was a problem hiding this comment.
In a followup PR, let's add this to our assistant/src/api/responses/*.ts, similar to events
| // Server-minted flow contract: when the caller omits both wire fields, | ||
| // the assistant MUST echo the freshly minted id back so the caller | ||
| // can navigate to its URL. An assistant that returns 202 + accepted | ||
| // without an id is broken; surface as a failure rather than letting | ||
| // the caller try to thread a `null` conversation id through downstream | ||
| // code. | ||
| if (conversationId === null && resolvedConversationId === undefined) { | ||
| return { | ||
| ok: false, | ||
| status: 422, | ||
| error: { | ||
| detail: | ||
| "Assistant accepted the message but did not return a conversation id.", | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
in the followup PR, once we have PostMessageResult in the vellumai/assistant-api, we could use the zod schema to confirm the valid response shape and conversationId will be defined at that point
| throw new Error( | ||
| "postChatMessage returned success without a conversation id", | ||
| ); |
There was a problem hiding this comment.
return status failed instead of page crash if this is still relevant
| * use `useAssistantSupports(MIN_VERSION)` from `./utils.ts` directly. | ||
| */ | ||
| import { useAssistantIdentityStore } from "@/stores/assistant-identity-store"; | ||
| import { compareParsed, parseSemver } from "@/utils/semver"; |
There was a problem hiding this comment.
don't duplicate the helper's logic that we already have
- PostMessageResult.conversationId is now always string (was string | null in the server-mint flow). The assistant is the authoritative source of truth; drop the redundant resolvedConversationId field on both result shapes. - Always send conversationKey on the wire when the caller has an id so pre-0.8.6 assistants (which don't read conversationId) still resolve the conversation via external-key lookup. On 0.8.6+ assistants, also send conversationId to trigger the strict internal-id lookup. The duplicated id is harmless: pre-0.8.6 ignores the unknown conversationId key; 0.8.6+ reads conversationId first. - Add non-hook assistantSupports(minVersion) to lib/backwards-compat/utils.ts so both the React hook (useAssistantSupports) and event-handler/async call sites share the same semver-comparison core. Refactor supportsServerMintedConversation() to call it instead of duplicating the parse/compare logic. - Replace the invariant throw in use-send-message.ts with a typecheck- enforced contract. postChatMessage's success arm now guarantees a non-empty conversationId so the runtime throw is no longer needed. - Codex P2 — server-mint queue race: add pendingDraftMintRef so the queue path in sendMessage awaits the in-flight first-message POST before posting follow-up messages. Without this, a follow-up submitted during the mint window would 404 on 0.8.6+ assistants (the assistant minted a different id from the still-unresolved local draft key). - Update tests: wire-field cutover now asserts both fields on 0.8.6+; server-mint tests assert the authoritative-id contract; add coverage for caller-supplied id being overridden by the assistant's response. Followup work (NOT in this PR): - Add PostMessageResult to assistant/src/api/responses/*.ts and zod- validate the response shape in postChatMessage (Vargas comments #2 + #4).
| body.conversationKey = conversationId; | ||
| if (pickConversationIdWireField() === "conversationId") { | ||
| body.conversationId = conversationId; | ||
| } |
There was a problem hiding this comment.
still two broken things here. This is what I think we want:
const conversationField = pickConversationIdWireField();
if (conversationId !== null || conversationField !== "conversationId") {
body[conversationField] = conversationId
}basically we still need to send the old field in null cases
| const pendingDraftMintRef = useRef<{ | ||
| draftId: string; | ||
| promise: Promise<string>; | ||
| } | null>(null); |
There was a problem hiding this comment.
We can simplify this by not allowing any sends while one is in flight. it should 200 quickly
Round 3 PR feedback (#32234): 1. messages.ts — pick exactly ONE wire field instead of duplicating conversationKey alongside conversationId on 0.8.6+ assistants. - 0.8.6+ with caller id: conversationId only - 0.8.6+ with null: neither (server-mint branch) - pre-0.8.6 with id: conversationKey only - pre-0.8.6 with null: conversationKey: null (legacy create-or-lookup remains addressed; defense-in-depth for callers that bypass the supportsServerMintedConversation gate) 2. use-send-message.ts — replace the deferred-promise race guard with a simpler "block any send while a mint is in flight" gate. The POST 200s quickly, so rejecting follow-up sends with a brief inline error is simpler than threading an unresolved id through the queue path. - pendingDraftMintRef: useRef<string | null> (was { draftId, promise }) - set before POST in sendMessageViaStream, cleared after resolve/reject - sendMessage entry returns early if the gate matches the active draft Tests updated to match the new contract; added coverage for conversationKey: null on pre-0.8.6 callers. 27/27 pass; tsc + eslint clean on touched files; assistant send-endpoint-busy 15/15 unchanged.
1 participant
Problem
When the web client posts an empty-handed
/sendto the assistant (noconversationId,sourceChannel: "vellum"), it relies on streaming theserver-side id back over the wire. That worked, but the chat UI then
called
/conversations/:idfor sidebar metadata before theconversationIdevent had landed on the client — producing a 404 and a"new conversation, then immediate 404" flash. PR #32146 was the stopgap
(bumped
pickConversationIdWireField's MIN_VERSION 0.8.5 → 0.8.6 toensure newer clients always send the id) — this PR is the real fix.
Shape of the fix
Assistant (
conversation-routes.ts):handleSendMessagenow mints a fresh UUID when the call arriveswith
sourceChannel: "vellum"and an emptyconversationId. Otherchannels are untouched (their id allocation rules differ).
client already consumes, so the wire shape doesn't change.
Web (
server-minted-conversation.ts— new compat gate):supportsServerMintedConversation()— MIN_VERSION0.8.6. Mirrorsthe convention from
pickConversationIdWireField(fix(web): park conversation-id wire-field cutover above current daemon (0.8.6) #32146). Clientsolder than 0.8.6 keep the legacy flow.
Web (
messages.ts):postChatMessage'sconversationIdargument widened tostring | nullso callers can ask for a server-mint.the assistant didn't echo an id (contract violation — would otherwise
silently break downstream
useChatHistory).Web (
use-send-message.ts):isDraftarg threaded intosendMessageViaStream.useServerMint = isDraft && supportsServerMintedConversation()gate decides which path to take.
throwguards the "we asked for mint but got nothing back" case (will
land in Sentry if the contract ever drifts).
Tests
assistant/src/__tests__/send-endpoint-busy.test.ts: new coveragefor the mint-on-empty path; legacy non-vellum behavior unchanged.
apps/web/src/lib/backwards-compat/server-minted-conversation.test.ts:version gate behavior at and around 0.8.6.
apps/web/src/domains/chat/api/post-chat-message.test.ts: 4 newscenarios — wire field omitted when null, minted id surfaces as
resolvedConversationId, 422 on broken contract, legacy non-nullpath unchanged.
All green locally:
bun test post-chat-message.test.ts server-minted-conversation.test.ts→ 18/18bun test assistant/src/__tests__/send-endpoint-busy.test.ts→ 15/15bunx tsc --noEmitclean for touched files (web + assistant)eslintcleanNotes / Followups
server-minted-conversation.{ts,test.ts}files still use"daemon" in their comments — the May 24 "daemon → assistant" rename
pass hadn't reached these new files yet. Happy to do that sweep in
a follow-up if you want it separate, or amend in here.
pickConversationIdWireField)can stay shipped — both gates are compatible and the version floors
match.