Merge pull request #73035 from NousResearch/bb/clarify-composer-enter

fix(desktop): let a typed message answer past a clarify prompt

apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx

@@ -1,7 +1,9 @@
 import { act, cleanup, renderHook, waitFor } from '@testing-library/react'
 import { afterEach, describe, expect, it, vi } from 'vitest'
 
+import { $clarifyRequests } from '@/store/clarify'
 import type { ComposerAttachment } from '@/store/composer'
+import { $gateway } from '@/store/gateway'
 
 import { useComposerSubmit } from './use-composer-submit'
 
@@ -184,3 +186,80 @@ describe('useComposerSubmit busy-turn routing', () => {
     )
   })
 })
+
+describe('useComposerSubmit with a clarify parked on the session', () => {
+  const gatewayRequest = vi.fn(async () => ({ ok: true }))
+
+  const parkClarify = (sessionId: string) => {
+    $clarifyRequests.set({
+      [sessionId]: { requestId: `req-${sessionId}`, question: 'which one?', choices: ['a', 'b'], sessionId }
+    })
+    $gateway.set({ request: gatewayRequest } as unknown as ReturnType<typeof $gateway.get>)
+  }
+
+  afterEach(() => {
+    cleanup()
+    gatewayRequest.mockClear()
+    $clarifyRequests.set({})
+    $gateway.set(null)
+    vi.restoreAllMocks()
+  })
+
+  it('skips the question and still sends the typed message on an idle session', async () => {
+    parkClarify('runtime-session')
+    const { hook, onSubmit } = renderSubmitHook({ text: 'actually do this instead' })
+
+    act(() => {
+      hook.result.current.submitDraft()
+    })
+
+    await waitFor(() =>
+      expect(gatewayRequest).toHaveBeenCalledWith('clarify.respond', {
+        request_id: 'req-runtime-session',
+        answer: ''
+      })
+    )
+    await waitFor(() =>
+      expect(onSubmit).toHaveBeenCalledWith('actually do this instead', expect.objectContaining({ attachments: [] }))
+    )
+    expect($clarifyRequests.get()['runtime-session']).toBeUndefined()
+  })
+
+  it('skips the question before steering a busy turn', async () => {
+    parkClarify('runtime-session')
+    const { hook, onSteer } = renderSubmitHook({ busy: true, text: 'change course' })
+
+    act(() => {
+      hook.result.current.submitDraft()
+    })
+
+    await waitFor(() => expect(onSteer).toHaveBeenCalledWith('change course'))
+    expect(gatewayRequest).toHaveBeenCalledWith('clarify.respond', { request_id: 'req-runtime-session', answer: '' })
+  })
+
+  it('leaves the question alone for an empty Enter (Stop, not an answer)', () => {
+    parkClarify('runtime-session')
+    const { hook, onCancel } = renderSubmitHook({ busy: true })
+
+    act(() => {
+      hook.result.current.submitDraft()
+    })
+
+    expect(gatewayRequest).not.toHaveBeenCalled()
+    expect($clarifyRequests.get()['runtime-session']).toBeDefined()
+    expect(onCancel).toHaveBeenCalledTimes(1)
+  })
+
+  it("leaves another session's question alone", async () => {
+    parkClarify('other-session')
+    const { hook, onSubmit } = renderSubmitHook({ text: 'unrelated message' })
+
+    act(() => {
+      hook.result.current.submitDraft()
+    })
+
+    await waitFor(() => expect(onSubmit).toHaveBeenCalled())
+    expect(gatewayRequest).not.toHaveBeenCalled()
+    expect($clarifyRequests.get()['other-session']).toBeDefined()
+  })
+})

apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts

@@ -2,6 +2,7 @@ import { type RefObject, useEffect, useRef } from 'react'
 
 import { SLASH_COMMAND_RE } from '@/lib/chat-runtime'
 import { triggerHaptic } from '@/lib/haptics'
+import { hasClarifyRequest, skipClarifyRequest } from '@/store/clarify'
 import { clearSessionDraft, type ComposerAttachment } from '@/store/composer'
 import { resetBrowseState } from '@/store/composer-input-history'
 import { enqueueQueuedPrompt, type QueuedPromptEntry } from '@/store/composer-queue'
@@ -141,6 +142,21 @@ export function useComposerSubmit({
     const text = draftRef.current
     const payloadPresent = text.trim().length > 0 || attachments.length > 0
 
+    // A clarify card parked on this session owns the turn: the agent is blocked
+    // inside its tool batch waiting on `clarify.respond`, so a follow-up routed
+    // through steer/queue sits undelivered until the clarify's own timeout
+    // (default 5 min) — the message looks sent and nothing happens. Typing a
+    // real message instead of picking an option IS the answer "none of these":
+    // skip the question so the tool returns, then route the words normally.
+    //
+    // Fire-and-forget, not awaited: the skip clears the card synchronously and
+    // both RPCs ride the same socket in call order, so the gateway resolves the
+    // clarify before it sees the follow-up. Awaiting first would leave the draft
+    // live for a tick — long enough for a second Enter to send it twice.
+    if (payloadPresent && !queueEdit && hasClarifyRequest(sessionId)) {
+      void skipClarifyRequest(sessionId)
+    }
+
     if (queueEdit) {
       exitQueuedEdit('save')
     } else if (busy) {

apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx

@@ -298,13 +298,16 @@ describe('ClarifyTool keyboard navigation', () => {
 })
 
 describe('ClarifyTool pending marker', () => {
-  it('marks a live choices card so type-to-focus yields its shortcut keys', () => {
+  it('marks a live choices card with its row count so type-to-focus yields exactly its keys', () => {
     renderLiveClarify()
 
-    // The marker is what `composerFocusBlockedBySurface` keys off of, so the
-    // global type-to-focus listener stands down and A/B/C… + 1-9 + Enter reach
-    // the card instead of the composer.
-    expect(document.querySelector('[data-clarify-choices]')).toBeTruthy()
+    // `clarifyCardOwnsKey` reads the count off this marker to yield only the
+    // shortcuts the card renders (A..N + "Other", 1-9, Enter) and let every
+    // other printable through to the composer.
+    const card = document.querySelector('[data-clarify-choices]')
+
+    expect(card).toBeTruthy()
+    expect(Number(card?.getAttribute('data-clarify-choices'))).toBeGreaterThan(0)
   })
 
   it('does not mark a free-text (no-choice) pending card', () => {

apps/desktop/src/components/assistant-ui/clarify-tool.tsx

@@ -489,6 +489,10 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) {
 
       const key = event.key.toLowerCase()
 
+      // Only the letters this card actually renders a row for. Anything past
+      // the last row belongs to the composer — the user is typing a message
+      // instead of picking an option, and swallowing the keystroke here would
+      // make the first letter of it vanish.
       if (key.length === 1 && key >= 'a' && key <= 'z') {
         const index = key.charCodeAt(0) - 97
 
@@ -538,11 +542,13 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) {
   }
 
   return (
-    // `data-clarify-choices` marks the panel as owning printable/Enter keys
-    // while its A/B/C… shortcuts are live, so the global type-to-focus listener
-    // (`composerFocusBlockedBySurface`) stands down and the letters reach this
-    // card instead of being redirected into the composer.
-    <ClarifyShell className="grid gap-2 px-2.5 py-2" data-clarify-choices={hasChoices ? '' : undefined}>
+    // `data-clarify-choices` marks the panel as owning its OWN shortcut keys
+    // (Enter, and 1..N+1 / A.. for the N choices plus "Other") while they're
+    // live, so the global type-to-focus listener (`clarifyCardOwnsKey`) yields
+    // exactly those and lets every other printable through to the composer —
+    // typing a real message instead of picking an option stays possible. The
+    // value is the choice count so the check needs no store access.
+    <ClarifyShell className="grid gap-2 px-2.5 py-2" data-clarify-choices={hasChoices ? choices.length : undefined}>
       <div className="flex items-start gap-2">
         <span className="flex-1 whitespace-pre-wrap font-medium leading-(--conversation-line-height)">{question}</span>
         <MessageQuestion aria-hidden className="mt-px size-4 shrink-0 text-(--ui-text-tertiary)" />

apps/desktop/src/lib/keybinds/composer-focus-keys.test.ts

@@ -80,22 +80,11 @@ describe('composerFocusBlockedBySurface', () => {
     expect(composerFocusBlockedBySurface()).toBe(true)
   })
 
-  it('blocks while a live clarify choices card owns its letter keys', () => {
+  it('ignores a clarify card — it yields only its own keys, per-key', () => {
     const card = document.createElement('div')
-    card.setAttribute('data-clarify-choices', '')
+    card.setAttribute('data-clarify-choices', '2')
     document.body.append(card)
 
-    expect(composerFocusBlockedBySurface()).toBe(true)
-  })
-
-  it('ignores a clarify card waiting in a kept-alive background tab', () => {
-    const tab = document.createElement('div')
-    tab.setAttribute('data-pane-hidden', '')
-    const card = document.createElement('div')
-    card.setAttribute('data-clarify-choices', '')
-    tab.append(card)
-    document.body.append(tab)
-
     expect(composerFocusBlockedBySurface()).toBe(false)
   })
 
@@ -174,15 +163,41 @@ describe('composerFocusKeysAllowed', () => {
     expect(composerFocusKeysAllowed(keydown({ key: 'a', code: 'KeyA', target: document.body }), 'type')).toBe(false)
   })
 
-  it('yields letter + Enter keys to a live clarify choices card', () => {
+  it('yields only the keys a live clarify card actually binds', () => {
     const card = document.createElement('div')
-    card.setAttribute('data-clarify-choices', '')
+    // Two choices → rows A/B plus the "Other" row C; 1/2/3 are the digit twins.
+    card.setAttribute('data-clarify-choices', '2')
     document.body.append(card)
 
-    // The clarify card's own A/B/C… + Enter shortcuts must win over type-to-focus.
-    expect(composerFocusKeysAllowed(keydown({ key: 'a', code: 'KeyA', target: document.body }), 'type')).toBe(false)
-    expect(composerFocusKeysAllowed(keydown({ key: 'Enter', code: 'Enter', target: document.body }), 'enter')).toBe(
-      false
-    )
+    const allowed = (key: string, combo: string) =>
+      composerFocusKeysAllowed(keydown({ key, target: document.body }), combo)
+
+    // The card's own shortcuts win over type-to-focus.
+    expect(allowed('a', 'type')).toBe(false)
+    expect(allowed('B', 'type')).toBe(false)
+    expect(allowed('c', 'type')).toBe(false)
+    expect(allowed('1', 'type')).toBe(false)
+    expect(allowed('3', 'type')).toBe(false)
+    expect(allowed('Enter', 'enter')).toBe(false)
+
+    // Everything past the last row is not the card's — typing a real message
+    // instead of picking an option must still reach the composer.
+    expect(allowed('d', 'type')).toBe(true)
+    expect(allowed('z', 'type')).toBe(true)
+    expect(allowed('4', 'type')).toBe(true)
+    expect(allowed('?', 'type')).toBe(true)
+    expect(allowed(' ', 'type')).toBe(true)
+  })
+
+  it('leaves every key alone for a clarify card in a background tab', () => {
+    const tab = document.createElement('div')
+    tab.setAttribute('data-pane-hidden', '')
+    const card = document.createElement('div')
+    card.setAttribute('data-clarify-choices', '2')
+    tab.append(card)
+    document.body.append(tab)
+
+    expect(composerFocusKeysAllowed(keydown({ key: 'a', target: document.body }), 'type')).toBe(true)
+    expect(composerFocusKeysAllowed(keydown({ key: 'Enter', target: document.body }), 'enter')).toBe(true)
   })
 })

apps/desktop/src/lib/keybinds/composer-focus-keys.ts

@@ -56,19 +56,61 @@ export function isActivateOnEnterTarget(target: EventTarget | null): boolean {
 }
 
 /**
- * Dialogs, menus, terminal, full pages, session switcher, any open overlay
- * (settings / command-center / star map / …), and a live clarify choices card —
+ * True when a live clarify card binds THIS key, so type-to-focus must yield it.
+ *
+ * The card owns Enter plus the shortcuts it actually renders — `1..N+1` and
+ * `A..` for its N choices and the trailing "Other" row. It does NOT own the
+ * rest of the alphabet: typing a real message instead of picking an option is a
+ * legitimate answer ("none of these"), and blanket-blocking every printable
+ * left the user unable to start that message at all — the first letter vanished
+ * and the composer never focused. Out-of-range keys fall through to the
+ * composer, which skips the question on send.
+ *
+ * The choice count rides in the attribute's value, so this stays a DOM read
+ * with no store coupling.
+ */
+export function clarifyCardOwnsKey(event: KeyboardEvent): boolean {
+  const card = queryVisible(BLOCKING_IN_SURFACE)
+
+  if (!card) {
+    return false
+  }
+
+  if (event.key === 'Enter') {
+    return true
+  }
+
+  // "Other" is the row past the last choice, hence the +1.
+  const rows = Number(card.getAttribute('data-clarify-choices')) + 1
+
+  if (!Number.isFinite(rows)) {
+    return false
+  }
+
+  const key = event.key.toLowerCase()
+
+  if (key.length !== 1) {
+    return false
+  }
+
+  const index = /^[1-9]$/.test(key) ? Number(key) - 1 : key >= 'a' && key <= 'z' ? key.charCodeAt(0) - 97 : -1
+
+  return index >= 0 && index < rows
+}
+
+/**
+ * Dialogs, menus, terminal, full pages, session switcher, and any open overlay —
  * they keep their keys, so type-to-focus / soft `/` / Enter stand down rather
  * than stealing keystrokes those surfaces own (or leaking them into the composer
- * mounted behind an overlay).
+ * mounted behind an overlay). A live clarify card is handled per-key by
+ * `clarifyCardOwnsKey`, not here — it only owns its own shortcuts.
  */
 export function composerFocusBlockedBySurface(): boolean {
   return (
     switcherActive() ||
     $workspaceIsPage.get() ||
     isFocusWithin('[data-terminal]') ||
-    Boolean(document.querySelector(BLOCKING_OVERLAY)) ||
-    Boolean(queryVisible(BLOCKING_IN_SURFACE))
+    Boolean(document.querySelector(BLOCKING_OVERLAY))
   )
 }
 
@@ -95,7 +137,8 @@ export function composerFocusKeysAllowed(event: KeyboardEvent, combo: string): b
     event.defaultPrevented ||
     event.isComposing ||
     isEditableTarget(event.target) ||
-    composerFocusBlockedBySurface()
+    composerFocusBlockedBySurface() ||
+    clarifyCardOwnsKey(event)
   ) {
     return false
   }

apps/desktop/src/store/clarify.test.ts

@@ -1,13 +1,16 @@
-import { afterEach, beforeEach, describe, expect, it } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
 import {
   $clarifyRequest,
   $clarifyRequests,
   type ClarifyRequest,
   clearClarifyRequest,
+  hasClarifyRequest,
   normalizeChoices,
-  setClarifyRequest
+  setClarifyRequest,
+  skipClarifyRequest
 } from './clarify'
+import { $gateway } from './gateway'
 import { $activeSessionId } from './session'
 
 function clarify(sessionId: string | null, requestId: string): ClarifyRequest {
@@ -81,6 +84,47 @@ describe('clarify store', () => {
   })
 })
 
+describe('skipClarifyRequest', () => {
+  const request = vi.fn(async () => ({ ok: true }))
+
+  beforeEach(() => {
+    $clarifyRequests.set({})
+    request.mockClear()
+    $gateway.set({ request } as unknown as ReturnType<typeof $gateway.get>)
+  })
+
+  afterEach(() => {
+    $clarifyRequests.set({})
+    $gateway.set(null)
+  })
+
+  it('answers the session\u2019s clarify with an empty answer and drops it', async () => {
+    setClarifyRequest(clarify('session-a', 'req-a'))
+    setClarifyRequest(clarify('session-b', 'req-b'))
+
+    await expect(skipClarifyRequest('session-a')).resolves.toBe(true)
+
+    expect(request).toHaveBeenCalledWith('clarify.respond', { request_id: 'req-a', answer: '' })
+    expect(hasClarifyRequest('session-a')).toBe(false)
+    // A background session's question is untouched — only the one being typed
+    // over is skipped.
+    expect(hasClarifyRequest('session-b')).toBe(true)
+  })
+
+  it('is a no-op when the session has no clarify parked', async () => {
+    await expect(skipClarifyRequest('session-a')).resolves.toBe(false)
+    expect(request).not.toHaveBeenCalled()
+  })
+
+  it('still reports the skip when the respond RPC fails', async () => {
+    setClarifyRequest(clarify('session-a', 'req-a'))
+    request.mockRejectedValueOnce(new Error('socket closed'))
+
+    await expect(skipClarifyRequest('session-a')).resolves.toBe(true)
+    expect(hasClarifyRequest('session-a')).toBe(false)
+  })
+})
+
 describe('normalizeChoices', () => {
   it('returns empty array for null/undefined', () => {
     expect(normalizeChoices(null)).toEqual([])

apps/desktop/src/store/clarify.ts

@@ -1,5 +1,6 @@
 import { atom, computed } from 'nanostores'
 
+import { $gateway } from './gateway'
 import { $activeSessionId } from './session'
 
 export interface ClarifyRequest {
@@ -102,3 +103,42 @@ export function clearClarifyRequest(requestId?: string, sessionId?: string | nul
     $clarifyRequests.set(next)
   }
 }
+
+/** Whether `sessionId` has a clarify parked on it right now (imperative read —
+ *  the composer checks this on Enter, not on every render). */
+export const hasClarifyRequest = (sessionId: string | null | undefined): boolean =>
+  Boolean($clarifyRequests.get()[keyFor(sessionId)])
+
+/**
+ * Answer `sessionId`'s pending clarify with an empty answer (a skip) and drop it
+ * locally, resolving to whether there was one to skip.
+ *
+ * The composer uses this when the user types a real message instead of picking
+ * an option: a clarify blocks the agent inside its tool batch, so leaving it
+ * unanswered would park the follow-up until the server-side clarify timeout
+ * (default 5 min) — the message looks sent and nothing happens. Skipping lets
+ * the tool return and the turn carry on with the user's actual words.
+ *
+ * An empty answer is the same thing the card's own Skip button sends, and
+ * `clarify.respond` is `allow_expired`, so racing the timeout is harmless.
+ */
+export async function skipClarifyRequest(sessionId: string | null | undefined): Promise<boolean> {
+  const request = $clarifyRequests.get()[keyFor(sessionId)]
+
+  if (!request) {
+    return false
+  }
+
+  // Clear first: the answer is already decided, and an in-flight RPC must not
+  // leave a live card the user can answer a second time.
+  clearClarifyRequest(request.requestId, request.sessionId)
+
+  try {
+    await $gateway.get()?.request('clarify.respond', { request_id: request.requestId, answer: '' })
+  } catch {
+    // The tool times out on its own; a failed skip must never swallow the
+    // message the user is actually sending.
+  }
+
+  return true
+}