From 6b2c23ae42bcc837471a87c844499bb04741842e Mon Sep 17 00:00:00 2001
From: teknium1 <127238744+teknium1@users.noreply.github.com>
Date: Wed, 23 Sep 2026 19:46:57 -0700
Subject: [PATCH] fix(desktop): model-pill provider must return a string;
 non-strings fall through
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review finding (MAJOR) on #120919: useComposerModelPillLabel accepted any
truthy return with `if (label)`. ModelPill drops the value straight into JSX
with no error boundary, so a plugin returning an object/array/number threw
"Objects are not valid as a React child" and blanked the whole composer —
exactly the failure mode the hook's throw-swallowing was meant to prevent.
Only a non-empty, non-whitespace string is now a label; anything else
declines like null.

Review finding (minor): the two existing tests are tightened instead of
adding new ones. The compact-mode "providers skipped" assertion was
`queryByText(...)` on a chevron-only render and passed regardless; it is now
a spy that must not be called. The throw test now also covers the
non-string return (red on b559ac9c: React child error) and two non-null
providers: the first registered string wins and the later provider's spy is
never invoked.

Docs: arbitration section states the string-only contract and that
`reasoningEffort` is always a string ('' when none).
---
 apps/desktop/src/app/chat/composer/contrib.ts |  5 +++-
 .../src/app/chat/composer/model-pill.test.tsx | 27 ++++++++++++-------
 .../developer-guide/desktop-plugin-sdk.md     | 13 +++++----
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/apps/desktop/src/app/chat/composer/contrib.ts b/apps/desktop/src/app/chat/composer/contrib.ts
index b3cfc80124..05b4ef0b14 100644
--- a/apps/desktop/src/app/chat/composer/contrib.ts
+++ b/apps/desktop/src/app/chat/composer/contrib.ts
@@ -201,7 +201,10 @@ export function useComposerModelPillLabel({ compact, model, reasoningEffort }: C
       try {
         const label = provider?.label?.(ctx)
 
-        if (label) {
+        // Only a non-empty string is a label. ModelPill renders the value
+        // straight into JSX with no error boundary, so an object/array/number
+        // from a plugin would throw and blank the composer — treat it as declining.
+        if (typeof label === 'string' && label.trim() !== '') {
           return label
         }
       } catch {
diff --git a/apps/desktop/src/app/chat/composer/model-pill.test.tsx b/apps/desktop/src/app/chat/composer/model-pill.test.tsx
index 819fcdf59f..1e6270d8ab 100644
--- a/apps/desktop/src/app/chat/composer/model-pill.test.tsx
+++ b/apps/desktop/src/app/chat/composer/model-pill.test.tsx
@@ -1,7 +1,7 @@
 import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'
 import { atom } from 'nanostores'
 import { useContext } from 'react'
-import { afterEach, describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it, vi } from 'vitest'
 
 import type { ChatBarState } from '@/app/chat/composer/types'
 import { type SessionView, SessionViewProvider } from '@/app/chat/session-view'
@@ -10,7 +10,7 @@ import { registry } from '@/contrib/registry'
 import { formatModelPillLabel } from '@/lib/model-status-label'
 import { $activeSessionId, $currentModel, setCurrentModel, setCurrentModelSource } from '@/store/session'
 
-import { COMPOSER_AREAS, type ComposerModelPillProvider } from './contrib'
+import { COMPOSER_AREAS, type ComposerModelPillContext, type ComposerModelPillProvider } from './contrib'
 import { requestModelMenuToggle } from './focus'
 import { ModelPill } from './model-pill'
 import { RICH_INPUT_SLOT } from './rich-editor'
@@ -191,22 +191,26 @@ describe('ModelPill label providers', () => {
   it('renders a provider-supplied label, and the core label once the provider declines', () => {
     setCurrentModel('deepseek/deepseek-v4-flash')
 
-    register(({ model, reasoningEffort }) => `${model} · ${reasoningEffort || 'none'}`)
+    const label = vi.fn(({ model, reasoningEffort }: ComposerModelPillContext) => `${model} · ${reasoningEffort || 'none'}`)
+    register(label)
 
     const { unmount } = render(
       <ModelPill disabled={false} model={modelState({ model: 'deepseek/deepseek-v4-flash' })} />
     )
 
     expect(screen.getByText('deepseek/deepseek-v4-flash · none')).toBeTruthy()
+    expect(label).toHaveBeenCalled()
     unmount()
 
     // Floating-composer (compact) mode renders only the chevron: providers are
-    // not consulted, so the provider text must not leak into the DOM.
+    // not consulted at all (the chevron has no text to leak, so only the spy
+    // proves the skip).
+    label.mockClear()
     const compactRender = render(
       <ModelPill compact disabled={false} model={modelState({ model: 'deepseek/deepseek-v4-flash' })} />
     )
 
-    expect(screen.queryByText(/· none/)).toBeNull()
+    expect(label).not.toHaveBeenCalled()
     compactRender.unmount()
 
     // Declining provider: the override text is gone, the core label is back.
@@ -219,17 +223,22 @@ describe('ModelPill label providers', () => {
     expect(screen.getByText(formatModelPillLabel('deepseek/deepseek-v4-flash', { fastMode: false }))).toBeTruthy()
   })
 
-  it('treats a throwing provider as declining and lets the next provider win', () => {
+  it('falls through on throw and on a non-string return; the first string wins and later providers are not asked', () => {
     setCurrentModel('deepseek/deepseek-v4-flash')
 
     register(() => {
       throw new Error('broken provider')
     }, 'broken')
-    register(() => 'next wins', 'working')
+    // A non-string (object/array/number) is not a label: rendering it would
+    // throw inside ModelPill (no error boundary there) and blank the composer.
+    register(() => ({ text: 'object label' }) as unknown as string, 'wrong-type')
+    register(() => 'first wins', 'first')
+    const second = vi.fn(() => 'second loses')
+    register(second, 'second')
 
     render(<ModelPill disabled={false} model={modelState({ model: 'deepseek/deepseek-v4-flash' })} />)
 
-    // The broken provider declined; the next one's label renders.
-    expect(screen.getByText('next wins')).toBeTruthy()
+    expect(screen.getByText('first wins')).toBeTruthy()
+    expect(second).not.toHaveBeenCalled()
   })
 })
diff --git a/website/docs/developer-guide/desktop-plugin-sdk.md b/website/docs/developer-guide/desktop-plugin-sdk.md
index e474e45e41..0eff36d102 100644
--- a/website/docs/developer-guide/desktop-plugin-sdk.md
+++ b/website/docs/developer-guide/desktop-plugin-sdk.md
@@ -596,11 +596,14 @@ ctx.register({
 ```
 
 **Arbitration.** Providers are consulted in registry order and the *first
-non-null, non-empty string wins*. A provider that returns `null` (or `''`)
-declines and the next one is asked; a provider that **throws** is treated as
-declining — the error is swallowed and the pill falls through to the next
-provider, then to the core label, so a broken plugin can never blank the pill.
-In compact (floating) mode the pill renders only the chevron and no provider is
+non-empty string wins*; later providers are not called. Anything else declines
+and the next provider is asked: `null`, `''`, a whitespace-only string, and any
+non-string value (an object, array or number is never rendered — the label is
+placed straight into JSX). A provider that **throws** also declines — the error
+is swallowed and the pill falls through to the next provider, then to the core
+label, so a broken plugin can never blank the pill. `reasoningEffort` is always
+a `string` (`''` when the model has no effort level, never `undefined`). In
+compact (floating) mode the pill renders only the chevron and no provider is
 called. `label()` is re-evaluated only when the registry, the model, the effort
 level or the compact flag changes.