From e17487c91b95f8fcef4ebff65a6e4ab069e6f016 Mon Sep 17 00:00:00 2001 From: Azmi Muwahid Date: Fri, 16 Oct 2026 19:34:00 +0700 Subject: [PATCH 1/2] fix(desktop): use the engine's risk_label for approval card headings When a tool's describe_fn names the actual action, the card's heading and badge use the engine's risk_label: a browser click's describe_fn reads the clicked element, so an ordinary button reads as a click, 'Send feedback' reads as sending, and 'Place order' still reads as a purchase. For other tools the tool-name rules come first (#346, #372: terminal_run, Home security); only when they return a generic label does the engine's label break the tie, and only when a describe_fn actually ran. Fixes #347 --- CHANGELOG.md | 4 + desktop/src/features/chat/ApprovalCard.tsx | 51 +++++++-- desktop/src/features/chat/approvalRisk.ts | 124 ++++++++++++++++++++- desktop/tests/approvalRisk.test.ts | 98 +++++++++++++++- 4 files changed, 260 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2930d77..de2bf563 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -403,6 +403,10 @@ 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. For other tools the card keeps naming the action from the tool's name + and declared risk, and only when that gives a generic label does the engine's label break the tie. - **Changing a rule while an email waits to go out now counts.** If you set an app or tool to **Ask** while one of its emails or messages waits the few seconds before it is sent, Sentient asks you before it goes out instead of sending it under the old rule. The same goes for **Only if I asked**, unless your message asked for that email. diff --git a/desktop/src/features/chat/ApprovalCard.tsx b/desktop/src/features/chat/ApprovalCard.tsx index 5f8c5d1f..781fd1a5 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, @@ -26,7 +29,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', @@ -67,21 +70,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'> + 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 2428d648..1b6b6544 100644 --- a/desktop/src/features/chat/approvalRisk.ts +++ b/desktop/src/features/chat/approvalRisk.ts @@ -4,11 +4,14 @@ * 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 browser 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. + * 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 @@ -162,3 +165,114 @@ 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): "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', + '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' +} + +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' +): EngineWording | null { + const label = (riskLabel ?? '').trim() + if (!label || label === ENGINE_FALLBACK[risk]) return null + return { label, verb: ENGINE_VERBS[label] ?? 'do this', 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 browser 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. + * 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 (approval.name.startsWith('browser_')) { + const label = (approval.riskLabel ?? '').trim() + if (label) return { label, verb: ENGINE_VERBS[label] ?? 'do this', 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 +} diff --git a/desktop/tests/approvalRisk.test.ts b/desktop/tests/approvalRisk.test.ts index 90247f04..1fe949c3 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, @@ -177,3 +177,99 @@ test('Home Assistant locks, alarms and garage doors read as home security, with assert.equal(blinds.label, 'Changes things') 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') +}) + +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') +}) From 9f49b0c0611122376ab11108c4056391b6821f51 Mon Sep 17 00:00:00 2001 From: Azmi Muwahid Date: Fri, 16 Oct 2026 20:43:06 +0700 Subject: [PATCH 2/2] fix(desktop): fall back to the rules verb for unknown engine labels Unknown engine labels (Home Assistant's 'Sets a temperature', the upcoming 'Signs up' from #382) read 'Sentient wants to do this'. Use what the tool-name rules would have said for the heading instead; the badge still shows the engine's label. Narrow the trusted browser branch to the tools with a describe_fn (browser_click/type/press) and trim the CHANGELOG. --- CHANGELOG.md | 3 +- desktop/src/features/chat/approvalRisk.ts | 35 +++++++++++++++++------ desktop/tests/approvalRisk.test.ts | 30 +++++++++++++++++++ 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de2bf563..aa94b44d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -405,8 +405,7 @@ and versions follow [Semantic Versioning](https://semver.org/). ### 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. For other tools the card keeps naming the action from the tool's name - and declared risk, and only when that gives a generic label does the engine's label break the tie. + "Place order" still reads as a purchase. - **Changing a rule while an email waits to go out now counts.** If you set an app or tool to **Ask** while one of its emails or messages waits the few seconds before it is sent, Sentient asks you before it goes out instead of sending it under the old rule. The same goes for **Only if I asked**, unless your message asked for that email. diff --git a/desktop/src/features/chat/approvalRisk.ts b/desktop/src/features/chat/approvalRisk.ts index 1b6b6544..bbbfbeec 100644 --- a/desktop/src/features/chat/approvalRisk.ts +++ b/desktop/src/features/chat/approvalRisk.ts @@ -6,9 +6,10 @@ * * 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 browser tool's describe_fn always runs and reads the actual element, so its risk_label + * 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. @@ -180,7 +181,7 @@ const ENGINE_FALLBACK: Record<'read' | 'write' | 'send' | 'exec', string> = { /** * Plain verbs for the engine's describe_fn action labels (sentient/browser/safety.py, - * terminal, nodes): "Sentient wants to ..." + verb. + * terminal, nodes, home assistant): "Sentient wants to ..." + verb. */ const ENGINE_VERBS: Record = { Purchase: 'make a purchase', @@ -189,6 +190,7 @@ const ENGINE_VERBS: Record = { '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', @@ -200,6 +202,13 @@ const ENGINE_VERBS: Record = { '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', @@ -225,11 +234,16 @@ export interface EngineWording { */ export function engineWording( riskLabel: string | null | undefined, - risk: 'read' | 'write' | 'send' | 'exec' + 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] ?? 'do this', tone: ENGINE_TONE[risk] } + return { label, verb: ENGINE_VERBS[label] ?? fallbackVerb, tone: ENGINE_TONE[risk] } } /** @@ -251,8 +265,9 @@ export interface CardWording { * 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 browser 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. + * 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 @@ -268,11 +283,13 @@ export function resolveWording(approval: { }): CardWording { if (approval.preview?.kind === 'trash' && approval.risk !== 'exec') return { label: 'Moves to the trash', verb: 'move something to the trash', tone: 'warning' } - if (approval.name.startsWith('browser_')) { + if (BROWSER_DESCRIBE_TOOLS.has(approval.name)) { const label = (approval.riskLabel ?? '').trim() - if (label) return { label, verb: ENGINE_VERBS[label] ?? 'do this', tone: ENGINE_TONE[approval.risk] } + // 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 + return engineWording(approval.riskLabel, approval.risk, wording.verb) ?? wording } diff --git a/desktop/tests/approvalRisk.test.ts b/desktop/tests/approvalRisk.test.ts index 1fe949c3..aa88ceb9 100644 --- a/desktop/tests/approvalRisk.test.ts +++ b/desktop/tests/approvalRisk.test.ts @@ -259,6 +259,36 @@ test('a generic rule result lets a specific engine label break the tie', () => { 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')