diff --git a/desktop/electron/src/browserbridge.test.ts b/desktop/electron/src/browserbridge.test.ts index 9c4ca8fd..239cd0cd 100644 --- a/desktop/electron/src/browserbridge.test.ts +++ b/desktop/electron/src/browserbridge.test.ts @@ -763,6 +763,7 @@ test('W3 dispatch: hub-leg read runs, ring-audited via:hub — but never hub-mir deps, { tool: 'browser_list_tabs', args: {}, agent_id: 'agent-remote', agent_handle: 'r2d2' }, new Set(), + 'browser', ); assert.equal(out.ok, true); if (out.ok) assert.match((out.result as ToolResult).content[0]?.text ?? '', /"tabId": 7/); @@ -792,7 +793,7 @@ test('W3 dispatch: local reads stay unaudited; actions and hub entries mirror', test('W3 dispatch: a revoked agent is refused for READS too — revoked means gone', async () => { const backend = actionBackend(); const { deps, entries } = auditDeps(backend); - const revoked = await dispatchHubInvoke(deps, { tool: 'browser_list_tabs', args: {}, agent_id: 'agent-remote' }, new Set(['agent-remote'])); + const revoked = await dispatchHubInvoke(deps, { tool: 'browser_list_tabs', args: {}, agent_id: 'agent-remote' }, new Set(['agent-remote']), 'browser'); assert.deepEqual(revoked, { ok: false, error: 'revoked by user on desktop' }); assert.equal(backend.calls.length, 0, 'refused before any CDP call'); assert.equal(entries.length, 0, 'a revoked refusal is a gate event, not an audited call'); @@ -801,7 +802,7 @@ test('W3 dispatch: a revoked agent is refused for READS too — revoked means go test('W3 dispatch: unknown tool refused without touching the backend', async () => { const backend = actionBackend(); const { deps, entries } = auditDeps(backend); - const out = await dispatchHubInvoke(deps, { tool: 'browser_nuke', args: {}, agent_id: 'agent-remote' }, new Set()); + const out = await dispatchHubInvoke(deps, { tool: 'browser_nuke', args: {}, agent_id: 'agent-remote' }, new Set(), 'browser'); assert.deepEqual(out, { ok: false, error: 'unknown_tool' }); assert.equal(backend.calls.length, 0); assert.equal(entries.length, 0); @@ -813,6 +814,7 @@ test('W3 dispatch: action call runs pre-authorized, audited once with via:hub', deps, { tool: 'browser_navigate', args: { tabId: 7, url: 'https://example.com/#frag' }, agent_id: 'agent-remote', agent_handle: 'r2d2' }, new Set(), + 'browser', ); assert.equal(out.ok, true); assert.equal(entries.length, 1, 'exactly one audit entry per action call'); @@ -828,6 +830,7 @@ test('W3 dispatch: action call runs pre-authorized, audited once with via:hub', deps, { tool: 'browser_navigate', args: { tabId: 999, url: 'https://example.com/' }, agent_id: 'agent-remote' }, new Set(), + 'browser', ); assert.equal(gone.ok, false); if (!gone.ok) assert.match(gone.error, /^TARGET_GONE: /); @@ -844,6 +847,7 @@ test('W3 dispatch: a revoked agent is refused before the tool machinery', async deps, { tool: 'browser_click', args: { tabId: 7, selector: '#go' }, agent_id: 'agent-remote' }, new Set(['agent-remote']), + 'browser', ); assert.deepEqual(out, { ok: false, error: 'revoked by user on desktop' }); assert.equal(backend.calls.length, 0, 'refused before any CDP call'); @@ -856,6 +860,7 @@ test('W3 dispatch: a tool-level isError result maps to the error half of the env deps, { tool: 'browser_eval', args: { tabId: 7, expression: 'thrower()' }, agent_id: 'agent-remote' }, new Set(), + 'browser', ); assert.equal(out.ok, false); if (!out.ok) { diff --git a/desktop/electron/src/browserbridge.ts b/desktop/electron/src/browserbridge.ts index f92bc9e4..e28200a1 100644 --- a/desktop/electron/src/browserbridge.ts +++ b/desktop/electron/src/browserbridge.ts @@ -1239,6 +1239,27 @@ export interface HubInvokePayload { /// hub's browser_invoke unwraps (ok → MCP result, !ok → agent-visible error). export type HubInvokeResult = { ok: true; result: unknown } | { ok: false; error: string }; +/// The two traffic classes the desktop exposes over the tunnel (D5). They are +/// separate envelope kinds because they are separate consent sentences: the +/// browser class drives embedded web pages, the desktop class describes and +/// captures the user's own screen. +export type TunnelClass = 'browser' | 'desktop'; + +export const TUNNEL_KINDS: Readonly> = { + browser: 'browser.invoke', + desktop: 'desktop.invoke', +}; + +/// Which class a tool belongs to, or null if we have never heard of it. The +/// desktop-UI tools live in READ_TOOLS for scope reasons (see +/// DESKTOP_ACTION_TOOL_NAMES), so membership alone cannot answer this — the +/// UI_TOOL_NAMES set does. +export function tunnelClassForTool(tool: string): TunnelClass | null { + if (UI_TOOL_NAMES.has(tool)) return 'desktop'; + if (READ_TOOLS.some((t) => t.name === tool) || ACTION_TOOL_NAMES.has(tool)) return 'browser'; + return null; +} + /// Dispatch one hub-relayed call in-process. This is the in-process entry /// into the SAME machinery the HTTP MCP path funnels into (callTool) — the /// bearer parse is skipped (the hub authenticated the agent and @@ -1248,10 +1269,18 @@ export async function dispatchHubInvoke( deps: McpServerDeps, payload: HubInvokePayload, revoked: ReadonlySet, + cls: TunnelClass, ): Promise { - const isRead = READ_TOOLS.some((t) => t.name === payload.tool); - const isAction = ACTION_TOOL_NAMES.has(payload.tool); - if (!isRead && !isAction) return { ok: false, error: 'unknown_tool' }; + const actual = tunnelClassForTool(payload.tool); + if (actual === null) return { ok: false, error: 'unknown_tool' }; + // Defense in depth against a hub that routed by the wrong kind: the hub + // gates BY CLASS (browser_invoke never raises a desktop_action card), so a + // ui_screenshot arriving in a browser.invoke envelope would be a capture + // nobody approved. The desktop is the authority for its own pixels — it + // checks rather than trusts. + if (actual !== cls) { + return { ok: false, error: `tool_kind_mismatch: '${payload.tool}' is not a ${TUNNEL_KINDS[cls]} tool` }; + } // The desktop's own kill switch, checked BEFORE the tool machinery runs — // a refusal is a gate event, not an audited action (the same posture as // W2's scope refusal). It covers READS too: "Revoke" must mean this agent diff --git a/desktop/electron/src/browserbridge_host.ts b/desktop/electron/src/browserbridge_host.ts index e65872cc..53b50979 100644 --- a/desktop/electron/src/browserbridge_host.ts +++ b/desktop/electron/src/browserbridge_host.ts @@ -51,6 +51,7 @@ import { READ_TOOLS, removeBridgeDiscovery, shouldMirrorAudit, + TUNNEL_KINDS, startBridgeServer, stripFragment, writeBridgeDiscovery, @@ -62,6 +63,7 @@ import { type HubInvokePayload, type HubInvokeResult, type McpServerDeps, + type TunnelClass, type UiCaptureRequest, type UiCaptureResult, } from './browserbridge'; @@ -420,10 +422,7 @@ async function startHubRelay(ctx: BridgeHubContext): Promise { const res = await fetch(`${ctx.baseUrl}/v1/teams/${encodeURIComponent(ctx.teamId)}/hosts`, { method: 'POST', headers: { 'content-type': 'application/json', authorization: `Bearer ${token}` }, - body: JSON.stringify({ - name: `desktop-${os.hostname()}`, - capabilities: { browser_bridge: true, app_version: app.getVersion(), tools: READ_TOOLS.length + ACTION_TOOLS.length }, - }), + body: JSON.stringify({ name: hostRegistrationName(), capabilities: hostCapabilities() }), signal: AbortSignal.timeout(10000), }); if (!res.ok) return; @@ -459,6 +458,38 @@ async function postHubHeartbeat(relay: HubRelay): Promise { } } +function hostRegistrationName(): string { + return `desktop-${os.hostname()}`; +} + +/// What this desktop advertises to the hub. `desktop_ui` (D5) tracks the +/// UI-context-sharing toggle, so a remote agent's hosts_list answer says +/// truthfully whether this desktop will talk about its own screen — the +/// toggle is not a refusal the agent discovers only on call. +function hostCapabilities(): Record { + return { + browser_bridge: true, + desktop_ui: uiFocusProvider?.available() ?? false, + app_version: app.getVersion(), + tools: READ_TOOLS.length + ACTION_TOOLS.length, + }; +} + +/// Re-post the registration so a mid-run toggle flip is reflected in what the +/// hub advertises (the POST upserts on (team, name), which is why W3 could +/// re-post on every enable). Best-effort and idempotent: a failure just +/// leaves the previous capabilities until the next sync. +export function refreshHostCapabilities(): void { + const relay = hubRelay; + if (relay === null) return; + void fetch(`${relay.ctx.baseUrl}/v1/teams/${encodeURIComponent(relay.ctx.teamId)}/hosts`, { + method: 'POST', + headers: { 'content-type': 'application/json', authorization: `Bearer ${relay.token}` }, + body: JSON.stringify({ name: hostRegistrationName(), capabilities: hostCapabilities() }), + signal: AbortSignal.timeout(10000), + }).catch(() => undefined); +} + /// Abortable sleep for the poll backoff. function sleep(ms: number, signal: AbortSignal): Promise { return new Promise((resolve) => { @@ -527,10 +558,13 @@ async function hubTunnelLoop(relay: HubRelay): Promise { } } -/// Route one envelope. Only kind browser.invoke is ours; anything else gets -/// the contract's unknown_kind error rather than killing the loop. +/// Route one envelope. Two kinds are ours (D5) — browser.invoke drives the +/// embedded tabs, desktop.invoke describes/captures the desktop's own UI — +/// and they share this dispatcher; anything else gets the contract's +/// unknown_kind error rather than killing the loop. async function answerTunnelEnvelope(env: TunnelEnvelope): Promise { - if (env.kind !== 'browser.invoke') return { ok: false, error: 'unknown_kind' }; + const cls = (Object.keys(TUNNEL_KINDS) as TunnelClass[]).find((k) => TUNNEL_KINDS[k] === env.kind); + if (cls === undefined) return { ok: false, error: 'unknown_kind' }; const deps = mcpDeps; if (deps === null) return { ok: false, error: 'bridge not running' }; const p = (env.payload !== null && typeof env.payload === 'object' ? env.payload : {}) as Record; @@ -540,7 +574,7 @@ async function answerTunnelEnvelope(env: TunnelEnvelope): Promise { // dispatchHubInvoke classifies ui_get_focus as a read tool — the gate at - // the tool (not just the catalog filter) is what keeps a remote call - // consent-gated until D5 builds its own stack. - const off = await dispatchHubInvoke(deps(SNAPSHOT, false), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set()); + // the tool (not just the catalog filter) is what keeps the D5 tunnel leg + // consent-gated: a relayed call hits the same refusal as a local one. + const off = await dispatchHubInvoke(deps(SNAPSHOT, false), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set(), 'desktop'); assert.equal(off.ok, false); if (!off.ok) assert.match(off.error, /UI_UNAVAILABLE/); - const on = await dispatchHubInvoke(deps(SNAPSHOT, true), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set()); + const on = await dispatchHubInvoke(deps(SNAPSHOT, true), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set(), 'desktop'); assert.equal(on.ok, true); }); diff --git a/desktop/electron/src/desktopui.ts b/desktop/electron/src/desktopui.ts index b1403c4e..6b6cd871 100644 --- a/desktop/electron/src/desktopui.ts +++ b/desktop/electron/src/desktopui.ts @@ -16,10 +16,13 @@ /// toggle-off removes just that entry. All file mechanics live in the /// electron-free kimimcp.ts. /// -/// D1 is LOCAL-ONLY: nothing here touches the hub or the tunnel (that is D5). +/// D1 shipped LOCAL-ONLY; since D5 this module also re-posts the `desktop_ui` +/// capability to the hub when the toggle flips (refreshHostCapabilities), so +/// `hosts_list` answers truthfully — the tunnel routing itself lives in +/// browserbridge_host.ts. import os from 'node:os'; import { installStableRelay, mergeSharingEntry, removeSharingEntry, type KimiMcpWrite } from './kimimcp'; -import { setUiFocusProvider, stdioBridgePath } from './browserbridge_host'; +import { refreshHostCapabilities, setUiFocusProvider, stdioBridgePath } from './browserbridge_host'; import type { Handler } from './ipc/dispatch'; /// Whether the user turned UI context sharing on (Settings → Assistant). @@ -82,6 +85,10 @@ export const desktopuiHandlers: Record = { sharingEnabled = args.enabled === true; if (!sharingEnabled) focusCache = null; const mcp = reconcileSharing(sharingEnabled); + // D5: `desktop_ui` in the hub host row tracks this toggle, so a remote + // agent's hosts_list answer says truthfully whether this desktop will + // talk about its own screen. No-op unless a hub relay is live. + refreshHostCapabilities(); return { enabled: sharingEnabled, mcp }; }, /// The renderer's focus push. Validated defensively (shape + size) — the diff --git a/desktop/electron/src/tunnelclass.test.ts b/desktop/electron/src/tunnelclass.test.ts new file mode 100644 index 00000000..f80d1416 --- /dev/null +++ b/desktop/electron/src/tunnelclass.test.ts @@ -0,0 +1,83 @@ +/// Tests for D5's two-class tunnel dispatch +/// (docs/plans/desktop-ui-context-and-pointing.md §3.6): the desktop now +/// answers TWO envelope kinds, and a tool must arrive under its own. That +/// check is the desktop refusing to trust the hub's routing — the hub gates +/// BY CLASS, so a ui_screenshot smuggled in a browser.invoke envelope would +/// be a capture nobody approved. `node --test`. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + ACTION_TOOLS, + dispatchHubInvoke, + READ_TOOLS, + TUNNEL_KINDS, + tunnelClassForTool, + UI_TOOL_NAMES, + type BridgeBackend, + type McpServerDeps, + type UiCaptureResult, +} from './browserbridge.ts'; + +const backend: BridgeBackend = { + listTargets: () => [{ tabId: 7, url: 'https://a.b/c', title: 't', partition: 'persist:webtab', bridge: 'full' }], + sendCommand: async () => ({ data: 'aGVsbG8=' }), +}; + +function deps(): McpServerDeps { + return { + backend, + serverInfo: { name: 'termipod-browser', version: '0.0.0-test' }, + uiFocusAvailable: () => true, + getUiFocus: () => ({ surface: 'read', captured_at: 'x' }), + captureUi: async (): Promise => ({ ok: true, data_b64: 'aGk=', width: 10, height: 10 }), + }; +} + +test('every tool belongs to exactly one class, and the kinds are distinct', () => { + for (const t of READ_TOOLS) { + assert.equal(tunnelClassForTool(t.name), UI_TOOL_NAMES.has(t.name) ? 'desktop' : 'browser', t.name); + } + for (const t of ACTION_TOOLS) assert.equal(tunnelClassForTool(t.name), 'browser', t.name); + assert.equal(tunnelClassForTool('browser_nuke'), null); + assert.notEqual(TUNNEL_KINDS.browser, TUNNEL_KINDS.desktop); +}); + +test('a desktop-UI tool smuggled in a browser envelope is refused', async () => { + // The hub's browser_invoke never raises a desktop_action card, so routing + // ui_screenshot as browser.invoke would land an unapproved capture. The + // desktop is the authority for its own pixels: it checks rather than trusts. + const out = await dispatchHubInvoke(deps(), { tool: 'ui_screenshot', args: {}, agent_id: 'ag_1' }, new Set(), 'browser'); + assert.equal(out.ok, false); + if (!out.ok) assert.match(out.error, /tool_kind_mismatch/); + + const focus = await dispatchHubInvoke(deps(), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set(), 'browser'); + assert.equal(focus.ok, false); +}); + +test('a browser tool smuggled in a desktop envelope is refused too', async () => { + const out = await dispatchHubInvoke(deps(), { tool: 'browser_list_tabs', args: {}, agent_id: 'ag_1' }, new Set(), 'desktop'); + assert.equal(out.ok, false); + if (!out.ok) assert.match(out.error, /tool_kind_mismatch/); +}); + +test('each tool routes under its own kind', async () => { + const focus = await dispatchHubInvoke(deps(), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set(), 'desktop'); + assert.equal(focus.ok, true); + const tabs = await dispatchHubInvoke(deps(), { tool: 'browser_list_tabs', args: {}, agent_id: 'ag_1' }, new Set(), 'browser'); + assert.equal(tabs.ok, true); +}); + +test('an unknown tool is unknown before it is mismatched', async () => { + // Order matters for the message an agent reads: "I have never heard of + // this" is more useful than "wrong envelope". + for (const cls of ['browser', 'desktop'] as const) { + const out = await dispatchHubInvoke(deps(), { tool: 'ui_mind_read', args: {}, agent_id: 'ag_1' }, new Set(), cls); + assert.deepEqual(out, { ok: false, error: 'unknown_tool' }); + } +}); + +test('revocation still outranks the class check', async () => { + const out = await dispatchHubInvoke(deps(), { tool: 'ui_get_focus', args: {}, agent_id: 'ag_1' }, new Set(['ag_1']), 'desktop'); + assert.equal(out.ok, false); + if (!out.ok) assert.match(out.error, /revoked/); +}); diff --git a/desktop/electron/src/uiscreenshot.test.ts b/desktop/electron/src/uiscreenshot.test.ts index 6ba8332f..61c9c2e5 100644 --- a/desktop/electron/src/uiscreenshot.test.ts +++ b/desktop/electron/src/uiscreenshot.test.ts @@ -217,7 +217,7 @@ test('ui_screenshot mirrors to the hub on every leg; ui_get_focus does not', () test('a hub-relayed capture is marked via:hub so the desktop raises no second card', async () => { const h = harness(); - const out = await dispatchHubInvoke(h.deps, { tool: 'ui_screenshot', args: {}, agent_id: 'ag_9', agent_handle: 'remote-1' }, new Set()); + const out = await dispatchHubInvoke(h.deps, { tool: 'ui_screenshot', args: {}, agent_id: 'ag_9', agent_handle: 'remote-1' }, new Set(), 'desktop'); assert.equal(out.ok, true); assert.equal(h.seen[0]?.via, 'hub', 'the hub gated this call already (D5) — a second card would double-prompt'); assert.equal(h.seen[0]?.agentId, 'ag_9'); @@ -226,7 +226,7 @@ test('a hub-relayed capture is marked via:hub so the desktop raises no second ca test('a revoked agent cannot capture, and never reaches the provider', async () => { const h = harness(); - const out = await dispatchHubInvoke(h.deps, { tool: 'ui_screenshot', args: {}, agent_id: 'ag_9' }, new Set(['ag_9'])); + const out = await dispatchHubInvoke(h.deps, { tool: 'ui_screenshot', args: {}, agent_id: 'ag_9' }, new Set(['ag_9']), 'desktop'); assert.equal(out.ok, false); assert.equal(h.seen.length, 0); }); diff --git a/docs/plans/desktop-ui-context-and-pointing.md b/docs/plans/desktop-ui-context-and-pointing.md index e43268c2..c423b085 100644 --- a/docs/plans/desktop-ui-context-and-pointing.md +++ b/docs/plans/desktop-ui-context-and-pointing.md @@ -4,20 +4,22 @@ > **Status:** In flight (2026-07-31) — wedges D1–D6 below; D1+D2 (the LOCAL > kimi-web loop) are the first priority, remote/hub-relayed delivery (D5) > is second. **D1 shipped (#476), D2 shipped (#477) + D2.1 (#480); -> D3 + D4 implemented — PRs pending.** Builds on the agent browser bridge +> D3–D5 implemented — PRs pending.** Builds on the agent browser bridge > (W1–W3 shipped, `docs/plans/desktop-agent-browser-bridge.md`). > **Derives from > [ADR-062](../decisions/062-desktop-ui-as-agent-addressable-entity.md)** — > verbs over a first-class UI entity: UIRef both directions, one > per-surface policy table, three representations, agent pointing (D6). > **Audience:** principal · contributors · maintainers -> **Last verified vs code:** the D3+D4 wedges on origin/main `4f5d86c3` -> (rebased past the unified assistant dock #483): frontend + electron typecheck -> green, 293 electron `node --test` pass (D3 +21, D4 +10), 380 frontend -> state/ssh tests pass, `lint-desktop-tokens.sh` clean at baseline 65. +> **Last verified vs code:** the D3–D5 wedges on origin/main `4f5d86c3` +> (rebased past the unified assistant dock #483): frontend + electron +> typecheck green, 299 electron `node --test` pass (D3 +21, D4 +10, D5 +> +6), 409 frontend state/ssh tests pass, `go build ./...` + +> `go test ./...` green, `lint-desktop-tokens.sh` clean at baseline 65. > The Electron halves (`capturePage`, the hub approval round trip, the -> live CDP hit-test) are **unexercised on this machine** — no display, -> no Electron binary — and need the Playwright/desktop pass +> live CDP hit-test) and the live tunnel are **unexercised on this +> machine** — no display, no Electron binary — and need the +> Playwright/desktop pass **Review amendments (2026-07-30), folded into the body below.** The relay fallback is read-token-only (§3.5); the `mcp.json` entry pins a stable @@ -668,6 +670,49 @@ workdir materialization fallback (`.termipod/annotations/.png` + path in the note text); hub tool tests mirroring `mcp_browser_bridge_test.go`. +> **D5 as shipped.** The generalization cost about what §3.6 predicted — +> one grant store, one approval helper, one router, two kind constants — +> and the interesting parts are the three places the classes deliberately +> DIFFER: +> +> - **Grants are keyed by kind; revocation is not.** Granting `browser` +> must never silence a `desktop_action` card (the review amendment). +> But "Revoke" in Settings → Remote driving means this agent no longer +> touches this desktop, so revoke clears every kind — the same +> asymmetry the desktop's own revoked-set already applies to reads. +> - **`ui_screenshot` consults no grant at all**, and the approval helper +> enforces that rather than trusting the client to hide the button: a +> class with no grant namespace ignores `option_id: "session"` +> entirely. The card also carries `session_grant: false` so a renderer +> states the consent shape instead of inferring it from the kind. +> - **The desktop re-checks the envelope kind.** The hub gates BY CLASS, +> so a `ui_screenshot` routed as `browser.invoke` would be a capture +> nobody approved. `dispatchHubInvoke` refuses a tool that arrived +> under the wrong kind — the desktop is the authority for its own +> pixels, and it checks rather than trusts. +> +> The capability key `desktop_ui` tracks the sharing toggle and is +> re-posted when it flips, so `hosts_list` answers truthfully instead of +> making the agent discover the refusal on call. Bridge toggle + +> Remote-driving toggle still gate the relay itself, so all three +> consents stand. +> +> **The driver fallback replaced three drop sites, not one.** The plan +> named pane/stdio and `image:false` ACP; gemini's exec-per-turn argv was +> a third with the same defect, so all three now materialize into +> `/.termipod/annotations/` (0600) and name the path in the +> text. Two consequences worth stating: an image-only input used to be +> *rejected* by the gemini driver and is now a valid turn whose body is +> the path (the user pointed at something and said nothing — a legitimate +> message); and a FAILED write keeps the historical drop-and-warn, with +> the reason in the event, because "notes the drop rather than failing +> silently" is the rule. Two review follow-ons: the raw-PaneDriver +> fallback now cd-wraps its launch into the same derived workdir every +> specialized launcher uses (its `Workdir` was previously a derivation +> the pane never entered, so a cwd-confined agent could not find the +> crop), and the annotations directory is capped (newest 32 kept — +> crops are one-turn artifacts, not a gallery). + **D6 — agent pointing (§3.4b).** Ref-chips: transcript renderer for agent-emitted UIRefs + the UIRef→focus dispatcher (chips ship first — they are pure rendering, no consent surface). `ui_highlight`: overlay diff --git a/docs/reference/attention-kinds.md b/docs/reference/attention-kinds.md index e62a5938..5a052ac4 100644 --- a/docs/reference/attention-kinds.md +++ b/docs/reference/attention-kinds.md @@ -3,7 +3,7 @@ > **Type:** reference > **Status:** Current (2026-06-05) > **Audience:** contributors (humans + AI agent maintainers) -> **Last verified vs code:** v1.0.805 +> **Last verified vs code:** 2026.730.1231 (D3 introduced `desktop_action`; D5 adds the hub-raised leg) **TL;DR.** When an agent needs the principal to weigh in, it picks one of three interaction shapes — `approval_request` (binary), `select` @@ -85,6 +85,44 @@ Read tools (`browser_list_tabs`, `browser_snapshot`, `browser_screenshot`, `browser_read_text`) never raise a row — they route immediately. +### Its per-call sibling — `desktop_action` + +| Kind | Answer space | Raised by | Rendering | +|---|---|---|---| +| `desktop_action` | binary {approve, reject} — **no session option** | the desktop itself (local calls) or the hub's `desktop_ui_invoke` handler (remote), for `ui_screenshot` | Allow once / Reject | + +The same machinery as `browser_action`, one rule apart: **a screenshot +of the desktop never gets a standing grant** +([ADR-062](../decisions/062-desktop-ui-as-agent-addressable-entity.md) +D-4, `plans/desktop-ui-context-and-pointing.md` §3.3). A frame of +everything the user can see is the most sensitive artifact the app can +emit, so consent is per call, forever — `option_id: "session"` is +accepted and **ignored**, and the card's `pending_payload` carries +`session_grant: false` so a client renders the right buttons instead of +inferring them from the kind. + +`pending_payload` differs by leg, because the two raisers know different +things — both are *references* to what was requested, never a pixel: the +image does not exist until this decision says yes. + +- **Desktop-raised (local calls):** `{tool, scope, surfaces, url, + agent_id, session_grant}` — the desktop can say which surfaces are on + screen, or which embedded tab. +- **Hub-raised (remote, `desktop_ui_invoke`):** `{host_id, host_name, + tool, args, agent_id, session_grant}` — the hub cannot know what is on + the remote screen, so a remote window-capture approval is + **sight-unseen**: the card names the host and the asking agent, not the + surfaces. (The desktop still re-applies its own refusal table after + routing, so a refused surface stays refused even post-approval.) + +Both legs raise it: a LOCAL agent's call is carded by the desktop (which +posts the row itself and parks for 2 minutes — shorter than the hub's 10, +because that leg holds an MCP call open over stdio), a hub-relayed call by +the hub before it routes. A call already carded by the hub is marked so the +desktop does not raise a second one. + +`ui_get_focus` never raises a row — it is a read. + ### The answerless sibling — `notice` | Kind | Answer space | MCP tool | Mobile rendering | diff --git a/hub/internal/hostrunner/annotation_materialize.go b/hub/internal/hostrunner/annotation_materialize.go new file mode 100644 index 00000000..541b857b --- /dev/null +++ b/hub/internal/hostrunner/annotation_materialize.go @@ -0,0 +1,188 @@ +// annotation_materialize.go — D5's fallback for image-less drivers +// (docs/plans/desktop-ui-context-and-pointing.md §3.5). +// +// The annotation overlay hands an image to the agent, and for the ACP +// and stream-json families that is native multimodal input. Three +// families cannot take it: +// +// - the PANE driver (M4 tmux): the pane is a terminal. There is no +// remote image channel — the kimi TUI's clipboard paste is a LOCAL +// feature, not something tmux send-keys can drive; +// - gemini's exec-per-turn argv (`gemini -p ""`): no inline +// image affordance at all; +// - an ACP agent that declares promptCapabilities.image == false. +// +// Until now all three DROPPED the image and (for two of them) posted a +// warning. Dropping is the wrong answer for an annotation: the user +// deliberately pointed at something, and every one of these agents can +// read a file. So the image is materialized into the agent's own +// workdir and the path is appended to the note text — "see the +// attached image at " — which the agent reads with its normal +// file tools. +// +// Two rules the plan states and this file enforces: +// - it lands under `/.termipod/annotations/`, a directory +// the agent already has, so no new permission surface appears; +// - "if the workdir write fails, the driver notes the drop rather +// than failing silently" — the caller keeps its existing warning +// event, and this function never fails the turn. + +package hostrunner + +import ( + "encoding/base64" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + "time" +) + +// decodeBase64Image tolerates the whitespace-bearing base64 the MCP +// attach path is known to produce (see the attach cap notes) — a +// forwarded payload that survived hub validation must not fail here on +// a newline. +func decodeBase64Image(data string) ([]byte, error) { + clean := strings.NewReplacer("\n", "", "\r", "", " ", "", "\t", "").Replace(data) + raw, err := base64.StdEncoding.DecodeString(clean) + if err != nil { + return nil, fmt.Errorf("not base64: %w", err) + } + if len(raw) == 0 { + return nil, fmt.Errorf("empty image") + } + return raw, nil +} + +// annotationDirName is the per-workdir home for materialized inputs. +// Under `.termipod/` because that prefix is already the agent-local +// scratch convention, and a dotted dir keeps it out of the way of the +// agent's actual work. +const annotationDirName = ".termipod/annotations" + +// annotationExt maps the mime types the hub accepts onto a file +// extension. An unknown mime gets `.bin`: the note names the path, and +// a wrong extension is a worse lie than a generic one. +func annotationExt(mime string) string { + switch strings.ToLower(strings.TrimSpace(mime)) { + case "image/png": + return ".png" + case "image/jpeg", "image/jpg": + return ".jpg" + case "image/gif": + return ".gif" + case "image/webp": + return ".webp" + default: + return ".bin" + } +} + +// annotationFilename is the stamped name for one materialized image. +// Deterministic given (stamp, index) so a test can assert it, and +// sortable so a workdir's annotations read chronologically. +func annotationFilename(stamp time.Time, index int, mime string) string { + return fmt.Sprintf("%s-%d%s", stamp.UTC().Format("20060102T150405Z"), index, annotationExt(mime)) +} + +// materializeImageInputs writes each image into +// `/.termipod/annotations/` and returns the paths written. +// +// Errors are returned but are NOT fatal to a turn: the caller reports +// the drop (its existing warning event) and sends the text alone. A +// partial success returns the paths that landed plus the error, so the +// note can still name what the agent can actually open. +func materializeImageInputs(workdir string, images []imageInput, stamp time.Time) ([]string, error) { + if len(images) == 0 { + return nil, nil + } + if workdir == "" { + return nil, fmt.Errorf("no workdir: cannot materialize %d image(s)", len(images)) + } + dir := filepath.Join(workdir, filepath.FromSlash(annotationDirName)) + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("mkdir %s: %w", dir, err) + } + paths := make([]string, 0, len(images)) + for i, img := range images { + raw, err := decodeBase64Image(img.data) + if err != nil { + return paths, fmt.Errorf("image %d: %w", i+1, err) + } + p := filepath.Join(dir, annotationFilename(stamp, i+1, img.mime)) + // 0o600: the crop is a frame of the user's screen. The agent + // runs as the same user, so this is enough to keep it off other + // accounts on a shared host. + if err := os.WriteFile(p, raw, 0o600); err != nil { + return paths, fmt.Errorf("write %s: %w", p, err) + } + paths = append(paths, p) + } + pruneAnnotationDir(dir) + return paths, nil +} + +// annotationKeep caps how many materialized images loiter per workdir. +// A crop is a one-turn artifact, not a gallery — without a cap the +// directory grows for the workdir's lifetime. Newest-kept, because a +// path named in a recent note should keep resolving; the filenames are +// UTC-stamped so lexical order IS chronological order. +const annotationKeep = 32 + +// pruneAnnotationDir drops the oldest materialized images beyond the +// cap. Best-effort by design: a prune failure must never fail the turn +// that just materialized successfully. +func pruneAnnotationDir(dir string) { + entries, err := os.ReadDir(dir) + if err != nil { + return + } + names := make([]string, 0, len(entries)) + for _, e := range entries { + if e.Type().IsRegular() { + names = append(names, e.Name()) + } + } + if len(names) <= annotationKeep { + return + } + sort.Strings(names) + for _, name := range names[:len(names)-annotationKeep] { + _ = os.Remove(filepath.Join(dir, name)) + } +} + +// fallbackReason renders the materialization failure for the warning +// event. "the driver notes the drop rather than failing silently" +// (plan §3.5) — the principal needs to know WHY their annotation did +// not reach the agent, not just that it didn't. +func fallbackReason(err error) string { + if err == nil { + return "partial write" + } + return err.Error() +} + +// annotationNote appends the materialized paths to the user's text so +// the agent knows the image exists and where to open it. The user's +// own words come first — they are the message. +func annotationNote(body string, paths []string) string { + if len(paths) == 0 { + return body + } + var b strings.Builder + if body != "" { + b.WriteString(body) + b.WriteString("\n\n") + } + if len(paths) == 1 { + b.WriteString("[attached image saved at " + paths[0] + " — open it with your file tools]") + return b.String() + } + b.WriteString("[attached images saved — open them with your file tools]") + for _, p := range paths { + b.WriteString("\n- " + p) + } + return b.String() +} diff --git a/hub/internal/hostrunner/annotation_materialize_test.go b/hub/internal/hostrunner/annotation_materialize_test.go new file mode 100644 index 00000000..70e8864e --- /dev/null +++ b/hub/internal/hostrunner/annotation_materialize_test.go @@ -0,0 +1,197 @@ +// Tests for the D5 image-materialization fallback +// (docs/plans/desktop-ui-context-and-pointing.md §3.5): the drivers +// that cannot take an image block still deliver the user's annotation, +// by writing it where the agent's own file tools can open it. + +package hostrunner + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +var testStamp = time.Date(2026, 7, 31, 4, 5, 6, 0, time.UTC) + +func TestMaterializeImageInputs_WritesUnderTheAgentsOwnWorkdir(t *testing.T) { + wd := t.TempDir() + // "aGVsbG8=" is "hello". + paths, err := materializeImageInputs(wd, []imageInput{ + {mime: "image/png", data: "aGVsbG8="}, + {mime: "image/jpeg", data: "aGVsbG8="}, + }, testStamp) + if err != nil { + t.Fatalf("materialize: %v", err) + } + if len(paths) != 2 { + t.Fatalf("want 2 paths, got %v", paths) + } + // Under .termipod/ — a directory the agent already has, so no new + // permission surface appears for the sake of an annotation. + want := filepath.Join(wd, ".termipod", "annotations") + for _, p := range paths { + if filepath.Dir(p) != want { + t.Errorf("path %q is not under %q", p, want) + } + info, serr := os.Stat(p) + if serr != nil { + t.Fatalf("stat %s: %v", p, serr) + } + if info.Mode().Perm() != 0o600 { + t.Errorf("%s mode = %v, want 0600 (it is a frame of the user's screen)", p, info.Mode().Perm()) + } + } + // The extension follows the mime, so the agent's reader picks the + // right decoder. + if filepath.Ext(paths[0]) != ".png" || filepath.Ext(paths[1]) != ".jpg" { + t.Errorf("extensions do not follow the mime types: %v", paths) + } + body, rerr := os.ReadFile(paths[0]) + if rerr != nil || string(body) != "hello" { + t.Errorf("decoded bytes = %q (err %v), want %q", string(body), rerr, "hello") + } + // Stamped + indexed, so a workdir's annotations read chronologically. + if !strings.HasPrefix(filepath.Base(paths[0]), "20260731T040506Z-1") { + t.Errorf("unexpected filename %q", filepath.Base(paths[0])) + } +} + +func TestMaterializeImageInputs_FailsLoudlyWithNowhereToWrite(t *testing.T) { + // No workdir derived: the caller must be able to tell this from + // success, because its fallback is to warn the principal that the + // annotation did not arrive (plan §3.5). + paths, err := materializeImageInputs("", []imageInput{{mime: "image/png", data: "aGVsbG8="}}, testStamp) + if err == nil { + t.Fatal("want an error when there is no workdir") + } + if len(paths) != 0 { + t.Errorf("want no paths, got %v", paths) + } + + // A workdir that cannot hold a directory (it is a FILE) fails the + // same way rather than panicking. + f := filepath.Join(t.TempDir(), "not-a-dir") + if werr := os.WriteFile(f, []byte("x"), 0o600); werr != nil { + t.Fatal(werr) + } + if _, err = materializeImageInputs(f, []imageInput{{mime: "image/png", data: "aGVsbG8="}}, testStamp); err == nil { + t.Error("want an error when the workdir is not a directory") + } +} + +func TestMaterializeImageInputs_PartialWriteReportsWhatLanded(t *testing.T) { + // The second image is malformed. The first still landed, and the + // caller needs its path so the note can name what the agent CAN + // open — reporting zero would throw away a delivered image. + wd := t.TempDir() + paths, err := materializeImageInputs(wd, []imageInput{ + {mime: "image/png", data: "aGVsbG8="}, + {mime: "image/png", data: "!!!not base64!!!"}, + }, testStamp) + if err == nil { + t.Fatal("want an error for the malformed image") + } + if len(paths) != 1 { + t.Fatalf("want the one good path back, got %v", paths) + } +} + +func TestMaterializeImageInputs_ToleratesWhitespaceInBase64(t *testing.T) { + // The MCP attach path is known to produce whitespace-bearing base64; + // a payload that survived hub validation must not die here on a + // newline. + wd := t.TempDir() + paths, err := materializeImageInputs(wd, []imageInput{{mime: "image/png", data: "aGVs\nbG8 ="}}, testStamp) + if err != nil || len(paths) != 1 { + t.Fatalf("materialize: %v (paths %v)", err, paths) + } + body, _ := os.ReadFile(paths[0]) + if string(body) != "hello" { + t.Errorf("decoded %q, want %q", string(body), "hello") + } +} + +func TestMaterializeImageInputs_NoImagesIsNotAnError(t *testing.T) { + paths, err := materializeImageInputs("", nil, testStamp) + if err != nil || paths != nil { + t.Errorf("empty input must be a no-op: %v / %v", paths, err) + } +} + +func TestMaterializeImageInputs_PrunesTheOldestBeyondTheCap(t *testing.T) { + wd := t.TempDir() + dir := filepath.Join(wd, ".termipod", "annotations") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + // Pre-seed a full directory of older crops (UTC-stamped names sort + // chronologically, so lexical order is age order). + for i := 0; i < annotationKeep; i++ { + name := annotationFilename(testStamp.Add(-time.Duration(annotationKeep-i)*time.Minute), 1, "image/png") + if err := os.WriteFile(filepath.Join(dir, name), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + } + paths, err := materializeImageInputs(wd, []imageInput{{mime: "image/png", data: "aGVsbG8="}}, testStamp) + if err != nil || len(paths) != 1 { + t.Fatalf("materialize: %v %v", paths, err) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != annotationKeep { + t.Fatalf("want the cap (%d) after pruning, got %d", annotationKeep, len(entries)) + } + // The newest — the one a note just named — survived; the oldest went. + if _, serr := os.Stat(paths[0]); serr != nil { + t.Errorf("the just-materialized crop must never be pruned: %v", serr) + } + oldest := annotationFilename(testStamp.Add(-time.Duration(annotationKeep)*time.Minute), 1, "image/png") + if _, serr := os.Stat(filepath.Join(dir, oldest)); !os.IsNotExist(serr) { + t.Errorf("the oldest crop should have been pruned") + } +} + +func TestAnnotationNote_PutsTheUsersWordsFirst(t *testing.T) { + one := annotationNote("why is this red?", []string{"/w/.termipod/annotations/a.png"}) + if !strings.HasPrefix(one, "why is this red?") { + t.Errorf("the user's message must lead: %q", one) + } + if !strings.Contains(one, "/w/.termipod/annotations/a.png") { + t.Errorf("the note must name the path: %q", one) + } + if !strings.Contains(one, "file tools") { + t.Errorf("the note must say HOW to open it: %q", one) + } + + // "Just point at this" — an empty body still yields a usable turn. + bare := annotationNote("", []string{"/w/a.png"}) + if strings.TrimSpace(bare) == "" || !strings.Contains(bare, "/w/a.png") { + t.Errorf("image-only annotation must still say something: %q", bare) + } + + multi := annotationNote("look", []string{"/w/a.png", "/w/b.png"}) + if !strings.Contains(multi, "/w/a.png") || !strings.Contains(multi, "/w/b.png") { + t.Errorf("every path must be named: %q", multi) + } + + // Nothing materialized: the body is untouched, so a driver that + // failed to write does not append an empty bracket. + if got := annotationNote("look", nil); got != "look" { + t.Errorf("no paths must leave the body alone, got %q", got) + } +} + +func TestAnnotationExt_UnknownMimeIsGenericNotWrong(t *testing.T) { + if got := annotationExt("image/heic"); got != ".bin" { + // A wrong extension is a worse lie than a generic one: the note + // names the path either way, and the agent sniffs the bytes. + t.Errorf("unknown mime ext = %q, want .bin", got) + } + if got := annotationExt("IMAGE/PNG"); got != ".png" { + t.Errorf("mime matching must be case-insensitive, got %q", got) + } +} diff --git a/hub/internal/hostrunner/driver_acp.go b/hub/internal/hostrunner/driver_acp.go index 78aaf1a7..8ceb2c59 100644 --- a/hub/internal/hostrunner/driver_acp.go +++ b/hub/internal/hostrunner/driver_acp.go @@ -1719,13 +1719,28 @@ func (d *ACPDriver) Input(ctx context.Context, kind string, payload map[string]a // principal sees why their attachment didn't reach the agent; // otherwise we forward as-is. if len(images) > 0 && !d.promptCapImage() { - _ = d.Poster.PostAgentEvent(ctx, d.AgentID, "system", "agent", - map[string]any{ - "reason": "agent did not advertise image input support — attached images dropped", - "dropped": len(images), - "engine": "acp", - "capability": "promptCapabilities.image", - }) + // desktop-ui-context D5 §3.5: dropping is the wrong answer + // for an annotation — the user deliberately pointed at + // something, and this agent can read a file even though it + // cannot take an image block. Materialize into its workdir + // and name the path in the text; only a FAILED write falls + // back to the historical drop-and-warn. + paths, merr := materializeImageInputs(d.Workdir, images, time.Now()) + if merr == nil && len(paths) == len(images) { + body = annotationNote(body, paths) + } else { + _ = d.Poster.PostAgentEvent(ctx, d.AgentID, "system", "agent", + map[string]any{ + "reason": "agent did not advertise image input support — attached images dropped", + "dropped": len(images) - len(paths), + "engine": "acp", + "capability": "promptCapabilities.image", + "fallback": fallbackReason(merr), + }) + if len(paths) > 0 { + body = annotationNote(body, paths) + } + } images = nil if body == "" && len(pdfs)+len(audios)+len(videos) == 0 { return fmt.Errorf("acp driver: text input has no body and image attachments were dropped (agent rejected promptCapabilities.image)") diff --git a/hub/internal/hostrunner/driver_exec_resume.go b/hub/internal/hostrunner/driver_exec_resume.go index e794079d..7572f818 100644 --- a/hub/internal/hostrunner/driver_exec_resume.go +++ b/hub/internal/hostrunner/driver_exec_resume.go @@ -270,18 +270,36 @@ func (d *ExecResumeDriver) Input(ctx context.Context, kind string, payload map[s // artifact-type-registry W7.2 — PDF/audio/video share the same // strip-and-warn path; gemini exec-per-turn argv accepts no // multimodal attachments at all. + // desktop-ui-context D5 §3.5: images get one more chance before + // the drop — materialized into the agent's workdir with the + // path named in the prompt text, which gemini CAN open. The + // other modalities have no such fallback and still drop. + images := extractImageInputs(payload) dropped := 0 - dropped += len(extractImageInputs(payload)) + var imgErr error + if len(images) > 0 { + paths, merr := materializeImageInputs(d.Workdir, images, time.Now()) + body = annotationNote(body, paths) + if merr != nil || len(paths) != len(images) { + dropped += len(images) - len(paths) + imgErr = merr + } + } dropped += len(extractAttachmentInputs(payload, "pdfs")) dropped += len(extractAttachmentInputs(payload, "audios")) dropped += len(extractAttachmentInputs(payload, "videos")) if dropped > 0 { - _ = d.Poster.PostAgentEvent(ctx, d.AgentID, "system", "agent", - map[string]any{ - "reason": "gemini exec-per-turn has no inline multimodal support — switch to gemini --acp (M1) for multimodal turns", - "engine": "gemini-exec", - "dropped": dropped, - }) + evt := map[string]any{ + "reason": "gemini exec-per-turn has no inline multimodal support — switch to gemini --acp (M1) for multimodal turns", + "engine": "gemini-exec", + "dropped": dropped, + } + // Same field the pane/ACP drivers carry: WHY the workdir + // fallback failed for the dropped images, not just that it did. + if imgErr != nil { + evt["fallback"] = fallbackReason(imgErr) + } + _ = d.Poster.PostAgentEvent(ctx, d.AgentID, "system", "agent", evt) } if body == "" { return fmt.Errorf("exec-resume driver: text input missing body") diff --git a/hub/internal/hostrunner/driver_exec_resume_test.go b/hub/internal/hostrunner/driver_exec_resume_test.go index 3e8d481d..17fe2a2f 100644 --- a/hub/internal/hostrunner/driver_exec_resume_test.go +++ b/hub/internal/hostrunner/driver_exec_resume_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "io" + "os" + "path/filepath" "strings" "sync" "testing" @@ -802,15 +804,13 @@ func TestExecResumeDriver_NextTurnModeArgvSplice(t *testing.T) { } } -// TestExecResumeDriver_TextStripsImagesAndWarns — W4.5: gemini's -// exec-per-turn argv has no inline-image affordance, so when a hub -// input carries `images`, the driver: -// - emits a kind=system event noting the strip + the upgrade -// path (switch to gemini --acp / M1 for multimodal turns), -// - lets the text portion proceed normally as gemini -p . -// Hub-side W4.1 validation has already enforced caps; this is the -// last-mile drop. -func TestExecResumeDriver_TextStripsImagesAndWarns(t *testing.T) { +// TestExecResumeDriver_TextMaterializesImagesIntoTheWorkdir — W4.5 +// dropped images outright (gemini's exec-per-turn argv has no +// inline-image affordance). desktop-ui-context D5 §3.5 replaces the +// drop with a WRITE: the image lands in the agent's own workdir and +// the prompt names the path, which gemini can open. No warning is due +// — nothing was lost. +func TestExecResumeDriver_TextMaterializesImagesIntoTheWorkdir(t *testing.T) { fam, ok := agentfamilies.ByName("gemini-cli") if !ok { t.Fatal("gemini-cli family missing") @@ -829,12 +829,13 @@ func TestExecResumeDriver_TextStripsImagesAndWarns(t *testing.T) { return newFakeGeminiCmd(append([]string{name}, args...), frames) } poster := &recordingPoster{} + wd := t.TempDir() drv := &ExecResumeDriver{ AgentID: "agt-w45", Handle: "@steward", Poster: poster, Bin: "/usr/bin/gemini", - Workdir: "/tmp/wt", + Workdir: wd, FrameProfile: fam.FrameProfile, CommandBuilder: cb, KillGrace: 500 * time.Millisecond, @@ -853,46 +854,102 @@ func TestExecResumeDriver_TextStripsImagesAndWarns(t *testing.T) { t.Fatalf("Input: %v", err) } - // argv must carry the body, not an image flag (gemini-exec has - // no such flag — but check anyway for forward-compat regression - // guard if someone adds one to gemini and we forget to rewire). argsMu.Lock() args := append([]string{}, capturedArgs...) argsMu.Unlock() - if !containsArg(args, "-p", "describe this") { - t.Errorf("argv missing -p body: %v", args) + // The user's words survive verbatim; the path is appended, never + // substituted (the message is theirs, the path is grounding). + var prompt string + for i, a := range args { + if a == "-p" && i+1 < len(args) { + prompt = args[i+1] + } + } + if !strings.HasPrefix(prompt, "describe this") { + t.Errorf("prompt lost the user's body: %q", prompt) + } + if !strings.Contains(prompt, annotationDirName) || !strings.Contains(prompt, ".png") { + t.Errorf("prompt does not name the materialized image: %q", prompt) } + // The bytes go to disk, never into argv (a base64 frame in the + // process table is both a leak and an ARG_MAX hazard). for _, a := range args { if a == "AAA=" { t.Errorf("argv leaked image data: %v", args) } } - - // Find the system warn event. + written, err := filepath.Glob(filepath.Join(wd, ".termipod", "annotations", "*.png")) + if err != nil || len(written) != 1 { + t.Fatalf("expected one materialized png in %s, got %v (err %v)", wd, written, err) + } + if info, serr := os.Stat(written[0]); serr != nil || info.Mode().Perm() != 0o600 { + t.Errorf("materialized crop must be 0600: %v (err %v)", info, serr) + } + // Nothing was dropped, so no drop warning is due. poster.mu.Lock() defer poster.mu.Unlock() - var found bool for _, e := range poster.events { - if e.Kind != "system" { - continue - } - reason, _ := e.Payload["reason"].(string) - engine, _ := e.Payload["engine"].(string) - if engine == "gemini-exec" && reason != "" && strings.Contains(strings.ToLower(reason), "no inline multimodal support") { - found = true - break + if e.Kind == "system" { + if reason, _ := e.Payload["reason"].(string); strings.Contains(reason, "dropped") { + t.Errorf("no drop warning is due when the image was delivered: %+v", e.Payload) + } } } - if !found { - t.Errorf("expected gemini-exec strip warning; got events %+v", poster.events) +} + +// TestExecResumeDriver_ImageOnlyBecomesAPathTurn — an image-only input +// used to be rejected (the strip left nothing to send). With the D5 +// fallback it becomes a valid turn whose whole body is the path: the +// user pointed at something and said nothing, which is a legitimate +// message. +func TestExecResumeDriver_ImageOnlyBecomesAPathTurn(t *testing.T) { + fam, _ := agentfamilies.ByName("gemini-cli") + frames := []string{ + `{"type":"init","session_id":"sess-w45c","model":"gemini-2.5-pro","timestamp":"t"}`, + `{"type":"result","status":"success","timestamp":"t"}`, + } + var capturedArgs []string + var argsMu sync.Mutex + cb := func(ctx context.Context, name string, args ...string) GeminiCmd { + argsMu.Lock() + capturedArgs = append([]string{name}, args...) + argsMu.Unlock() + return newFakeGeminiCmd(append([]string{name}, args...), frames) + } + drv := &ExecResumeDriver{ + AgentID: "agt-w45c", + Handle: "@steward", + Poster: &recordingPoster{}, + Bin: "/usr/bin/gemini", + Workdir: t.TempDir(), + FrameProfile: fam.FrameProfile, + CommandBuilder: cb, + KillGrace: 500 * time.Millisecond, + } + if err := drv.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + defer drv.Stop() + + if err := drv.Input(context.Background(), "text", map[string]any{ + "images": []any{map[string]any{"mime_type": "image/png", "data": "AAA="}}, + }); err != nil { + t.Fatalf("Input: %v", err) + } + argsMu.Lock() + args := append([]string{}, capturedArgs...) + argsMu.Unlock() + if len(args) == 0 { + t.Fatal("expected a turn to run") } } -// TestExecResumeDriver_TextImagesOnlyStillRejected — gemini-exec -// can't carry images at all; an image-only input (no body) is still -// invalid because the strip leaves nothing to send. The text-input -// missing-body error is the right user-visible signal. -func TestExecResumeDriver_TextImagesOnlyStillRejected(t *testing.T) { +// TestExecResumeDriver_ImageDropWarnsWhenTheWorkdirWriteFails — the +// plan's explicit rule: "if the workdir write fails, the driver notes +// the drop rather than failing silently". With no workdir there is +// nowhere to put the crop, so the historical drop-and-warn stands AND +// an image-only input is rejected again. +func TestExecResumeDriver_ImageDropWarnsWhenTheWorkdirWriteFails(t *testing.T) { fam, _ := agentfamilies.ByName("gemini-cli") cb := func(ctx context.Context, name string, args ...string) GeminiCmd { t.Fatalf("CommandBuilder must not be invoked when body is empty") @@ -904,7 +961,7 @@ func TestExecResumeDriver_TextImagesOnlyStillRejected(t *testing.T) { Handle: "@steward", Poster: poster, Bin: "/usr/bin/gemini", - Workdir: "/tmp/wt", + Workdir: "", // no derivation — nowhere to write FrameProfile: fam.FrameProfile, CommandBuilder: cb, KillGrace: 500 * time.Millisecond, @@ -915,15 +972,11 @@ func TestExecResumeDriver_TextImagesOnlyStillRejected(t *testing.T) { defer drv.Stop() err := drv.Input(context.Background(), "text", map[string]any{ - "images": []any{ - map[string]any{"mime_type": "image/png", "data": "AAA="}, - }, + "images": []any{map[string]any{"mime_type": "image/png", "data": "AAA="}}, }) if err == nil { - t.Fatal("expected error for image-only input on gemini-exec") + t.Fatal("expected error for image-only input that could not be materialized") } - // Still warn — the principal needs to know images were dropped - // before the error message about missing body. poster.mu.Lock() defer poster.mu.Unlock() var sawWarn bool @@ -937,7 +990,7 @@ func TestExecResumeDriver_TextImagesOnlyStillRejected(t *testing.T) { } } if !sawWarn { - t.Errorf("expected gemini-exec strip warning before missing-body error; events=%+v", poster.events) + t.Errorf("expected gemini-exec drop warning; events=%+v", poster.events) } } diff --git a/hub/internal/hostrunner/driver_pane.go b/hub/internal/hostrunner/driver_pane.go index 67db7be4..98ba86e4 100644 --- a/hub/internal/hostrunner/driver_pane.go +++ b/hub/internal/hostrunner/driver_pane.go @@ -47,6 +47,13 @@ type PaneDriver struct { // SendKeys lets tests inject a fake for tmux send-keys; nil defaults // to the real tmuxSendKeys below. Used by Input (P1.8) for M4 input. SendKeys PaneSendKeysFunc + // Workdir is the agent's cwd, when the runner could derive one. A + // pane is a terminal — there is no image channel into it at all — + // so an annotation image is materialized here and its path named in + // the text (desktop-ui-context D5 §3.5). Empty means "no workdir + // derived": the image is reported as dropped rather than written + // somewhere the agent cannot see. + Workdir string mu sync.Mutex started bool @@ -226,6 +233,23 @@ func (d *PaneDriver) Input(ctx context.Context, kind string, payload map[string] switch kind { case "text": body, _ := payload["body"].(string) + // D5 §3.5: the pane cannot receive an image (the kimi TUI's + // clipboard paste is a LOCAL feature, not something send-keys + // can drive), so an annotation crop is written into the agent's + // workdir and the path rides the text it CAN read. + if images := extractImageInputs(payload); len(images) > 0 { + paths, merr := materializeImageInputs(d.Workdir, images, time.Now()) + body = annotationNote(body, paths) + if merr != nil || len(paths) != len(images) { + _ = d.Poster.PostAgentEvent(ctx, d.AgentID, "system", "agent", + map[string]any{ + "reason": "a tmux pane has no image input channel and the workdir fallback failed — attached images dropped", + "dropped": len(images) - len(paths), + "engine": "pane", + "fallback": fallbackReason(merr), + }) + } + } if body == "" { return fmt.Errorf("pane driver: text input missing body") } diff --git a/hub/internal/hostrunner/runner.go b/hub/internal/hostrunner/runner.go index bc43793e..e5129034 100644 --- a/hub/internal/hostrunner/runner.go +++ b/hub/internal/hostrunner/runner.go @@ -881,7 +881,27 @@ func (a *Runner) launchOne(ctx context.Context, sp Spawn) { } var p string var err error + paneWorkdir := "" if cmd != "" { + // Run the raw pane in the SAME derived workdir every specialized + // launcher cds into — without the cd, the pane inherits the tmux + // server's cwd while annotation images (D5 §3.5) materialize into + // the derived path, where a cwd-confined agent would never look. + // Best-effort: a failed derivation/mkdir launches bare (the old + // behaviour) and leaves Workdir empty, so materialization + // drop-with-warns instead of naming a directory the agent isn't in. + if wd := DeriveWorkdir(a.Client.Team, spec.Backend.DefaultWorkdir, sp.ProjectID, + sp.Handle, sp.ChildID, false); wd != "" { + if expanded, werr := expandHome(wd); werr == nil { + if merr := os.MkdirAll(expanded, 0o755); merr == nil { + paneWorkdir = expanded + cmd = fmt.Sprintf("cd %s && %s", shellEscape(expanded), cmd) + } else { + a.Log.Warn("raw pane workdir mkdir failed; launching in the launcher default cwd", + "handle", sp.Handle, "workdir", expanded, "err", merr) + } + } + } // Raw PaneDriver: the agent runs in this tmux pane, so secrets go // via tmux -e (launchCmdWithEnv), never the command string. p, err = launchCmdWithEnv(ctx, a.Launcher, sp, cmd, secretEnv) @@ -920,11 +940,16 @@ func (a *Runner) launchOne(ctx context.Context, sp Spawn) { return } pane = p + // paneWorkdir is set ONLY when the launch above actually cd'd into + // it — Workdir must name where the agent runs, not a derivation the + // pane never entered (an annotation image materialized there would + // be invisible to a cwd-confined agent). drv = &PaneDriver{ AgentID: sp.ChildID, PaneID: pane, Poster: a.agentPoster, Log: a.Log, + Workdir: paneWorkdir, } } diff --git a/hub/internal/hostrunner/runner_launch_paneless_test.go b/hub/internal/hostrunner/runner_launch_paneless_test.go index 7fbc9b5d..92a6dfa5 100644 --- a/hub/internal/hostrunner/runner_launch_paneless_test.go +++ b/hub/internal/hostrunner/runner_launch_paneless_test.go @@ -353,6 +353,55 @@ func TestLaunchOne_RefusesEmptyBackendCmd(t *testing.T) { } } +// TestLaunchOne_RawPaneRunsInTheDerivedWorkdir pins the D5 §3.5 +// consistency rule: PaneDriver.Workdir is where annotation images +// materialize, so the pane command MUST actually run there — the raw +// fallback now cd-wraps exactly like every specialized launcher, and +// Workdir is set only when that cd really happened. +func TestLaunchOne_RawPaneRunsInTheDerivedWorkdir(t *testing.T) { + hub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + t.Cleanup(hub.Close) + + wd := t.TempDir() + launcher := &recordingLauncher{pane: "hub-agents:raw-worker.0"} + r := &Runner{ + Client: NewClient(hub.URL, "tok", "default"), + HostID: "host-x", + Launcher: launcher, + Log: slog.New(slog.NewTextHandler(io.Discard, nil)), + drivers: map[string]Driver{}, + tailers: map[string]*Tailer{}, + worktrees: map[string]WorktreeSpec{}, + panes: map[string]paneState{}, + templates: &agentTemplates{}, + } + r.agentPoster = r.Client + r.inputs = NewInputRouter(r.Client, r.Log) + + sp := Spawn{ + ChildID: "agent-raw-pane", + Handle: "raw-worker", + Kind: "some-tui", + Mode: "M4", + SpawnSpec: "driving_mode: M4\nbackend:\n cmd: some-tui --yolo\n default_workdir: " + wd + "\n", + } + r.launchOne(context.Background(), sp) + + got := launcher.gotCmd + if !strings.HasPrefix(got, "cd ") || !strings.Contains(got, wd) || !strings.Contains(got, "&& some-tui --yolo") { + t.Errorf("raw pane cmd should cd into the derived workdir first; got %q", got) + } + drv, ok := r.drivers[sp.ChildID].(*PaneDriver) + if !ok { + t.Fatalf("want a PaneDriver, got %T", r.drivers[sp.ChildID]) + } + if drv.Workdir != wd { + t.Errorf("Workdir must name where the pane actually runs: want %q, got %q", wd, drv.Workdir) + } +} + // TestLaunchOne_SkipsWhenDriverAlreadyRegistered pins the W3 dedup // guard. The respawn-loop bug in the coder.v1 incident (v1.0.619) // reproduced as follows: a malformed spawn fell through to the diff --git a/hub/internal/server/mcp_browser_bridge.go b/hub/internal/server/mcp_browser_bridge.go index 86a059a6..54bf9adc 100644 --- a/hub/internal/server/mcp_browser_bridge.go +++ b/hub/internal/server/mcp_browser_bridge.go @@ -43,6 +43,11 @@ import ( // Payload; an unknown Kind comes back as a transport error. const browserTunnelKind = "browser.invoke" +// browserGrantKind namespaces this class's session grants in the +// shared store. The desktop-UI class has its own (mcp_desktop_ui.go), +// so a grant for one never silences the other's card. +const browserGrantKind = "browser" + // browserInvokeTimeout caps one hub→desktop round trip. Tighter than // the approval wait; long enough for a navigate + CDP evaluation. const browserInvokeTimeout = 60 * time.Second @@ -99,40 +104,64 @@ func browserToolNames() []string { return out } -// browserGrantStore is the session-grant cache for action-tool -// approvals (W3). Keyed team|hostID|agentID; presence means the +// bridgeGrantStore is the session-grant cache for action-tool +// approvals (W3). Keyed kind|team|hostID|agentID; presence means the // principal approved with option_id="session" and this agent may run -// action tools against that desktop without further prompts. No -// expiry — a hub restart clears it (same posture as TunnelManager's -// in-memory state); revocation is explicit via the revoke endpoint. -type browserGrantStore struct { - m sync.Map // "team|hostID|agentID" → struct{}{} +// action tools of THAT KIND against that desktop without further +// prompts. No expiry — a hub restart clears it (same posture as +// TunnelManager's in-memory state); revocation is explicit via the +// revoke endpoint. +// +// The `kind` dimension is load-bearing (desktop-ui-context plan §3.6 +// review amendment): W3 keyed grants team|host|agent with no kind, so +// generalizing the helper to a second traffic class without it would +// let a browser-driving session grant silence a desktop screenshot +// card — two different sentences, one consent. (Screenshots go +// further and consult no grant at all; see the desktop_action rule in +// mcp_desktop_ui.go.) +type bridgeGrantStore struct { + m sync.Map // "kind|team|hostID|agentID" → struct{}{} } -func browserGrantKey(team, hostID, agentID string) string { - return team + "|" + hostID + "|" + agentID +func bridgeGrantKey(kind, team, hostID, agentID string) string { + return kind + "|" + team + "|" + hostID + "|" + agentID } -func (g *browserGrantStore) has(team, hostID, agentID string) bool { - _, ok := g.m.Load(browserGrantKey(team, hostID, agentID)) +func (g *bridgeGrantStore) has(kind, team, hostID, agentID string) bool { + _, ok := g.m.Load(bridgeGrantKey(kind, team, hostID, agentID)) return ok } -func (g *browserGrantStore) grant(team, hostID, agentID string) { - g.m.Store(browserGrantKey(team, hostID, agentID), struct{}{}) +func (g *bridgeGrantStore) grant(kind, team, hostID, agentID string) { + g.m.Store(bridgeGrantKey(kind, team, hostID, agentID), struct{}{}) } -// revoke drops one (team, host, agent) grant; an empty agentID clears -// every grant for the (team, host) pair — the desktop's own "forget -// all sessions" path. -func (g *browserGrantStore) revoke(team, hostID, agentID string) { +// revoke drops grants for one (team, host, agent) across EVERY kind; +// an empty agentID clears every grant for the (team, host) pair — the +// desktop's own "forget all sessions" path. +// +// Revocation is deliberately kind-BLIND even though granting is +// kind-scoped: "Revoke" in Settings → Remote driving means this agent +// no longer touches this desktop, and an agent that kept a standing +// browser grant after being revoked would make that pill a lie +// (the same asymmetry the desktop's own revoked-set applies to reads). +func (g *bridgeGrantStore) revoke(team, hostID, agentID string) { + suffix := "|" + team + "|" + hostID + "|" if agentID != "" { - g.m.Delete(browserGrantKey(team, hostID, agentID)) - return + suffix += agentID } - prefix := team + "|" + hostID + "|" g.m.Range(func(k, _ any) bool { - if ks, ok := k.(string); ok && strings.HasPrefix(ks, prefix) { + ks, ok := k.(string) + if !ok { + return true + } + if agentID != "" { + if strings.HasSuffix(ks, suffix) { + g.m.Delete(k) + } + return true + } + if strings.Contains(ks, suffix) { g.m.Delete(k) } return true @@ -197,9 +226,18 @@ func (s *Server) mcpBrowserInvoke(ctx context.Context, team, agentID string, raw // Action tools need a human decision unless the principal already // granted this (desktop, agent) session. - if class == "action" && !s.browserGrants.has(team, a.HostID, agentID) { - deny, jerr := s.requestBrowserApproval(ctx, team, agentID, agentHandle, - a.HostID, hostName, a.Tool, a.Args) + if class == "action" && !s.bridgeGrants.has(browserGrantKind, team, a.HostID, agentID) { + deny, jerr := s.requestBridgeApproval(ctx, bridgeApproval{ + Team: team, + AgentID: agentID, + AgentHandle: agentHandle, + HostID: a.HostID, + HostName: hostName, + Tool: a.Tool, + Args: a.Args, + AttentionKind: "browser_action", + GrantKind: browserGrantKind, + }) if jerr != nil { return nil, jerr } @@ -207,15 +245,42 @@ func (s *Server) mcpBrowserInvoke(ctx context.Context, team, agentID string, raw return deny, nil } } - return s.routeBrowserInvoke(ctx, agentID, agentHandle, a.HostID, a.Tool, a.Args) + return s.routeTunnelInvoke(ctx, browserTunnelKind, agentID, agentHandle, a.HostID, a.Tool, a.Args) +} + +// bridgeApproval is one approval request against a desktop. The two +// traffic classes differ only in the attention kind they raise and +// whether a session grant is on offer, so they share the parking +// machinery rather than each growing their own copy of it. +type bridgeApproval struct { + Team string + AgentID string + AgentHandle string + HostID string + HostName string + Tool string + Args json.RawMessage + // AttentionKind is the attention_items.kind the card is raised as + // ("browser_action", "desktop_action") — it selects the renderer on + // every client, so it must match what those clients branch on. + AttentionKind string + // GrantKind names the grant namespace an option_id="session" + // approve writes to. EMPTY means no session grant exists for this + // class: an approve authorizes exactly this call, no matter which + // option the principal picked. That is the desktop screenshot rule + // (plan §3.3) and it is enforced HERE, not by hoping the client + // omits the button. + GrantKind string } -// requestBrowserApproval raises the browser_action approval card and -// parks on its resolution. Returns (nil, nil) when the call may -// proceed (approve), (denyResult, nil) when the principal rejected or -// the wait timed out, or (nil, jrpcError) on an infrastructure fault. -// A session-option approve records the grant before returning. -func (s *Server) requestBrowserApproval(ctx context.Context, team, agentID, agentHandle, hostID, hostName, tool string, args json.RawMessage) (any, *jrpcError) { +// requestBridgeApproval raises the approval card and parks on its +// resolution. Returns (nil, nil) when the call may proceed (approve), +// (denyResult, nil) when the principal rejected or the wait timed out, +// or (nil, jrpcError) on an infrastructure fault. A session-option +// approve records the grant before returning — when the class has one. +func (s *Server) requestBridgeApproval(ctx context.Context, req bridgeApproval) (any, *jrpcError) { + team, agentID, agentHandle := req.Team, req.AgentID, req.AgentHandle + hostID, hostName, tool, args := req.HostID, req.HostName, req.Tool, req.Args displayAgent := agentHandle if displayAgent == "" { displayAgent = agentID @@ -227,6 +292,9 @@ func (s *Server) requestBrowserApproval(ctx context.Context, team, agentID, agen "tool": tool, "args": redactBrowserArgs(args), "agent_id": agentID, + // The card states the consent shape it can grant, so a client + // renders the right buttons instead of inferring them by kind. + "session_grant": req.GrantKind != "", }) id := NewID() @@ -240,15 +308,17 @@ func (s *Server) requestBrowserApproval(ctx context.Context, team, agentID, agen id, project_id, scope_kind, scope_id, kind, summary, severity, current_assignees_json, status, created_at, actor_kind, actor_handle, pending_payload_json - ) VALUES (?, NULL, 'team', ?, 'browser_action', + ) VALUES (?, NULL, 'team', ?, ?, ?, 'minor', '[]', 'open', ?, 'agent', NULLIF(?, ''), ?)`, - id, team, summary, now, agentHandle, string(payload), + id, team, req.AttentionKind, summary, now, agentHandle, string(payload), ); err != nil { return nil, &jrpcError{Code: -32000, Message: err.Error()} } - s.recordAudit(ctx, team, "browser_action.request", "attention", id, - "browser action awaiting approval: "+tool+" on "+hostName, + // The summary names the CLASS that raised it — a browser_action request + // must not audit as a "desktop action" (and vice versa). + s.recordAudit(ctx, team, req.AttentionKind+".request", "attention", id, + strings.ReplaceAll(req.AttentionKind, "_", " ")+" awaiting approval: "+tool+" on "+hostName, map[string]any{"tool": tool, "host_id": hostID, "agent_id": agentID}) pctx, cancel := context.WithTimeout(ctx, browserApprovalTimeout) @@ -276,10 +346,12 @@ func (s *Server) requestBrowserApproval(ctx context.Context, team, agentID, agen decision, _ := last["decision"].(string) if decision == "approve" { // option_id="session" upgrades this approve to a session grant: - // subsequent action calls from this agent to this desktop skip - // the card until revoked or the hub restarts. - if optionID, _ := last["option_id"].(string); optionID == "session" { - s.browserGrants.grant(team, hostID, agentID) + // subsequent action calls of the SAME KIND from this agent to + // this desktop skip the card until revoked or the hub restarts. + // A class with no grant namespace ignores the option entirely — + // per-call means per-call. + if optionID, _ := last["option_id"].(string); optionID == "session" && req.GrantKind != "" { + s.bridgeGrants.grant(req.GrantKind, team, hostID, agentID) } return nil, nil } @@ -293,13 +365,13 @@ func (s *Server) requestBrowserApproval(ctx context.Context, team, agentID, agen }), nil } -// routeBrowserInvoke packages one call as a browser.invoke tunnel -// envelope and parks on the desktop's response. The desktop answers +// routeTunnelInvoke packages one call as a tunnel envelope of the +// given kind and parks on the desktop's response. The desktop answers // Status 200 with BodyB64 = base64({"ok":true,"result":...} or // {"ok":false,"error":"..."}); a non-200 Status is a transport-level // failure and is treated as an error too. Failures come back as MCP // error results (agent-visible, retryable), not protocol faults. -func (s *Server) routeBrowserInvoke(ctx context.Context, agentID, agentHandle, hostID, tool string, args json.RawMessage) (any, *jrpcError) { +func (s *Server) routeTunnelInvoke(ctx context.Context, tunnelKind, agentID, agentHandle, hostID, tool string, args json.RawMessage) (any, *jrpcError) { if len(args) == 0 || string(args) == "null" { args = json.RawMessage(`{}`) } @@ -314,15 +386,15 @@ func (s *Server) routeBrowserInvoke(ctx context.Context, agentID, agentHandle, h defer cancel() resp, err := s.tunnel.enqueueAndWait(rctx, hostID, &tunnelRequest{ ReqID: NewID(), - Kind: browserTunnelKind, + Kind: tunnelKind, Payload: payload, }) if err != nil { - return mcpResultError("desktop did not answer browser.invoke: " + err.Error()), nil + return mcpResultError("desktop did not answer " + tunnelKind + ": " + err.Error()), nil } if resp.Status != 0 && resp.Status != http.StatusOK { return mcpResultError(fmt.Sprintf( - "desktop browser bridge returned transport status %d", resp.Status)), nil + "desktop bridge returned transport status %d", resp.Status)), nil } body, err := base64.StdEncoding.DecodeString(resp.BodyB64) if err != nil { @@ -350,25 +422,10 @@ func (s *Server) routeBrowserInvoke(ctx context.Context, agentID, agentHandle, h } // browserBridgeCapable reads the truthiness of capabilities_json's -// browser_bridge key. The desktop writes a JSON true; tolerate the -// other JSON-truthy shapes (non-empty string, non-zero number) so a -// hand-rolled capabilities row can't silently strand the feature. +// browser_bridge key. The generic reader (hostCapable, mcp_desktop_ui.go) +// does the work; this name stays as the browser class's spelling of it. func browserBridgeCapable(capsJSON string) bool { - var caps map[string]any - if err := json.Unmarshal([]byte(capsJSON), &caps); err != nil { - return false - } - switch v := caps["browser_bridge"].(type) { - case nil: - return false - case bool: - return v - case string: - return v != "" - case float64: - return v != 0 - } - return true + return hostCapable(capsJSON, "browser_bridge") } // redactBrowserArgs returns the tool args as a map with the VALUES of @@ -430,6 +487,6 @@ func (s *Server) handleBrowserBridgeRevoke(w http.ResponseWriter, r *http.Reques writeErr(w, http.StatusBadRequest, "malformed body: "+err.Error()) return } - s.browserGrants.revoke(team, host, in.AgentID) + s.bridgeGrants.revoke(team, host, in.AgentID) w.WriteHeader(http.StatusNoContent) } diff --git a/hub/internal/server/mcp_browser_bridge_test.go b/hub/internal/server/mcp_browser_bridge_test.go index ac024b47..ba4d8b8b 100644 --- a/hub/internal/server/mcp_browser_bridge_test.go +++ b/hub/internal/server/mcp_browser_bridge_test.go @@ -443,7 +443,7 @@ func TestBrowserInvoke_ActionRejectDenies(t *testing.T) { if n := len(fake.seen()); n != 0 { t.Errorf("rejected call must not route; fake desktop saw %d requests", n) } - if s.browserGrants.has(defaultTeamID, hostID, agentID) { + if s.bridgeGrants.has(browserGrantKind, defaultTeamID, hostID, agentID) { t.Error("reject must not record a session grant") } } @@ -460,8 +460,8 @@ func TestBrowserBridgeRevoke_ClearsGrant(t *testing.T) { }) // Stand up grants for two agents against this host. - s.browserGrants.grant(defaultTeamID, hostID, agentID) - s.browserGrants.grant(defaultTeamID, hostID, "other-agent") + s.bridgeGrants.grant(browserGrantKind, defaultTeamID, hostID, agentID) + s.bridgeGrants.grant(browserGrantKind, defaultTeamID, hostID, "other-agent") // Host-kind deputy tokens may not revoke — grants are principal // decisions. Mint a host token scoped to the team to prove it. @@ -475,7 +475,7 @@ func TestBrowserBridgeRevoke_ClearsGrant(t *testing.T) { map[string]any{"agent_id": agentID}); status != http.StatusForbidden { t.Fatalf("host token: status=%d body=%s; want 403", status, body) } - if !s.browserGrants.has(defaultTeamID, hostID, agentID) { + if !s.bridgeGrants.has(browserGrantKind, defaultTeamID, hostID, agentID) { t.Fatal("host-token call must not have revoked the grant") } @@ -485,10 +485,10 @@ func TestBrowserBridgeRevoke_ClearsGrant(t *testing.T) { map[string]any{"agent_id": agentID}); status != http.StatusNoContent { t.Fatalf("revoke: status=%d body=%s; want 204", status, body) } - if s.browserGrants.has(defaultTeamID, hostID, agentID) { + if s.bridgeGrants.has(browserGrantKind, defaultTeamID, hostID, agentID) { t.Error("grant survived revoke") } - if !s.browserGrants.has(defaultTeamID, hostID, "other-agent") { + if !s.bridgeGrants.has(browserGrantKind, defaultTeamID, hostID, "other-agent") { t.Error("targeted revoke cleared another agent's grant") } @@ -511,7 +511,7 @@ func TestBrowserBridgeRevoke_ClearsGrant(t *testing.T) { map[string]any{}); status != http.StatusNoContent { t.Fatalf("clear-all revoke: status=%d body=%s; want 204", status, body) } - if s.browserGrants.has(defaultTeamID, hostID, "other-agent") { + if s.bridgeGrants.has(browserGrantKind, defaultTeamID, hostID, "other-agent") { t.Error("clear-all revoke left a grant behind") } } diff --git a/hub/internal/server/mcp_desktop_ui.go b/hub/internal/server/mcp_desktop_ui.go new file mode 100644 index 00000000..d4e7c351 --- /dev/null +++ b/hub/internal/server/mcp_desktop_ui.go @@ -0,0 +1,206 @@ +// mcp_desktop_ui.go — D5 hub-relayed desktop UI context. +// +// The sibling of mcp_browser_bridge.go for the SECOND traffic class the +// desktop exposes: not the embedded browser tabs, but the desktop's own +// UI as an agent-addressable entity (ADR-062). An agent anywhere in the +// team asks a desktop what its user is looking at (`ui_get_focus`), or +// — per-call approved, always — for a frame of it (`ui_screenshot`); +// `desktop_ui_invoke` packages the call as a Kind="desktop.invoke" +// tunnel envelope and parks on the response, exactly as browser_invoke +// does with "browser.invoke" (ADR-028 D-1). +// +// Everything structural is shared with the browser class — the grant +// store, the approval parking, the tunnel routing — because a third +// class should cost ~zero (plan §3.6). What is NOT shared is the +// consent shape, and that is the point of this file: +// +// - the capability key is `desktop_ui`, registered only while the +// desktop's UI-context-sharing toggle is on. Bridge toggle + +// sharing toggle + Remote-driving opt-in, all three, before a +// remote agent can reach any of this (plan §3.6); +// - session grants are keyed BY KIND, so a browser-driving grant +// cannot silence a desktop card (the §3.6 review amendment); +// - `ui_screenshot` consults no grant at all. It raises a +// `desktop_action` card on every single call, and the card says so +// — screenshots are the one artifact with no standing consent +// (plan §3.3, ADR-062 D-4). +// +// The hub relays and never stores: no focus snapshot, no capture, ever +// lands in a table (ADR-062 D-7). What crosses is audited by the +// desktop's own ring + mirror, exactly as the browser class is. + +package server + +import ( + "context" + "database/sql" + "encoding/json" + "errors" + "fmt" + "strings" +) + +// desktopTunnelKind discriminates desktop-UI envelopes on the A2A +// reverse tunnel. The desktop's poll loop routes by Kind and refuses a +// tool that does not belong to the kind it arrived under. +const desktopTunnelKind = "desktop.invoke" + +// desktopGrantKind namespaces this class's session grants. Present for +// symmetry and for the class's future action tools; `ui_screenshot` +// itself never consults it (see desktopUIGrantable). +const desktopGrantKind = "desktop_ui" + +// desktopUICapability is the hosts.capabilities key the desktop sets +// while UI context sharing is on. +const desktopUICapability = "desktop_ui" + +// The desktop-UI tools, split by risk class. Must match the desktop's +// own classification (browserbridge.ts) — the desktop enforces its own +// copy, so a skewed edit here only changes which calls the hub gates, +// never which the desktop permits. +var desktopUIReadTools = []string{ + "ui_get_focus", +} + +var desktopUIActionTools = []string{ + "ui_screenshot", +} + +func desktopUIToolClass(tool string) string { + for _, n := range desktopUIReadTools { + if n == tool { + return "read" + } + } + for _, n := range desktopUIActionTools { + if n == tool { + return "action" + } + } + return "" +} + +func desktopUIToolNames() []string { + out := make([]string, 0, len(desktopUIReadTools)+len(desktopUIActionTools)) + out = append(out, desktopUIReadTools...) + out = append(out, desktopUIActionTools...) + return out +} + +// desktopUIGrantable reports whether an action tool of this class may +// EVER ride a session grant. `ui_screenshot` may not: a frame of +// everything the user can see is the most sensitive artifact the app +// can emit, so consent is per call, forever (plan §3.3). Encoded as a +// predicate rather than a comment because the grant path is shared +// with a class that does offer session grants. +func desktopUIGrantable(tool string) bool { + return tool != "ui_screenshot" +} + +// --------------------------------------------------------------------- +// desktop_ui_invoke — native MCP tool +// --------------------------------------------------------------------- + +type desktopUIInvokeArgs struct { + HostID string `json:"host_id"` + Tool string `json:"tool"` + Args json.RawMessage `json:"args"` +} + +func (s *Server) mcpDesktopUIInvoke(ctx context.Context, team, agentID string, raw json.RawMessage) (any, *jrpcError) { + var a desktopUIInvokeArgs + if err := json.Unmarshal(raw, &a); err != nil || a.HostID == "" || a.Tool == "" { + return nil, &jrpcError{Code: -32602, Message: "host_id and tool required"} + } + class := desktopUIToolClass(a.Tool) + if class == "" { + return nil, &jrpcError{Code: -32602, Message: fmt.Sprintf( + "unknown desktop UI tool %q — must be one of: %s", + a.Tool, strings.Join(desktopUIToolNames(), ", "))} + } + + // The desktop must be registered in the caller's team, online, and + // advertising desktop_ui — which it does only while UI context + // sharing is on, so "the toggle is off" reads to the agent as a + // missing capability rather than a mysterious refusal. + var hostName, hostStatus, capsJSON string + err := s.db.QueryRowContext(ctx, ` + SELECT name, status, COALESCE(capabilities_json, '') + FROM hosts WHERE team_id = ? AND id = ?`, team, a.HostID). + Scan(&hostName, &hostStatus, &capsJSON) + if errors.Is(err, sql.ErrNoRows) { + return mcpResultError(fmt.Sprintf( + "host %s not found in this team — call hosts_list and pick a host "+ + "whose capabilities show desktop_ui", a.HostID)), nil + } + if err != nil { + return nil, &jrpcError{Code: -32000, Message: err.Error()} + } + if hostStatus != "online" { + return mcpResultError(fmt.Sprintf( + "desktop %s (%s) is %s, not online — call hosts_list and pick an "+ + "online host whose capabilities show desktop_ui", + hostName, a.HostID, hostStatus)), nil + } + if !hostCapable(capsJSON, desktopUICapability) { + return mcpResultError(fmt.Sprintf( + "host %s (%s) does not advertise the desktop_ui capability — its user "+ + "has UI context sharing off (Settings → Assistant), or remote driving "+ + "is not enabled. Call hosts_list to see which desktops do", + hostName, a.HostID)), nil + } + + agentHandle, _ := s.lookupHandleByID(ctx, team, agentID) + + // Action tools need a human decision. A grantable one may ride a + // prior session grant for THIS class; ui_screenshot never does. + if class == "action" { + grantKind := "" + if desktopUIGrantable(a.Tool) { + grantKind = desktopGrantKind + } + if grantKind == "" || !s.bridgeGrants.has(grantKind, team, a.HostID, agentID) { + deny, jerr := s.requestBridgeApproval(ctx, bridgeApproval{ + Team: team, + AgentID: agentID, + AgentHandle: agentHandle, + HostID: a.HostID, + HostName: hostName, + Tool: a.Tool, + Args: a.Args, + AttentionKind: "desktop_action", + GrantKind: grantKind, + }) + if jerr != nil { + return nil, jerr + } + if deny != nil { + return deny, nil + } + } + } + return s.routeTunnelInvoke(ctx, desktopTunnelKind, agentID, agentHandle, a.HostID, a.Tool, a.Args) +} + +// hostCapable reads the truthiness of one capabilities_json key. The +// desktop writes a JSON true; the other JSON-truthy shapes are +// tolerated so a hand-rolled capabilities row can't silently strand a +// feature. (Generalized from browserBridgeCapable — same rule, two +// keys.) +func hostCapable(capsJSON, key string) bool { + var caps map[string]any + if err := json.Unmarshal([]byte(capsJSON), &caps); err != nil { + return false + } + switch v := caps[key].(type) { + case nil: + return false + case bool: + return v + case string: + return v != "" + case float64: + return v != 0 + } + return true +} diff --git a/hub/internal/server/mcp_desktop_ui_test.go b/hub/internal/server/mcp_desktop_ui_test.go new file mode 100644 index 00000000..7683fc13 --- /dev/null +++ b/hub/internal/server/mcp_desktop_ui_test.go @@ -0,0 +1,387 @@ +// Tests for D5's hub-relayed desktop UI context +// (docs/plans/desktop-ui-context-and-pointing.md §3.6). Mirrors +// mcp_browser_bridge_test.go — the machinery is shared, so these tests +// cover what is DIFFERENT about the second traffic class: its own +// capability key, its own envelope kind, per-kind session grants, and +// the rule that a screenshot never rides a grant at all. + +package server + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + "time" +) + +// seedDesktopUIHost inserts an online host row advertising desktop_ui — +// the shape the desktop writes while UI context sharing is on. +func seedDesktopUIHost(t *testing.T, s *Server, team, caps string) string { + t.Helper() + hostID := NewID() + if _, err := s.db.Exec(` + INSERT INTO hosts (id, team_id, name, status, capabilities_json, created_at) + VALUES (?, ?, ?, 'online', ?, ?)`, + hostID, team, "desktop-ui-"+hostID, caps, NowUTC()); err != nil { + t.Fatalf("seed desktop_ui host: %v", err) + } + return hostID +} + +// awaitAttentionRow waits for a card of `kind` other than `notID`. +// The exclusion matters: created_at has second granularity, so "newest +// row" cannot distinguish two cards raised inside the same second — +// and the whole point of the per-call rule is that the SECOND capture +// raises its OWN card. +func awaitAttentionRow(t *testing.T, s *Server, kind, notID string) string { + t.Helper() + for i := 0; i < 100; i++ { + var id string + if err := s.db.QueryRow( + `SELECT id FROM attention_items WHERE kind = ? AND id <> ? ORDER BY created_at DESC LIMIT 1`, + kind, notID).Scan(&id); err == nil && id != "" { + return id + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("%s attention row never appeared", kind) + return "" +} + +func attentionRowCount(t *testing.T, s *Server, kind string) int { + t.Helper() + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM attention_items WHERE kind = ?`, kind).Scan(&n); err != nil { + t.Fatalf("count %s rows: %v", kind, err) + } + return n +} + +func desktopInvokeResult(s *Server, agentID string, args map[string]any) chan struct { + out any + jerr *jrpcError +} { + raw, _ := json.Marshal(args) + done := make(chan struct { + out any + jerr *jrpcError + }, 1) + go func() { + o, e := s.mcpDesktopUIInvoke(context.Background(), defaultTeamID, agentID, raw) + done <- struct { + out any + jerr *jrpcError + }{out: o, jerr: e} + }() + return done +} + +// ── Validation + discovery ─────────────────────────────────────────── + +func TestDesktopUIInvoke_UnknownToolNamesTheRealOnes(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + + _, jerr := s.mcpDesktopUIInvoke(context.Background(), defaultTeamID, agentID, + json.RawMessage(`{"host_id":"h1","tool":"ui_read_the_vault"}`)) + if jerr == nil || jerr.Code != -32602 { + t.Fatalf("want -32602, got %+v", jerr) + } + for _, want := range []string{"ui_read_the_vault", "ui_get_focus", "ui_screenshot"} { + if !strings.Contains(jerr.Message, want) { + t.Errorf("error should mention %q; got %q", want, jerr.Message) + } + } + + // A BROWSER tool is not a desktop-UI tool. The classes are separate + // consent sentences, so crossing them is a protocol fault — not a + // quietly-routed call that would skip this class's gate. + _, jerr = s.mcpDesktopUIInvoke(context.Background(), defaultTeamID, agentID, + json.RawMessage(`{"host_id":"h1","tool":"browser_click"}`)) + if jerr == nil || jerr.Code != -32602 { + t.Fatalf("browser tool via desktop_ui_invoke: want -32602, got %+v", jerr) + } +} + +func TestBrowserInvoke_RejectsDesktopUITools(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + + // The mirror of the cross-class check above: a desktop-UI tool routed + // as browser.invoke would arrive without THIS class's card (the hub + // gates by class), so it must die at the catalog — the desktop's own + // kind re-check (tunnelclass.test.ts) is the second wall, not the + // only one. + for _, tool := range []string{"ui_screenshot", "ui_get_focus"} { + _, jerr := s.mcpBrowserInvoke(context.Background(), defaultTeamID, agentID, + json.RawMessage(`{"host_id":"h1","tool":"`+tool+`"}`)) + if jerr == nil || jerr.Code != -32602 { + t.Fatalf("%s via browser_invoke: want -32602, got %+v", tool, jerr) + } + } +} + +func TestDesktopUIInvoke_CapabilityIsTheSharingToggle(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + + // A desktop with the bridge on but UI sharing OFF advertises + // browser_bridge and not desktop_ui: the refusal must say so, since + // that is a toggle the user can flip rather than a bug. + hostID := seedDesktopUIHost(t, s, defaultTeamID, `{"browser_bridge":true}`) + args, _ := json.Marshal(map[string]any{"host_id": hostID, "tool": "ui_get_focus"}) + out, jerr := s.mcpDesktopUIInvoke(context.Background(), defaultTeamID, agentID, args) + if jerr != nil { + t.Fatalf("want an agent-visible error result, got jrpc %+v", jerr) + } + text := mcpResultTextBody(t, out) + if !strings.Contains(text, "desktop_ui") || !strings.Contains(text, "UI context sharing") { + t.Errorf("refusal should name the capability AND the toggle; got %q", text) + } + + // Offline desktops are refused before anything else. + offline := seedDesktopUIHost(t, s, defaultTeamID, `{"desktop_ui":true}`) + if _, err := s.db.Exec(`UPDATE hosts SET status='offline' WHERE id = ?`, offline); err != nil { + t.Fatal(err) + } + args, _ = json.Marshal(map[string]any{"host_id": offline, "tool": "ui_get_focus"}) + out, _ = s.mcpDesktopUIInvoke(context.Background(), defaultTeamID, agentID, args) + if !strings.Contains(mcpResultTextBody(t, out), "not online") { + t.Errorf("offline desktop should be refused: %q", mcpResultTextBody(t, out)) + } +} + +// ── Routing ────────────────────────────────────────────────────────── + +func TestDesktopUIInvoke_ReadRoutesOnDesktopInvokeWithNoCard(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + hostID := seedDesktopUIHost(t, s, defaultTeamID, `{"browser_bridge":true,"desktop_ui":true}`) + + fake := startFakeBrowserDesktop(t, s, hostID, func(env *tunnelRequest) (int, string) { + return okBrowserEnvelope(`{"content":[{"type":"text","text":"{\"surface\":\"read\"}"}]}`) + }) + + args, _ := json.Marshal(map[string]any{"host_id": hostID, "tool": "ui_get_focus"}) + out, jerr := s.mcpDesktopUIInvoke(context.Background(), defaultTeamID, agentID, args) + if jerr != nil { + t.Fatalf("read invoke: %+v", jerr) + } + if !strings.Contains(mcpResultTextBody(t, out), "surface") { + t.Errorf("unexpected result: %q", mcpResultTextBody(t, out)) + } + + seen := fake.seen() + if len(seen) != 1 { + t.Fatalf("want one envelope, got %d", len(seen)) + } + // The envelope kind is what the desktop routes on — and what lets it + // refuse a ui_* tool that arrived as browser.invoke. + if seen[0].Kind != desktopTunnelKind { + t.Errorf("kind = %q, want %q", seen[0].Kind, desktopTunnelKind) + } + var payload map[string]any + if err := json.Unmarshal(seen[0].Payload, &payload); err != nil { + t.Fatalf("payload: %v", err) + } + if payload["tool"] != "ui_get_focus" { + t.Errorf("payload tool = %v", payload["tool"]) + } + // Reads never raise a card. + if n := attentionRowCount(t, s, "desktop_action"); n != 0 { + t.Errorf("a read raised %d approval cards; want 0", n) + } +} + +// ── The screenshot rule: per call, always ──────────────────────────── + +func TestDesktopUIScreenshot_RaisesACardEveryTimeAndNeverGrants(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + hostID := seedDesktopUIHost(t, s, defaultTeamID, `{"desktop_ui":true}`) + startFakeBrowserDesktop(t, s, hostID, func(env *tunnelRequest) (int, string) { + return okBrowserEnvelope(`{"content":[{"type":"image","data":"aGk=","mimeType":"image/png"}]}`) + }) + + // First call: approve WITH the session option. The option is the + // strongest consent a principal can express, and for a screenshot it + // must still not create a standing grant (plan §3.3). + done := desktopInvokeResult(s, agentID, map[string]any{"host_id": hostID, "tool": "ui_screenshot"}) + id := awaitAttentionRow(t, s, "desktop_action", "") + resolveAttention(t, s, id, map[string]any{"decision": "approve", "option_id": "session", "by": "director"}) + if got := <-done; got.jerr != nil { + t.Fatalf("first capture: %+v", got.jerr) + } + if s.bridgeGrants.has(desktopGrantKind, defaultTeamID, hostID, agentID) { + t.Fatal("a screenshot approve must never record a session grant") + } + + // Second call: a NEW card, because there is no standing consent. + done = desktopInvokeResult(s, agentID, map[string]any{"host_id": hostID, "tool": "ui_screenshot"}) + id2 := awaitAttentionRow(t, s, "desktop_action", id) + if id2 == id { + t.Fatal("the second capture reused the first card — it must raise its own") + } + resolveAttention(t, s, id2, map[string]any{"decision": "approve", "by": "director"}) + if got := <-done; got.jerr != nil { + t.Fatalf("second capture: %+v", got.jerr) + } + if n := attentionRowCount(t, s, "desktop_action"); n != 2 { + t.Errorf("want 2 cards for 2 captures, got %d", n) + } +} + +func TestDesktopUIScreenshot_CardDescribesTheRequestAndDeclaresNoGrant(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + hostID := seedDesktopUIHost(t, s, defaultTeamID, `{"desktop_ui":true}`) + startFakeBrowserDesktop(t, s, hostID, func(env *tunnelRequest) (int, string) { + return okBrowserEnvelope(`{"content":[]}`) + }) + + done := desktopInvokeResult(s, agentID, map[string]any{"host_id": hostID, "tool": "ui_screenshot", "args": map[string]any{"tabId": 3}}) + id := awaitAttentionRow(t, s, "desktop_action", "") + + var payloadJSON, summary string + if err := s.db.QueryRow( + `SELECT COALESCE(pending_payload_json,''), summary FROM attention_items WHERE id = ?`, id). + Scan(&payloadJSON, &summary); err != nil { + t.Fatalf("read card: %v", err) + } + var payload map[string]any + if err := json.Unmarshal([]byte(payloadJSON), &payload); err != nil { + t.Fatalf("card payload: %v", err) + } + if payload["tool"] != "ui_screenshot" { + t.Errorf("card tool = %v", payload["tool"]) + } + // The card states the consent shape it can grant, so a client renders + // the right buttons instead of inferring them from the kind. + if grant, _ := payload["session_grant"].(bool); grant { + t.Error("a screenshot card must declare session_grant:false") + } + if !strings.Contains(summary, "ui_screenshot") { + t.Errorf("summary should name the tool: %q", summary) + } + resolveAttention(t, s, id, map[string]any{"decision": "reject", "by": "director"}) + <-done +} + +func TestDesktopUIScreenshot_RejectDeniesWithoutRouting(t *testing.T) { + s, _ := newTestServer(t) + _, agentID := seedChannelAndAgent(t, s, "", "") + hostID := seedDesktopUIHost(t, s, defaultTeamID, `{"desktop_ui":true}`) + fake := startFakeBrowserDesktop(t, s, hostID, func(env *tunnelRequest) (int, string) { + t.Error("a denied capture must never reach the desktop") + return http.StatusOK, `{"ok":true}` + }) + + done := desktopInvokeResult(s, agentID, map[string]any{"host_id": hostID, "tool": "ui_screenshot"}) + id := awaitAttentionRow(t, s, "desktop_action", "") + resolveAttention(t, s, id, map[string]any{"decision": "reject", "reason": "not now", "by": "director"}) + got := <-done + if got.jerr != nil { + t.Fatalf("deny should be an agent-visible result, got %+v", got.jerr) + } + if !strings.Contains(mcpResultTextBody(t, got.out), "not now") { + t.Errorf("the denial should carry the principal's reason: %q", mcpResultTextBody(t, got.out)) + } + if len(fake.seen()) != 0 { + t.Error("nothing should have been routed") + } +} + +// ── Per-kind grants (the §3.6 review amendment) ────────────────────── + +func TestBridgeGrants_AreKeyedByKind(t *testing.T) { + g := &bridgeGrantStore{} + g.grant(browserGrantKind, "team", "host", "agent") + + if !g.has(browserGrantKind, "team", "host", "agent") { + t.Fatal("the grant we just wrote is missing") + } + // THE point of the kind dimension: a browser-driving session grant + // must not silence a desktop card. W3's store had no kind, so a naive + // "one helper, two kind constants" generalization would have. + if g.has(desktopGrantKind, "team", "host", "agent") { + t.Fatal("a browser grant leaked into the desktop class") + } + // Neighbours stay untouched. + if g.has(browserGrantKind, "other-team", "host", "agent") || + g.has(browserGrantKind, "team", "other-host", "agent") || + g.has(browserGrantKind, "team", "host", "other-agent") { + t.Fatal("grant key is not scoped to (kind, team, host, agent)") + } +} + +func TestBridgeGrants_RevokeIsKindBlind(t *testing.T) { + g := &bridgeGrantStore{} + g.grant(browserGrantKind, "team", "host", "agent") + g.grant(desktopGrantKind, "team", "host", "agent") + g.grant(browserGrantKind, "team", "host", "other-agent") + + // Granting is kind-scoped; REVOKING is not. "Revoke" in Settings → + // Remote driving means this agent no longer touches this desktop — + // an agent that kept a standing browser grant afterwards would make + // that pill a lie. + g.revoke("team", "host", "agent") + if g.has(browserGrantKind, "team", "host", "agent") || g.has(desktopGrantKind, "team", "host", "agent") { + t.Fatal("revoke must clear every kind for that agent") + } + if !g.has(browserGrantKind, "team", "host", "other-agent") { + t.Fatal("revoke hit an agent it was not aimed at") + } + + // An empty agent id clears the whole (team, host) pair, every kind. + g.grant(desktopGrantKind, "team", "host", "agent") + g.grant(browserGrantKind, "team", "other-host", "keeper") + g.revoke("team", "host", "") + if g.has(desktopGrantKind, "team", "host", "agent") || g.has(browserGrantKind, "team", "host", "other-agent") { + t.Fatal("host-wide revoke left grants behind") + } + if !g.has(browserGrantKind, "team", "other-host", "keeper") { + t.Fatal("host-wide revoke crossed into another host") + } +} + +// ── Capability reading ─────────────────────────────────────────────── + +func TestHostCapable_ReadsOneKeyWithoutGuessing(t *testing.T) { + cases := []struct { + caps string + key string + want bool + }{ + {`{"desktop_ui":true}`, "desktop_ui", true}, + {`{"desktop_ui":false}`, "desktop_ui", false}, + {`{"browser_bridge":true}`, "desktop_ui", false}, + {`{}`, "desktop_ui", false}, + {``, "desktop_ui", false}, + {`not json`, "desktop_ui", false}, + // Hand-rolled rows shouldn't silently strand the feature. + {`{"desktop_ui":"yes"}`, "desktop_ui", true}, + {`{"desktop_ui":1}`, "desktop_ui", true}, + {`{"desktop_ui":0}`, "desktop_ui", false}, + {`{"desktop_ui":""}`, "desktop_ui", false}, + } + for _, c := range cases { + if got := hostCapable(c.caps, c.key); got != c.want { + t.Errorf("hostCapable(%q, %q) = %v, want %v", c.caps, c.key, got, c.want) + } + } + // The browser class reads through the same function. + if !browserBridgeCapable(`{"browser_bridge":true}`) || browserBridgeCapable(`{"desktop_ui":true}`) { + t.Error("browserBridgeCapable must read its own key only") + } +} + +func TestDesktopUIGrantable_ScreenshotIsTheExemption(t *testing.T) { + if desktopUIGrantable("ui_screenshot") { + t.Error("ui_screenshot must never be grantable (plan §3.3)") + } + if !desktopUIGrantable("ui_get_focus") { + t.Error("the predicate should not refuse everything by default") + } +} diff --git a/hub/internal/server/native_tools.go b/hub/internal/server/native_tools.go index f78aebdc..2fd29aff 100644 --- a/hub/internal/server/native_tools.go +++ b/hub/internal/server/native_tools.go @@ -783,6 +783,38 @@ func buildNativeTools() []nativeTool { Tier: TierSignificant, WorkerEligible: true, Handler: teamAgentArgs((*Server).mcpBrowserInvoke), }, + { + Name: "desktop_ui_invoke", + Short: "Ask an online desktop what its user is looking at, or (approved per call) for a screenshot.", + Description: "Invoke a desktop-UI tool on an online TermiPod desktop that registered the " + + "desktop_ui capability; the call rides the hub's A2A reverse tunnel, so the desktop " + + "can be on a different host than you. Discover desktops via hosts_list (hosts whose " + + "capabilities show desktop_ui — a desktop advertises it only while its user has UI " + + "context sharing on). ui_get_focus returns a compact JSON snapshot of the workbench " + + "surface(s) on screen plus focus state (open tab, focused agent, Inspect file + " + + "selection, terminal pane): ids, paths and fragment-stripped URLs only, never message " + + "bodies, vault material or settings values — it is the cheap, precise way to ground " + + "\"this\"/\"here\", and the ids join straight into your other hub tools. " + + "ui_screenshot returns a PNG of the desktop window (or one embedded tab) and raises " + + "an approval card the user must accept ON EVERY CALL — there is no session grant, and " + + "a capture is refused outright while a sensitive surface is on screen. Prefer " + + "ui_get_focus; reach for pixels only when the question is visual. " + + "Required: host_id, tool. Optional: args (object, default {}).", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "host_id": map[string]any{"type": "string"}, + "tool": map[string]any{ + "type": "string", + "enum": desktopUIToolNames(), + }, + "args": map[string]any{"type": "object", "default": map[string]any{}}, + }, + "required": []string{"host_id", "tool"}, + }, + Tier: TierSignificant, WorkerEligible: true, + Handler: teamAgentArgs((*Server).mcpDesktopUIInvoke), + }, { Name: "agents_fanout", Short: "Spawn N workers in one orchestrator-worker fan-out.", @@ -929,7 +961,8 @@ var nativeToolMeta = map[string]struct { "pause_self": {false, []string{"shutdown_self", "agents_terminate"}}, "shutdown_self": {false, []string{"pause_self"}}, "permission_prompt": {false, []string{"request_approval"}}, - "browser_invoke": {false, []string{"hosts_list", "request_approval"}}, + "browser_invoke": {false, []string{"hosts_list", "desktop_ui_invoke", "request_approval"}}, + "desktop_ui_invoke": {false, []string{"hosts_list", "browser_invoke", "request_approval"}}, "agents_fanout": {false, []string{"agents_gather", "agents_spawn"}}, "agents_gather": {true, []string{"agents_fanout", "reports_post"}}, "reports_post": {false, []string{"tasks_complete", "agents_gather"}}, diff --git a/hub/internal/server/server.go b/hub/internal/server/server.go index ca90120e..05915072 100644 --- a/hub/internal/server/server.go +++ b/hub/internal/server/server.go @@ -83,11 +83,12 @@ type Server struct { policy *policyStore escalator *Escalator tunnel *TunnelManager - // browserGrants is the W3 browser-bridge session-grant cache - // (principal approved an action tool with option_id="session"). + // bridgeGrants is the session-grant cache for BOTH tunnel classes + // (browser.invoke and desktop.invoke — grants are keyed by kind; + // principal approved an action tool with option_id="session"). // In-memory, no expiry — hub restart clears, same posture as // TunnelManager. See mcp_browser_bridge.go. - browserGrants *browserGrantStore + bridgeGrants *bridgeGrantStore agentFamilies *agentfamilies.Registry // pricing serves the session-cost chip (ADR-036 D8 chip 2). One // loader per server; thread-safe; mtime-hot-reloaded so an @@ -227,7 +228,7 @@ func New(cfg Config) (*Server, error) { s.sched = NewScheduler(s, cfg.Logger) s.escalator = NewEscalator(s, cfg.Logger, 0) s.tunnel = newTunnelManager() - s.browserGrants = &browserGrantStore{} + s.bridgeGrants = &bridgeGrantStore{} // Pricing loader (ADR-036 D10). Warner closure adapts the // loader's action/summary/meta call into the server's recordAudit // row — operator-visible parse errors land in audit_events under