diff --git a/CHANGELOG.md b/CHANGELOG.md index 27d9102e..c71b992f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -444,6 +444,9 @@ and versions follow [Semantic Versioning](https://semver.org/). - A task that repeats the same failing step three times now pauses as stuck and asks what to do, instead of failing. ### Fixed +- **Approval cards name browser clicks the way the engine sees them.** A browser tool's label reads the clicked + element, so clicking an ordinary button reads as a click and "Send feedback" reads as sending, while clicking + "Place order" still reads as a purchase. - **A paused task stays paused when Sentient restarts.** Before, a task you paused while it was working carried on by itself the next time Sentient started, and one still being planned was planned. Now it waits: the task shows "Paused before it finished. Resume the task to continue.", and nothing runs for it. An approval it was waiting on diff --git a/desktop/src/features/chat/ApprovalCard.tsx b/desktop/src/features/chat/ApprovalCard.tsx index 8ab6e1f8..893eb92c 100644 --- a/desktop/src/features/chat/ApprovalCard.tsx +++ b/desktop/src/features/chat/ApprovalCard.tsx @@ -1,11 +1,14 @@ import { IconAlertTriangle, + IconCamera, IconCheck, + IconClick, IconClockX, IconCreditCard, IconEdit, IconEye, IconFolder, + IconKeyboard, IconMail, IconMailOpened, IconSend, @@ -27,7 +30,7 @@ import { cn, humanize, truncate } from '@/lib/utils' import { ActionPreview } from './ActionPreview' import { hostOf, refLabel } from './cards/bits' import { toolMeta } from './toolMeta' -import { approvalRisk, type RiskLabel } from './approvalRisk' +import { resolveWording, type RiskLabel } from './approvalRisk' const STATUS_TEXT: Partial> = { allow: 'You allowed this once', @@ -69,21 +72,47 @@ const RISK_ICONS: Record = { Reads: IconEye } +/** + * Icons for the engine's describe_fn action labels (#347). Keyed by label with a generic + * fallback: the engine may send labels this table has never seen. + */ +const ENGINE_ICONS: Record = { + Purchase: IconCreditCard, + Deletes: IconTrash, + 'Posts publicly': IconSend, + 'Sends an email': IconMail, + Sends: IconSend, + Submits: IconSend, + 'Runs a command': IconTerminal2, + 'Runs code': IconTerminal2, + 'Changes something': IconEdit, + 'Looks something up': IconEye, + Clicks: IconClick, + 'Types into the page': IconKeyboard, + 'Presses a key': IconKeyboard, + 'Takes a photo': IconCamera, + 'Looks at the screen': IconEye, + 'Moves to the trash': IconTrash +} + /** * The engine reports the effective risk (§10 `risk_fn`: a click on "Place order" becomes `send`). - * The wording comes from {@link approvalRisk}: the tool's name and declared risk first, word - * guessing only as a last resort, and never from the arguments' content — except the slug - * keys of multi-execute tools, which name the action (#344). A trash-kind preview (#142) - * reads as a move to the trash, not a loss, and is checked first. + * The wording comes from {@link resolveWording}: a trash-kind preview (#142) reads as a move to + * the trash, not a loss, and is checked first; a browser tool's describe_fn label is trusted + * as-is; otherwise the tool's name and declared risk come first (#346, #372), word guessing + * only as a last resort and never from the arguments' content — except the slug keys of + * multi-execute tools, which name the action (#344). When the rules return a generic label, + * the engine's risk_label breaks the tie if a describe_fn actually ran (#347). */ export function effectiveRisk( approval: Pick & Pick, 'preview' | 'riskLabel'> ): EffectiveRisk { - // a delete for good became its app's trash version (#142): it can come back, so it reads as a change, not a loss - if (approval.preview?.kind === 'trash' && approval.risk !== 'exec') - return { label: 'Moves to the trash', verb: 'move something to the trash', icon: IconTrash, tone: 'warning' } - const wording = approvalRisk(approval) - return { ...wording, icon: RISK_ICONS[wording.label] } + const wording = resolveWording(approval) + const icon = + wording.label in RISK_ICONS + ? RISK_ICONS[wording.label as RiskLabel] + : (ENGINE_ICONS[wording.label] ?? IconShieldQuestion) + return { ...wording, icon } } /** One plain line saying what exactly will happen, e.g. `Click "Place order" on amazon.in`. */ diff --git a/desktop/src/features/chat/approvalRisk.ts b/desktop/src/features/chat/approvalRisk.ts index b09d83f9..7c5b1836 100644 --- a/desktop/src/features/chat/approvalRisk.ts +++ b/desktop/src/features/chat/approvalRisk.ts @@ -4,11 +4,15 @@ * Split out of ApprovalCard.tsx so it can be unit-tested without the UI imports (#344). * It has no runtime imports: `node --test tests/approvalRisk.test.ts` runs it directly. * - * The rule (#344): the heading comes from the tool's NAME and its DECLARED RISK first - * (e.g. `send_email`, `gmail_send` means "send an email"). Word guessing is a last resort - * and only ever looks at the tool name and the reason (the tool's description, e.g. - * "mcp_mailer: Send an email to one address"): never at the arguments' content, so an - * email body mentioning "post", "publish" or "tweet" cannot read as posting publicly. + * The rule (#344, refined by #347): the heading comes from the tool's NAME and its DECLARED RISK first + * (e.g. `send_email`, `gmail_send` means "send an email"; Home Assistant's locks read as "Home security"). + * Two exceptions: a click/type/press tool's describe_fn always runs and reads the actual element, so its risk_label + * names the action better than the rules (a "Send feedback" button reads as Sends, not Purchase); and when + * the rules return a generic label, the engine's risk_label breaks the tie if a describe_fn actually ran. + * When the engine's label has no known verb, the heading falls back to what the rules would have said. + * Word guessing is a last resort and only ever looks at the tool name and the reason (the tool's description, + * e.g. "mcp_mailer: Send an email to one address"): never at the arguments' content, so an email body + * mentioning "post", "publish" or "tweet" cannot read as posting publicly. * * One deliberate exception: multi-execute tools (Composio's COMPOSIO_MULTI_EXECUTE_TOOL) * carry the real action in slug keys (`tool_slug`, `action_slug`, `slug` — the same @@ -168,3 +172,130 @@ function isEmailSendingTool(name: string): boolean { if (tokens.some((t) => /^(draft|drafts)$/.test(t))) return false return isEmailTool(name) && tokens.some((t) => /^(send|sends|sending|sent|reply|forward)$/.test(t)) } + +/** + * The engine's generic per-risk fallback labels (sentient/tools/base.py RISK_LABELS): + * the engine always fills risk_label, so receiving one of these means no tool + * describe_fn ran and the card should keep the tool-name rules below (#346). + */ +const ENGINE_FALLBACK: Record<'read' | 'write' | 'send' | 'exec', string> = { + read: 'Looks something up', + write: 'Changes something', + send: 'Sends', + exec: 'Runs code' +} + +/** + * Plain verbs for the engine's describe_fn action labels (sentient/browser/safety.py, + * terminal, nodes, home assistant): "Sentient wants to ..." + verb. + */ +const ENGINE_VERBS: Record = { + Purchase: 'make a purchase', + Deletes: 'delete something', + 'Posts publicly': 'post something publicly', + 'Sends an email': 'send an email', + Sends: 'send something on your behalf', + Submits: 'submit something for you', + 'Signs up': 'sign you up for something', + 'Runs a command': 'run a command on this computer', + 'Runs code': 'run code on your computer', + 'Changes something': 'make a change', + 'Looks something up': 'look something up', + Clicks: 'click', + 'Types into the page': 'type into the page', + 'Presses a key': 'press a key', + 'Takes a photo': 'take a photo', + 'Looks at the screen': 'look at the screen' +} + +/** + * The browser tools whose describe_fn reads the actual element (sentient/browser/tools.py): + * only these get a risk_kind, so only their risk_label is trusted as-is. The other + * browser_* tools (e.g. browser_select) keep the tool-name rules. + */ +const BROWSER_DESCRIBE_TOOLS: ReadonlySet = new Set(['browser_click', 'browser_type', 'browser_press']) + +const ENGINE_TONE: Record<'read' | 'write' | 'send' | 'exec', RiskTone> = { + read: 'neutral', + write: 'warning', + send: 'danger', + exec: 'danger' +} + +export interface EngineWording { + label: string + /** "Sentient wants to ..." */ + verb: string + tone: RiskTone +} + +/** + * Wording from the engine's own risk_label, or null to keep the tool-name rules (#346). + * + * A tool's describe_fn names the actual action (a browser click's describe_fn reads the + * clicked element: "Place order" -> "Purchase", an ordinary button -> "Clicks"), so when + * the engine's label is specific it names the card more accurately than our rules (#347). + * The engine always fills risk_label, so a label that is just the generic per-risk + * fallback means no describe_fn ran: return null and let approvalRisk name it. + */ +export function engineWording( + riskLabel: string | null | undefined, + risk: 'read' | 'write' | 'send' | 'exec', + /** + * What the tool-name rules said: used for the heading when the engine's label + * has no known verb, so the heading stays a real sentence (#347 review). + */ + fallbackVerb = 'do this' +): EngineWording | null { + const label = (riskLabel ?? '').trim() + if (!label || label === ENGINE_FALLBACK[risk]) return null + return { label, verb: ENGINE_VERBS[label] ?? fallbackVerb, tone: ENGINE_TONE[risk] } +} + +/** + * Labels approvalRisk returns when the tool-name rules could not name the action. + * Only for these does the engine's risk_label get the tiebreaker (#347). + */ +export const GENERIC_RISK_LABELS: ReadonlySet = new Set(['Sends', 'Changes things', 'Reads']) + +export interface CardWording { + label: string + /** "Sentient wants to ..." */ + verb: string + tone: RiskTone + /** One plain sentence on what could go wrong (#175). */ + caution?: string +} + +/** + * The full heading precedence for an approval card, icon aside (#347): + * + * 1. A trash-kind preview (#142) reads as a move to the trash, not a loss. + * 2. A click/type/press tool's describe_fn always runs and reads the actual element, so + * its risk_label is trusted as-is: "Send feedback" reads as Sends, not Purchase. + * Other browser_* tools have no describe_fn and keep the tool-name rules. + * 3. The tool-name rules (#346, #372) come first otherwise: terminal_run, Home security + * and the rest already name the action with their caution and icon. + * 4. When the rules return a generic label, the engine's risk_label breaks the tie if + * a describe_fn actually ran. + */ +export function resolveWording(approval: { + risk: 'read' | 'write' | 'send' | 'exec' + reason?: string | null + name: string + arguments?: Record + preview?: { kind?: string | null } | null + riskLabel?: string | null +}): CardWording { + if (approval.preview?.kind === 'trash' && approval.risk !== 'exec') + return { label: 'Moves to the trash', verb: 'move something to the trash', tone: 'warning' } + if (BROWSER_DESCRIBE_TOOLS.has(approval.name)) { + const label = (approval.riskLabel ?? '').trim() + // The verb falls back to what the rules would have said, so an engine label + // with no known verb (e.g. a future "Signs up") still reads as a sentence. + if (label) return { label, verb: ENGINE_VERBS[label] ?? approvalRisk(approval).verb, tone: ENGINE_TONE[approval.risk] } + } + const wording = approvalRisk(approval) + if (!GENERIC_RISK_LABELS.has(wording.label)) return wording + return engineWording(approval.riskLabel, approval.risk, wording.verb) ?? wording +} diff --git a/desktop/tests/approvalRisk.test.ts b/desktop/tests/approvalRisk.test.ts index dea85068..35682237 100644 --- a/desktop/tests/approvalRisk.test.ts +++ b/desktop/tests/approvalRisk.test.ts @@ -3,7 +3,7 @@ // Run with `npm test` (no Electron needed). import assert from 'node:assert/strict' import { test } from 'node:test' -import { approvalRisk } from '../src/features/chat/approvalRisk.ts' +import { approvalRisk, engineWording, resolveWording } from '../src/features/chat/approvalRisk.ts' const email = (name: string, body: string, reason = 'mcp_mailer: Send an email to one address') => ({ risk: 'send' as const, @@ -178,6 +178,132 @@ test('Home Assistant locks, alarms and garage doors read as home security, with assert.equal(blinds.caution, undefined) }) +// #347: the engine's risk_label (from a tool's describe_fn) is used when it is specific; +// the generic per-risk fallback means no describe_fn ran, so the tool-name rules keep naming it. +test('a specific engine label is used for the heading', () => { + const w = engineWording('Purchase', 'send') + assert.equal(w?.label, 'Purchase') + assert.equal(w?.verb, 'make a purchase') + assert.equal(w?.tone, 'danger') +}) + +test('a browser click on an ordinary button reads as a click', () => { + const w = engineWording('Clicks', 'write') + assert.equal(w?.label, 'Clicks') + assert.equal(w?.verb, 'click') + assert.equal(w?.tone, 'warning') +}) + +test('the generic send fallback keeps the #346 tool-name rules', () => { + assert.equal(engineWording('Sends', 'send'), null) +}) + +test('the generic write fallback keeps the #346 tool-name rules', () => { + assert.equal(engineWording('Changes something', 'write'), null) +}) + +test('a missing label keeps the #346 tool-name rules', () => { + assert.equal(engineWording(null, 'send'), null) + assert.equal(engineWording(' ', 'send'), null) +}) + +test('an unknown engine label is shown as-is with a plain verb', () => { + const w = engineWording('Does something new', 'write') + assert.equal(w?.label, 'Does something new') + assert.equal(w?.verb, 'do this') + assert.equal(w?.tone, 'warning') +}) + +test('labels are trimmed before matching', () => { + assert.equal(engineWording(' Clicks ', 'write')?.label, 'Clicks') +}) + +// #347 review: the full precedence — browser labels trusted, tool-name rules first, +// engine label only as the tiebreaker for generic rule results. +test('a browser_click labelled Sends reads as Sends, not Purchase', () => { + const w = resolveWording({ + risk: 'send', + name: 'browser_click', + reason: 'browser: Buttons that buy, pay for things or submit forms', + riskLabel: 'Sends' + }) + assert.equal(w.label, 'Sends') + assert.equal(w.verb, 'send something on your behalf') +}) + +test('a browser_click labelled Purchase still reads as a purchase', () => { + const w = resolveWording({ + risk: 'send', + name: 'browser_click', + reason: 'browser: Buttons that buy, pay for things or submit forms', + riskLabel: 'Purchase' + }) + assert.equal(w.label, 'Purchase') +}) + +test('terminal_run keeps the tool-name rules ahead of the engine label', () => { + const w = resolveWording({ risk: 'exec', name: 'terminal_run', riskLabel: 'Runs a command' }) + assert.equal(w.label, 'Runs a command') + assert.equal(w.verb, 'run a command on this computer') +}) + +test('a Home Assistant unlock keeps its security wording and caution', () => { + const w = resolveWording({ risk: 'send', name: 'home_assistant_unlock', riskLabel: 'Unlocks a door' }) + assert.equal(w.label, 'Home security') + assert.equal(w.caution, 'Anyone at that door can walk in once it is unlocked.') +}) + +test('a generic rule result lets a specific engine label break the tie', () => { + const w = resolveWording({ risk: 'send', name: 'device_take_photo', riskLabel: 'Takes a photo' }) + assert.equal(w.label, 'Takes a photo') + assert.equal(w.verb, 'take a photo') +}) + +// #347 review round 2: an engine label with no known verb keeps the rules' verb for +// the heading, so it stays a real sentence. The badge still shows the engine's label. +test('an unknown engine label falls back to the rules verb', () => { + const w = resolveWording({ risk: 'write', name: 'home_assistant_set_temperature', riskLabel: 'Sets a temperature' }) + assert.equal(w.label, 'Sets a temperature') + assert.equal(w.verb, 'make a change') +}) + +test('a browser_click labelled Signs up reads as signing up', () => { + const w = resolveWording({ + risk: 'send', + name: 'browser_click', + reason: 'browser: Buttons that buy, pay for things or submit forms', + riskLabel: 'Signs up' + }) + assert.equal(w.label, 'Signs up') + assert.equal(w.verb, 'sign you up for something') +}) + +test('a browser_select without a describe_fn keeps the tool-name rules', () => { + const w = resolveWording({ + risk: 'write', + name: 'browser_select', + reason: 'browser: Dropdowns and selects', + riskLabel: 'Changes something' + }) + assert.equal(w.label, 'Changes things') + assert.equal(w.verb, 'make a change') +}) + +test('a named rule result is not overridden by a fallback engine label', () => { + const w = resolveWording({ risk: 'send', name: 'gmail_send', riskLabel: 'Sends' }) + assert.equal(w.label, 'Sends an email') +}) + +test('a trash preview still reads as a move to the trash', () => { + const w = resolveWording({ + risk: 'write', + name: 'gmail_delete', + preview: { kind: 'trash' }, + riskLabel: 'Deletes' + }) + assert.equal(w.label, 'Moves to the trash') +}) + // #378: a free subscribe the engine labels "Signs up" reads as a sign-up, not a purchase; // a priced one keeps the purchase wording. const click = (riskLabel: string) => ({