From ce04d972904e98ef2ae3d3d21abff96598bad882 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:19:28 -0700 Subject: [PATCH] fix(computer-use): address modifier safety review --- .../computer-use-modifier-safety.test.mjs | 42 +++++++++++-------- .../computer-use-skill-guidance.test.mjs | 12 ++++-- .../Sources/OrcaComputerUseMacOS/main.swift | 2 +- .../AgentEntrypointSourceSafetyTests.swift | 9 +++- native/computer-use-windows/runtime.ps1 | 1 + src/cli/handlers/computer-action-flags.ts | 3 +- .../computer-action-validation.test.ts | 14 +++++++ 7 files changed, 59 insertions(+), 24 deletions(-) diff --git a/config/scripts/computer-use-modifier-safety.test.mjs b/config/scripts/computer-use-modifier-safety.test.mjs index 79c739a7ec3..f39015796a9 100644 --- a/config/scripts/computer-use-modifier-safety.test.mjs +++ b/config/scripts/computer-use-modifier-safety.test.mjs @@ -8,36 +8,47 @@ function source(path) { return readFileSync(join(projectDir, path), 'utf8') } +function sourceBetween(contents, startMarker, endMarker) { + const start = contents.indexOf(startMarker) + const end = contents.indexOf(endMarker, start + startMarker.length) + if (start < 0 || end < 0) { + throw new Error(`Missing source boundary: ${startMarker} → ${endMarker}`) + } + return contents.slice(start, end) +} + describe('computer-use modifier safety', () => { it('uses mouse-event flags instead of held modifier keys on macOS', () => { const macOS = source('native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift') - const clickInput = macOS.slice( - macOS.indexOf('static func click('), - macOS.indexOf('static func scroll(') - ) - const mouseInput = macOS.slice( - macOS.indexOf('private static func mouse('), - macOS.indexOf('private static func keyEvent(') + const clickInput = sourceBetween(macOS, 'static func click(', 'static func scroll(') + const mouseInput = sourceBetween( + macOS, + 'private static func mouse(', + 'private static func keyEvent(' ) expect(mouseInput).toContain('event.flags = flags') + expect(clickInput.match(/flags: flags/gu)).toHaveLength(3) expect(clickInput).not.toContain('down: true') }) it('submits each modified Windows click in a closed, timed SendInput batch', () => { const windows = source('native/computer-use-windows/runtime.ps1') - const modifiedClick = windows.slice( - windows.indexOf('public static void SendModifiedClick'), - windows.indexOf('private static INPUT KeyboardInput') + const modifiedClick = sourceBetween( + windows, + 'public static void SendModifiedClick', + 'private static INPUT KeyboardInput' ) - const mouseClick = windows.slice( - windows.indexOf('function Send-OrcaMouseClick'), - windows.indexOf('function Send-OrcaDrag') + const mouseClick = sourceBetween( + windows, + 'function Send-OrcaMouseClick', + 'function Send-OrcaDrag' ) expect(modifiedClick).toContain('SendInput((uint)values.Length, values') expect(modifiedClick).toContain('SendInput((uint)releaseValues.Length, releaseValues') expect(modifiedClick).toContain('if (sent != (uint)values.Length)') + expect(modifiedClick).toContain('releases.Add(MouseInput(mouseInput, mouseUp))') expect(modifiedClick).not.toContain('int count') expect(mouseClick).toMatch( /for \(\$i = 0; \$i -lt \$clickCount; \$i\+\+\) \{\s+\[OrcaDesktopWin32\]::SendModifiedClick\(/ @@ -48,10 +59,7 @@ describe('computer-use modifier safety', () => { it('keeps Linux modifier release in the xdotool sequence and a fallback', () => { const linux = source('native/computer-use-linux/runtime.py') - const modifiedClick = linux.slice( - linux.indexOf('def modified_click_at('), - linux.indexOf('def scroll_at(') - ) + const modifiedClick = sourceBetween(linux, 'def modified_click_at(', 'def scroll_at(') expect(modifiedClick).toContain('command.extend(["keyup", modifier])') expect(modifiedClick).toContain('is_wayland') diff --git a/config/scripts/computer-use-skill-guidance.test.mjs b/config/scripts/computer-use-skill-guidance.test.mjs index 3c83be3fb3e..764ff234e19 100644 --- a/config/scripts/computer-use-skill-guidance.test.mjs +++ b/config/scripts/computer-use-skill-guidance.test.mjs @@ -1,6 +1,7 @@ import { readFileSync } from 'node:fs' import { join, resolve } from 'node:path' import { describe, expect, it } from 'vitest' +import { BUNDLED_SKILL_GUIDES } from '../../src/cli/bundled-skill-guides' const projectDir = resolve(import.meta.dirname, '../..') // Why: computer-use now ships a hybrid discovery stub, so its version-sensitive command @@ -8,6 +9,7 @@ const projectDir = resolve(import.meta.dirname, '../..') // installable stub projection is checked separately below. const guidePath = join(projectDir, 'skill-guides', 'computer-use.md') const stubPath = join(projectDir, 'skills', 'computer-use', 'SKILL.md') +const bundledGuide = BUNDLED_SKILL_GUIDES.find((guide) => guide.name === 'computer-use')?.markdown describe('computer-use skill guidance', () => { it('keeps web-app targeting on the computer-use surface', () => { @@ -45,11 +47,13 @@ describe('computer-use skill guidance', () => { expect(skill).not.toContain('`result.elements`') }) - it('requires atomic modifier-click actions', () => { - const skill = readFileSync(guidePath, 'utf8') + it('requires atomic modifier-click actions in the source and bundled guide', () => { + expect(bundledGuide).toBeDefined() - expect(skill).toContain('click --modifiers ') - expect(skill).toContain('Never synthesize separate modifier-down and modifier-up commands') + for (const skill of [readFileSync(guidePath, 'utf8'), bundledGuide]) { + expect(skill).toContain('click --modifiers ') + expect(skill).toContain('Never synthesize separate modifier-down and modifier-up commands') + } }) }) diff --git a/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift b/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift index 374aa4690db..87100b2faec 100644 --- a/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift +++ b/native/computer-use-macos/Sources/OrcaComputerUseMacOS/main.swift @@ -2358,8 +2358,8 @@ private enum Input { var pressedModifiers: [KeyModifier] = [] defer { for modifier in pressedModifiers.reversed() { - try? keyEvent(modifier.keyCode, down: false, flags: flags, pid: pid) flags.remove(modifier.flag) + try? keyEvent(modifier.keyCode, down: false, flags: flags, pid: pid) } } for modifier in parsed.modifiers { diff --git a/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift b/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift index 34f41454822..389b83bac6b 100644 --- a/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift +++ b/native/computer-use-macos/Tests/OrcaComputerUseMacOSTests/AgentEntrypointSourceSafetyTests.swift @@ -23,7 +23,14 @@ final class AgentEntrypointSourceSafetyTests: XCTestCase { let source = try agentEntrypointSource() XCTAssertTrue(source.contains("var pressedModifiers: [KeyModifier] = []")) - XCTAssertTrue(source.contains("defer {\n for modifier in pressedModifiers.reversed()")) + XCTAssertTrue(source.contains( + """ + defer { + for modifier in pressedModifiers.reversed() { + flags.remove(modifier.flag) + try? keyEvent(modifier.keyCode, down: false, flags: flags, pid: pid) + """ + )) XCTAssertTrue(source.contains("event.flags = flags\n event.postToPid(pid)")) } diff --git a/native/computer-use-windows/runtime.ps1 b/native/computer-use-windows/runtime.ps1 index 96ab0e9b2aa..39ab77e1704 100644 --- a/native/computer-use-windows/runtime.ps1 +++ b/native/computer-use-windows/runtime.ps1 @@ -111,6 +111,7 @@ public static class OrcaDesktopWin32 { var sent = SendInput((uint)values.Length, values, Marshal.SizeOf(typeof(INPUT))); if (sent != (uint)values.Length) { var releases = new List(); + releases.Add(MouseInput(mouseInput, mouseUp)); for (var index = modifiers.Length - 1; index >= 0; index--) { releases.Add(KeyboardInput(keyboardInput, modifiers[index], keyUp)); } diff --git a/src/cli/handlers/computer-action-flags.ts b/src/cli/handlers/computer-action-flags.ts index e751348b0d6..72d3d213f1f 100644 --- a/src/cli/handlers/computer-action-flags.ts +++ b/src/cli/handlers/computer-action-flags.ts @@ -63,7 +63,8 @@ export function getComputerClickActionFlags(flags: Map mouseButton?: string modifiers?: string } { - const modifiers = getOptionalStringFlag(flags, 'modifiers') + const rawModifiers = flags.get('modifiers') + const modifiers = typeof rawModifiers === 'string' ? rawModifiers : undefined const result = { elementIndex: getOptionalNonNegativeIntegerFlag(flags, 'element-index'), x: getOptionalNumberFlag(flags, 'x'), diff --git a/src/cli/handlers/computer-action-validation.test.ts b/src/cli/handlers/computer-action-validation.test.ts index c420e3f4a50..2ff677a7920 100644 --- a/src/cli/handlers/computer-action-validation.test.ts +++ b/src/cli/handlers/computer-action-validation.test.ts @@ -312,6 +312,20 @@ describe('orca computer action CLI validation', () => { vi.mocked(console.error).mockClear() process.exitCode = undefined + await main( + ['computer', 'click', '--app', 'Finder', '--element-index', '1', '--modifiers', ''], + '/tmp/repo/src' + ) + + expect(callMock).not.toHaveBeenCalled() + expect(vi.mocked(console.error).mock.calls[0][0]).toContain( + 'Click modifiers accept modifier keys only' + ) + expect(process.exitCode).toBe(1) + + vi.mocked(console.error).mockClear() + process.exitCode = undefined + await main( ['computer', 'scroll', '--app', 'Finder', '--element-index', '1', '--direction', 'diagonal'], '/tmp/repo/src'