mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-15 09:21:36 +00:00
fix(approval): carry allow_permanent to TUI + desktop approval prompts
When a tirith content-security warning is present the approval backend forces allow_permanent=False and silently downgrades an "always" choice to session scope (the persistence loop in check_all_command_guards only honors "always" → permanent when no tirith finding exists). But the gateway notify payload that drives the TUI and the Electron desktop app never carried that flag, so both surfaces always rendered "Always allow" — offering a permanent allow the backend would quietly refuse to persist. Plumb allow_permanent end-to-end: - tools/approval.py: include `allow_permanent: not has_tirith` in the gateway approval_data the notify callback emits as `approval.request`. - ui-tui: thread `allowPermanent` through the event handler, gateway types, and ApprovalReq; ApprovalPrompt drops the "always" option (and renumbers the quick-pick keys) when it's false. - apps/desktop: thread `allow_permanent` through the gateway payload type, the per-session approval store, and the inline ApprovalBar, which now hides the "Always allow…" dropdown item when permanent allow is disallowed — reusing the existing DropdownMenu / confirm-Dialog UI. The desktop/TUI render path for approvals already landed in #38578 (the root cause of approvals not surfacing in the GUI); this completes the salvage of #37856 by carrying allow_permanent across both surfaces. #37856's original thread-local _block() approach is dropped: desktop/TUI approvals resolve via approval.respond → resolve_gateway_approval (the per-session queue), not the _block()/request_id correlation, so a worker-thread callback waiting on _block would never be released by the real UI. Tests: gateway notify payload carries allow_permanent (True without tirith, False with a tirith warning); ui-tui approvalAction reduced option set + event-handler allowPermanent propagation; desktop store round-trip + the ApprovalBar showing/hiding "Always allow". Supersedes #37856 Closes #37812 Co-authored-by: LeonSGP43 <cine.dreamer.one@gmail.com>
This commit is contained in:
parent
d221e369b8
commit
81436e143e
14 changed files with 214 additions and 28 deletions
|
|
@ -933,6 +933,9 @@ export function useMessageStream({
|
|||
// raise it and wait — the sidebar flags "needs input" and the inline bar
|
||||
// surfaces once the user focuses that chat.
|
||||
setApprovalRequest({
|
||||
// Only an explicit false (tirith content-security warning) drops the
|
||||
// permanent-allow option; the 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
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -61,6 +61,10 @@ 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
|
||||
// The backend drops the permanent-allow path when a tirith content-security
|
||||
// warning is present (it would silently degrade "always" → session scope), so
|
||||
// don't offer the option. Only an explicit false hides it.
|
||||
const allowPermanent = request.allowPermanent !== false
|
||||
|
||||
const respond = useCallback(
|
||||
async (choice: ApprovalChoice) => {
|
||||
|
|
@ -144,16 +148,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>
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -68,6 +68,10 @@ 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 {
|
||||
// Only an explicit false hides "Always allow" — set when the backend will not
|
||||
// honor a permanent allow (a tirith content-security warning is present), so
|
||||
// the bar doesn't offer a choice that silently degrades to session scope.
|
||||
allowPermanent?: boolean
|
||||
command: string
|
||||
description: string
|
||||
}
|
||||
|
|
|
|||
|
|
@ -285,3 +285,65 @@ class TestProgrammingErrorsPropagateFromWrapper:
|
|||
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 = set_current_session_key(session_key)
|
||||
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
|
||||
|
|
|
|||
|
|
@ -1428,6 +1428,13 @@ 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 (see prompt_dangerous_approval
|
||||
# below). When a tirith content-security warning is present the
|
||||
# backend silently downgrades an "always" choice to session scope
|
||||
# (see the persistence loop after the decision), so the UI must not
|
||||
# offer a permanent allow it can't honor. Without this field the
|
||||
# TUI/desktop always render "Always allow".
|
||||
"allow_permanent": not has_tirith,
|
||||
}
|
||||
decision = _await_gateway_decision(
|
||||
session_key, notify_cb, approval_data, surface="gateway"
|
||||
|
|
|
|||
|
|
@ -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' })
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -728,8 +728,13 @@ export function createGatewayEventHandler(ctx: GatewayEventHandlerContext): (ev:
|
|||
return
|
||||
case 'approval.request': {
|
||||
const description = String(ev.payload.description ?? 'dangerous command')
|
||||
// Backend omits the field for the common case; only an explicit false
|
||||
// (tirith warning present) 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
|
||||
|
|
|
|||
|
|
@ -7,10 +7,16 @@ 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
|
||||
// When the backend disallows a permanent allow (tirith content-security
|
||||
// warning present) the "always" option is dropped — picking it would only
|
||||
// be silently downgraded to session scope, so don't offer it.
|
||||
const APPROVAL_OPTS_NO_ALWAYS = ['once', 'session', 'deny'] as const
|
||||
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 +24,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
|
||||
|
|
@ -34,26 +37,31 @@ type ApprovalAction =
|
|||
* 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 +70,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 +108,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 +117,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>
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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' }
|
||||
|
|
|
|||
|
|
@ -90,6 +90,9 @@ export interface DelegationStatus {
|
|||
}
|
||||
|
||||
export interface ApprovalReq {
|
||||
// When false the backend will not honor a permanent allow (e.g. a tirith
|
||||
// content-security warning is present), so the prompt hides "Always allow".
|
||||
allowPermanent?: boolean
|
||||
command: string
|
||||
description: string
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue