diff --git a/.changeset/setmessages-honest-surface-8342.md b/.changeset/setmessages-honest-surface-8342.md new file mode 100644 index 000000000..5547c4b48 --- /dev/null +++ b/.changeset/setmessages-honest-surface-8342.md @@ -0,0 +1,41 @@ +--- +'@object-ui/plugin-chatbot': minor +--- + +`useObjectChat().setMessages` now keeps the promise its declaration makes, and the +`chatResult as any` that was hiding the gap is gone (objectui#8342). + +The member is declared `(messages: unknown[]) => void` and the hook returned +`@ai-sdk/react`'s own `setMessages`, which accepts only its `UIMessage[]`. +Parameters are contravariant, so that assignment is unsound — `tsc` reported it as +a TS2322 the moment the cast came off, and the declared type was telling every +consumer they could hand over an arbitrary array when the function underneath +could not take one. Nothing broke only because no caller had yet taken the type at +its word; one who did got a runtime failure the compiler had blessed. + +The parameter stays `unknown[]`. This package does not republish the SDK's pinned +`UIMessage` on its own surface — the same call objectui#8214 made one file over +for `AnyPart.state`, and typing this member against the SDK would re-break it on +the next dependency bump. Instead the hook now wraps the SDK function and checks +every element first: an object with a string `id`, a `'user' | 'assistant' | +'system'` role, and a `parts` array — exactly the three members `UIMessage` +requires. `parts` is checked for array-ness only, because the part union is open +(a `data-*` part carries an author-defined payload) and restating it is the +coupling this change exists to avoid. + +**Behaviour change, and the reason this is not a patch.** A value that is not a +chat message is now REFUSED, not filtered and not passed on: the call throws a +`TypeError` naming the offending index, and the SDK's store is left untouched +because the whole array is validated before anything is written. Filtering was +rejected deliberately — this is a re-hydration path where the caller's statement +is "the thread is now exactly these messages", so dropping the failures would +install a shorter thread that the `void` return makes undetectable. The declared +TYPE does not move, so nothing that compiled stops compiling; what narrows is the +set of values a consumer can successfully pass at runtime, which is the opposite +direction from objectui#8214's widen. + +`@object-ui/app-shell`'s `useReconcileOnError` is the one in-repo consumer. Its +payload comes from `toUIMessages`, which emits exactly `id` / `role` / `parts`, so +it passes the check unchanged; and it already calls through a `try`/`catch` that +falls back to the ordinary error banner, so a future malformed server payload +degrades to "show the error" rather than to a quietly-truncated transcript. diff --git a/packages/plugin-chatbot/src/__tests__/useObjectChat.setMessagesHonest-8342.test.tsx b/packages/plugin-chatbot/src/__tests__/useObjectChat.setMessagesHonest-8342.test.tsx new file mode 100644 index 000000000..91725ba15 --- /dev/null +++ b/packages/plugin-chatbot/src/__tests__/useObjectChat.setMessagesHonest-8342.test.tsx @@ -0,0 +1,198 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `useObjectChat().setMessages` — the deliberately-loose member tells the truth + * (objectui#8342). + * + * The hook declared `setMessages?: (messages: unknown[]) => void` and returned + * the AI SDK's own function, which accepts only its `UIMessage[]`. Parameters + * are CONTRAVARIANT, so that assignment is unsound and `tsc` said so — the one + * thing suppressing it was a `chatResult as any` at the destructure. The fix + * keeps the loose parameter (this package does not republish `@ai-sdk/react`'s + * pinned `UIMessage`) and narrows INSIDE the hook, so the promise on the + * surface is one the implementation keeps. + * + * Two halves, and they only mean something together: + * + * - the COMPILE-TIME block pins the declaration. It is erased by vitest, so + * `pnpm test` proves nothing about it; only + * `pnpm --filter @object-ui/plugin-chatbot type-check` can, and this + * package's `tsconfig.test.json` is the project that reads this file. + * - the RUNTIME blocks pin what the declaration is now a statement ABOUT. + * Pinning either alone reproduces exactly the blindness that shipped: a + * declaration that disagreed with the values flowing through it. + * + * The contract under test on a non-surviving element is REFUSE LOUDLY, not + * filter and not pass through. See the member's doc comment in + * `useObjectChat.ts` for why; the assertions below are what stops a later + * change from quietly picking one of the other two. + */ + +import { renderHook, act } from '@testing-library/react'; +import { vi, describe, it, expect, beforeEach } from 'vitest'; +import { useObjectChat, type UseObjectChatReturn } from '../useObjectChat'; + +const API = 'https://example.test/api/v1/ai/agents/build/chat'; + +const { sdkSetMessages } = vi.hoisted(() => ({ sdkSetMessages: vi.fn() })); + +vi.mock('@ai-sdk/react', () => ({ + useChat: () => ({ + messages: [], + status: 'ready', + error: undefined, + sendMessage: vi.fn(), + regenerate: vi.fn(), + stop: vi.fn(), + setMessages: sdkSetMessages, + }), +})); + +/** A thread as the server persists it — `id` / `role` / `parts`, the three members `UIMessage` requires. */ +const HYDRATED = [ + { id: 'm1', role: 'user', parts: [{ type: 'text', text: 'build me an app' }] }, + { + id: 'm2', + role: 'assistant', + parts: [ + { type: 'text', text: 'Built your app.' }, + // An open `data-*` part: author-defined payload, no closed union to check + // against. It must survive — the guard checks `parts` for array-ness only. + { type: 'data-build-progress', id: 'bp-1', data: { phase: 'done' } }, + ], + }, +]; + +function renderApiMode() { + return renderHook(() => useObjectChat({ api: API, conversationId: 'c1' })); +} + +beforeEach(() => { + sdkSetMessages.mockClear(); +}); + +// --------------------------------------------------------------------------- +// Compile-time: the declaration itself. Erased at runtime — `type-check` reads +// these, vitest does not. +// --------------------------------------------------------------------------- +type Assert = T; +type IsAny = 0 extends 1 & T ? true : false; +type Equal = (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; + +type Published = UseObjectChatReturn['setMessages']; + +// Probe hygiene first: an `any` anywhere on this member would make every +// assertion below answer whatever it is asked. +type _NotAny = Assert, false>>; +type _NotAnyParam = Assert>[0]>, false>>; + +// Optional, because local mode does not expose it at all. +type _StillOptional = Assert>; + +// The card, as a type. `(messages: unknown[]) => void` and nothing else: +// re-narrowing this to the SDK's `UIMessage[]` is option A, which the ruling +// declined precisely so a dependency bump cannot move the published surface. +type _HonestSignature = Assert, (messages: unknown[]) => void>>; + +// The parameter really is the top array type — a caller may hand over anything +// an `unknown[]` can hold, and the compiler must not object. +type _AcceptsUnknownArray = Assert< + Equal>[0], unknown[]> +>; + +describe('the returned setMessages belongs to the hook, not to the SDK', () => { + it('is a wrapper, so what the declaration promises is enforced somewhere', () => { + const { result } = renderApiMode(); + // Lit control for the negative below: the member is present and callable. + expect(typeof result.current.setMessages).toBe('function'); + // The whole shape of the fix. Before it, this WAS the SDK's function and + // the declared parameter was a promise nothing kept. + expect(result.current.setMessages).not.toBe(sdkSetMessages); + }); +}); + +describe('a well-formed thread passes through untouched', () => { + it('forwards every element, in order, by identity', () => { + const { result } = renderApiMode(); + + act(() => { + result.current.setMessages?.(HYDRATED); + }); + + expect(sdkSetMessages).toHaveBeenCalledTimes(1); + const [forwarded] = sdkSetMessages.mock.calls[0] as [unknown[]]; + // Not filtered, not re-shaped: same length, same elements, same order. + expect(forwarded).toHaveLength(HYDRATED.length); + expect(forwarded[0]).toBe(HYDRATED[0]); + expect(forwarded[1]).toBe(HYDRATED[1]); + // The open `data-*` part survived — `parts` is checked for array-ness only. + expect((forwarded[1] as { parts: unknown[] }).parts).toHaveLength(2); + }); +}); + +describe('a non-message element is REFUSED, not filtered and not passed on', () => { + it('throws a TypeError naming the offending index', () => { + const { result } = renderApiMode(); + + expect(() => result.current.setMessages?.([HYDRATED[0], { oops: true }])).toThrow(TypeError); + expect(() => result.current.setMessages?.([HYDRATED[0], { oops: true }])).toThrow( + /index 1/, + ); + }); + + it('writes NOTHING — the store never sees a truncated thread', () => { + const { result } = renderApiMode(); + + expect(() => result.current.setMessages?.([HYDRATED[0], { oops: true }])).toThrow(); + + // The anti-FILTER pin, and the reason the check runs over the whole array + // before anything is handed on. A filtering implementation would have + // called the SDK exactly once here, with the single surviving message — + // installing a shorter thread that the `void` return makes undetectable. + expect(sdkSetMessages).not.toHaveBeenCalled(); + }); + + it.each([ + ['a null hole', null], + ['a primitive', 'not a message'], + ['no id', { role: 'user', parts: [] }], + ['a non-string id', { id: 7, role: 'user', parts: [] }], + ['an unknown role', { id: 'm', role: 'tool', parts: [] }], + ['no parts', { id: 'm', role: 'user' }], + ['a non-array parts', { id: 'm', role: 'user', parts: { type: 'text' } }], + ])('refuses %s', (_label, bad) => { + const { result } = renderApiMode(); + expect(() => result.current.setMessages?.([bad])).toThrow(TypeError); + expect(sdkSetMessages).not.toHaveBeenCalled(); + }); + + it('refuses a non-array argument too', () => { + const { result } = renderApiMode(); + // A JS host can reach a published surface with anything. The declared type + // says `unknown[]`; the implementation says so out loud. + const setMessages = result.current.setMessages as unknown as (m: unknown) => void; + expect(() => setMessages(null)).toThrow(TypeError); + expect(sdkSetMessages).not.toHaveBeenCalled(); + }); +}); + +describe('the in-hook caller still works through the wrapper', () => { + it('clear() empties the thread — an empty array survives the narrowing', () => { + const { result } = renderApiMode(); + + act(() => { + result.current.clear(); + }); + + expect(sdkSetMessages).toHaveBeenCalledTimes(1); + expect(sdkSetMessages).toHaveBeenCalledWith([]); + }); +}); diff --git a/packages/plugin-chatbot/src/useObjectChat.ts b/packages/plugin-chatbot/src/useObjectChat.ts index 2ae7a6284..ff06f46f9 100644 --- a/packages/plugin-chatbot/src/useObjectChat.ts +++ b/packages/plugin-chatbot/src/useObjectChat.ts @@ -356,7 +356,41 @@ export interface UseObjectChatReturn { reload: () => void; /** Clear all messages */ clear: () => void; - /** ADR-0013 D2: re-hydrate the thread (API mode only); undefined in local mode. */ + /** + * ADR-0013 D2: re-hydrate the thread (API mode only); undefined in local mode. + * + * Deliberately loose, and — since objectui#8342 — honest about it. The + * parameter stays `unknown[]` because this package will not republish + * `@ai-sdk/react`'s pinned `UIMessage` on its own surface; that is the same + * call objectui#8214 made one file over for `AnyPart.state`. Parameters are + * CONTRAVARIANT, so this declaration used to be a lie: the value handed out + * was the SDK's own `setMessages`, which accepts only its `UIMessage[]`, and + * the only thing stopping `tsc` from saying so was a `chatResult as any` + * inside the hook. + * + * What the implementation now guarantees, which is what makes the + * declaration a promise it keeps: the hook wraps the SDK function and CHECKS + * every element before handing the array on. A value that is not a chat + * message — not an object, or missing a string `id`, a + * `'user' | 'assistant' | 'system'` role, or a `parts` array — is REFUSED + * LOUDLY: the call throws a `TypeError` naming the offending index, and the + * SDK's store is left untouched, because the whole array is checked before + * anything is written. + * + * Refusing rather than FILTERING is the contract, on purpose. This is a + * re-hydration path: the caller's statement is "the thread is now exactly + * these messages". Dropping the elements that failed would install a SHORTER + * thread with no way for the caller to notice — the return type is `void` — + * which is the same silent-deletion failure objectui#4424 was graded on. The + * one in-repo consumer, `@object-ui/app-shell`'s `useReconcileOnError`, + * already calls this inside a `try`/`catch` that falls through to the + * ordinary error banner, so a refusal degrades to "show the error" instead of + * to a quietly-truncated transcript. + * + * Note the surface accepts an ARRAY only. The SDK's own `setMessages` also + * takes an updater callback; this member never advertised one and still + * does not. + */ setMessages?: (messages: unknown[]) => void; /** Whether the hook is operating in API (streaming) mode */ isApiMode: boolean; @@ -397,6 +431,70 @@ function normalizeMessages(msgs?: OuiChatMessage[]): ObjectChatMessage[] { })); } +/** + * The message element the PINNED `@ai-sdk/react` `setMessages` accepts, + * DERIVED from that function rather than restated. No SDK type is named here, + * so a version bump that moves `UIMessage` moves this alias with it and the + * published `UseObjectChatReturn['setMessages']` never has to move at all — + * which is precisely why objectui#8342 was ruled option B and not option A. + */ +type SdkChatMessage = Extract< + Parameters['setMessages']>[0], + readonly unknown[] +>[number]; + +/** + * Is `value` a chat message the SDK's store can hold? + * + * Checks exactly the three members `UIMessage` REQUIRES and every SDK read + * path dereferences: a string `id`, one of the three roles, and a `parts` + * array. `parts` is checked for array-ness only, NOT element by element — the + * part union is open (a custom `data-...` part carries an author-defined + * payload, `UIDataTypes = Record`), so there is no closed set + * to check against and restating the union would be exactly the SDK-coupling + * objectui#8342's ruling declined. `mapMessages.ts`'s `AnyPart` is the same + * decision on the inbound side; this is its outbound mirror. + */ +function isSdkChatMessage(value: unknown): value is SdkChatMessage { + if (typeof value !== 'object' || value === null) return false; + const msg = value as { id?: unknown; role?: unknown; parts?: unknown }; + return ( + typeof msg.id === 'string' && + (msg.role === 'user' || msg.role === 'assistant' || msg.role === 'system') && + Array.isArray(msg.parts) + ); +} + +/** + * Narrow an arbitrary `unknown[]` to what the SDK's `setMessages` takes, or + * refuse loudly — see {@link UseObjectChatReturn.setMessages} for why refusing + * beats filtering on this path. The array is fully checked BEFORE the caller's + * value can reach the store, so a refusal leaves the thread exactly as it was. + * + * The result is built by pushing values the type predicate has already + * narrowed; there is deliberately no assertion here, because an `as` would + * just move objectui#8342's defect one line over. + */ +function narrowToSdkChatMessages(messages: unknown[]): SdkChatMessage[] { + if (!Array.isArray(messages)) { + throw new TypeError( + `useObjectChat: setMessages expects an array of chat messages, received ${typeof messages}.`, + ); + } + const narrowed: SdkChatMessage[] = []; + messages.forEach((message, index) => { + if (!isSdkChatMessage(message)) { + throw new TypeError( + `useObjectChat: setMessages received a value at index ${index} that is not a chat ` + + `message (it needs a string \`id\`, a 'user' | 'assistant' | 'system' \`role\` and ` + + `a \`parts\` array). Nothing was written; the thread is unchanged.`, + ); + } + narrowed.push(message); + }); + return narrowed; +} + /** * useObjectChat – Composable hook for ObjectUI Chatbot. * @@ -619,6 +717,11 @@ export function useObjectChat(options: UseObjectChatOptions = {}): UseObjectChat cur.length > 0 && cur[cur.length - 1]?.role === 'user' ) { + // The SDK's own `setMessages`, reached through `chatRef` — NOT + // the narrowed wrapper the hook returns (objectui#8342). The + // value is the SDK's own live `messages` minus its last element, + // so it is already `UIMessage[]`; re-checking it here would only + // pay for a guarantee the source already carries. chat.setMessages(cur.slice(0, -1)); } // A rejected send (esp. a 429 quota block) means the usage picture @@ -680,8 +783,8 @@ export function useObjectChat(options: UseObjectChatOptions = {}): UseObjectChat sendMessage: aiSendMessage, regenerate, stop, - setMessages, - } = chatResult as any; + setMessages: aiSetMessages, + } = chatResult; const isLoading = status === 'submitted' || status === 'streaming'; @@ -720,6 +823,19 @@ export function useObjectChat(options: UseObjectChatOptions = {}): UseObjectChat [aiSendMessage, onSend, apiMessages], ); + // objectui#8342 — the honest half of the deliberately-loose surface. The + // published member takes `unknown[]`; the SDK's own `setMessages` takes only + // its `UIMessage[]`, and parameters are contravariant, so passing the SDK + // function straight out advertised a capability it does not have. The + // narrowing happens HERE, which is what turns the declaration into a promise + // the implementation keeps. + const setMessages = useCallback( + (messages: unknown[]) => { + aiSetMessages(narrowToSdkChatMessages(messages)); + }, + [aiSetMessages], + ); + const clear = useCallback(() => { setMessages([]); }, [setMessages]); @@ -797,9 +913,11 @@ export function useObjectChat(options: UseObjectChatOptions = {}): UseObjectChat stop, reload: regenerate, clear, - // ADR-0013 D2: expose the underlying useChat setMessages so the host can - // re-hydrate the thread from the server after a stream-transport failure - // (the reply may already be persisted server-side — reconcile, don't re-run). + // ADR-0013 D2: let the host re-hydrate the thread from the server after + // a stream-transport failure (the reply may already be persisted + // server-side — reconcile, don't re-run). This is the NARROWING wrapper + // above, not the SDK function itself: the declared parameter is + // `unknown[]` and the wrapper is what makes that true (objectui#8342). setMessages, isApiMode: true, input: apiInput,