refactor(computer-use): enforce semantic-only actions and enable guarded keyboard input - #4497
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The coordinate half of this is right and clean. The mismatch is real: main lets the model plan left_click, scroll, zoom and the rest while the only shipping guard (allowCompatibilityInputDispatch, set to false by the sole caller) refuses them, and maka-cu advertises pointActions: []. maka-agent/maka-cu#6 is merged and fails closed on dispatch.point, so merge order does not matter. The derived presentation point never reaches an executor (its two consumers are the overlay context and the hook), pointer_mutation has zero residue anywhere, the skill catalog is regenerated, and the approval scope keys are observation-scoped so nothing persisted is orphaned. Builds and the three CU suites pass locally.
One thing I need you to decide before this merges. On main the same option gated dispatchKey too (maka-cu-backend.ts:2116), and its comment says every type, key and press_key ends there in every shipping configuration. This PR removes both guards. Keyboard dispatch is now on by default, gated only by the executor's keyActions, which maka-cu advertises as ["type", "key"]. The test diff shows it: every keyboard case on main had to pass allowCompatibilityInputDispatch: true to reach dispatch.key, and here they reach it with no option at all. The body says "preserve keyboard actions" and "the option that could enable synthetic pointer input"; neither is what the code does. The remaining guards (focusToken, physicalInputFailure, keyboard_mutation approval) are all still there, so this is not a hole, but it is a feature switched on inside a deletion, and the skill text quietly moved from "do not plan around press_key, type, key" to "keyboard actions remain capability-dependent". If turning keyboard on is intended, say so in the title or body, add one test asserting that the default configuration reaches dispatch.key, and drop the now-dead allowCompatibilityInputDispatch: false in scripts/computer-use/real-ax-harness.mjs, whose stated purpose was "no synthetic input". If it is not intended, keep the keyboard guard (hard-coded off is fine, no option needed).
Deletion residue worth taking in the same round, since it is exactly the drift the comment you removed from core/computer-use.ts warned about: scripts/computer-use/e2e-scenarios.mjs and report-sanitize.mjs still list left_click, mouse_move, scroll and friends in their action vocabularies; MODEL_CALL_PLAIN_VALUES keeps a scroll entry that can never match, and MODEL_CALL_GEOMETRY_ARGS keeps coordinate, start_coordinate, region with a comment about drag origins and zoom rectangles (both callers are on the live path, so if this is for replay say so); pip-feed.ts in desktop still reads resolvedScreenPoint, a field that never had a producer and is now gone from CuRunResult.
Smaller: the deleted treats a global-pointer path as a compromised session case was the only backend-level check of the executor's self-reported path, and keyboard now really does go through cg_event_*, so an equivalent case on dispatch.key would be welcome; computer-action-label.ts keeping the coordinate cases for history rendering is reasonable but reads like a miss without a sentence; coordinate-background as a tier name now labels screenshot, wait and keyboard results, and the body should say why the tier vocabulary is not touched here.
Evidence boundary: static read of 2640f986 against main cdb29399; core, runtime and computer-use built and CU suites green (266, 35, 112); maka-cu read from its HOST_PROTOCOL.md and PR #6, not its Swift source; the pinned maka-cu build in bundled-tools.json not checked against #6.
AI-assisted review: drafted with Maka; I verified the keyboard guard removal, the presentation-point consumers and the vocabulary residue myself.
简体中文
坐标这一半删得对也删得干净，maka-cu#6 已合并且 fail-closed，合并顺序无约束。合并前需要你定一件事：main 上同一个选项也门控着 dispatchKey，本 PR 把两处守卫都删了，键盘合成输入在默认配置下被打开，正文却写的是 preserve。是有意的就在标题或正文说明，加一条默认配置下到达 dispatch.key 的测试，删掉 harness 里已失效的参数；不是有意的就把键盘守卫留下。另外 scripts/computer-use 的动作词表、core 里的坐标投影残留、desktop 的 resolvedScreenPoint 建议同轮清掉。
| @@ -2113,9 +2067,6 @@ export function createMakaCuBackend(opts: MakaCuBackendOptions): MakaCuBackend { | |||
| */ | |||
| target?: { token: string; digest: string }, | |||
| ): Promise<CuRunResult> { | |||
There was a problem hiding this comment.
On main this call sat behind allowCompatibilityInputDispatch, off in every shipping configuration. With that guard gone, keyboard synthesis is on by default, gated only by keyActions, which maka-cu advertises as ["type", "key"]. If that is intended, the body should say enable, not preserve, and one test should assert the default configuration reaches here.
There was a problem hiding this comment.
Confirmed: enabling bound keyboard dispatch by default is intentional. This revision makes that contract explicit in the PR body and adds a default-configuration test that reaches dispatch.key. Keyboard mutations still require an observation-bound window/control or verified focus, pass the recent-physical-input guard, use an executor-advertised key capability, and remain subject to keyboard_mutation approval. I also added a backend test that rejects an invalid executor-reported path for dispatch.key.
|
Addressed the review on head
Local validation is listed in the updated PR body. CI is running on the new revision. @Astro-Han, please re-review when it is ready. |
…ction-space-followup
|
Merged the current Current validation: Core 770/770, Runtime 3152/3152, Computer Use 113/113, Desktop full dependency/application build, Biome on all affected files, and @Astro-Han, could you please re-review head |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The coordinate-action removal is internally consistent: the coordinate variants, compatibility gate, provider vocabulary, and presentation-only paths were traced across Core, Runtime, Desktop, Computer Use, scripts, and the native protocol boundary. I found one remaining model/executor contract mismatch.
Validation on exact head 19f764c905bc71f253a3a1717d264ad964de17f5: clean install, build:test, full typecheck, Core 770/770, Runtime 3139 passed / 13 skipped, Computer Use 113/113, changed-file Biome, ASF headers, and git diff --check. A conflict-free synthetic merge onto current origin/main 726fb809edd32ed2cc90688b5f6fbccbd78786a1 passed the same build/typecheck and all three suites. The hosted test check is still running.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| 'wait', | ||
| 'zoom', | ||
| ] as const; | ||
| export const CU_ACTION_TYPES = ['screenshot', 'type', 'key', 'hold_key', 'wait'] as const; |
There was a problem hiding this comment.
[P2] Remove or implement hold_key before advertising this action space
This catalog feeds the model-facing schema, and the Runtime path accepts hold_key, adapts it to a CuAction, then calls deps.backend.run. The only production backend handles only wait, screenshot, type, and key (packages/computer-use/src/maka-cu-backend.ts:2306-2320); every other action reaches the unconditional unsupported_action return at lines 2322-2325. Therefore every valid model-generated hold_key call is guaranteed to fail. This revision also removes the previous tool-description warning that hold_key has no maka.cu/2 execution path while retaining it in real-run/report vocabularies, so the advertised semantic-only contract still contains an unusable verb. Please either remove hold_key from the model-facing catalogs/schemas/reporting, or add protocol/backend support plus an end-to-end backend test.
|
Follow-up on the model/executor contract review: head Negative coverage now verifies that both schemas and the codec reject Fresh local validation after merging current @Astro-Han, could you please re-review this new head after hosted CI completes? An independent human approval is still required before merge. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The previous hold_key model/executor mismatch is fixed on this head. The action is no longer advertised by the Core catalog (packages/core/src/computer-use.ts:170), is absent from Runtime's strict accepted union (packages/runtime/src/computer-use-codec.ts:72-289), and is rejected by both model-facing schemas and the adapter (packages/runtime/src/__tests__/computer-use-wire-schema.test.ts:129-138, packages/runtime/src/__tests__/computer-use-codec-adapt.test.ts:85-95). The remaining UI label is explicitly retained only for rendering persisted historical transcripts (packages/ui/src/tool-activity/computer-action-label.ts:229-237). I found no remaining P0-P3 issue in the full 33-file change.
Validation on exact head 64e2e6b1020c3725733590365e4cb288576bf769: clean Node 22 install, build:test, full typecheck, Core 774/774, Runtime 3139 passed / 13 skipped, Computer Use 113/113, changed-file Biome, ASF headers, and git diff --check. A conflict-free synthetic merge 651ccc50e5ad4b87667cd0f43acec2f757f6feaf onto current origin/main 079f784a59b2717b46460458eca754f7fe9c5386 passed the same build, typecheck, and all three suites. The hosted test check also completed successfully. Native macOS execution was not available on this Linux review host.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
No blockers. One defect to fix before merge, and one part of the diff I would rather see land on its own.
The direction holds up. Coordinate actions were model-facing while allowCompatibilityInputDispatch was hard off in every shipping wiring, so models could plan pointer actions no executor would run. This is a net deletion that leaves the strict schemas as the single authority, and the negative coverage pins it through the exports the tool actually uses rather than the schema module.
I checked the two things that could have forced a migration, and both come back clean:
hold_key is genuinely unreachable. It falls to pointActionFor, which has no case for it, so run() answers unsupported_action before any dispatch gate. Deleting it is cleanup, not capability removal.
Removing 'pointer_mutation' from COMPUTER_USE_APPROVAL_CLASSES strands no stored records. That constant is a strict decode gate (interaction-permission-review.ts:1007, and oneOf at :1447 throws), so it was worth being sure. It is safe because no persisted request carries an approvalClass at all: the only constructor that produces one, projectInteractionPermissionRequest at interaction.ts:414, has no production caller, and the sole writer into core_interaction_requests is HostInteractionCoordinator, which admits exactly three request shapes (interaction-coordinator.ts:29-31). There is no permission branch, so the table holds no row the narrowed union could reject.
[P2] The old skill hash was not carried forward
Editing the Computer Use skill body moves contentSha256 from sha256:64aa2ef2… to sha256:ef548414…, but legacyContentSha256 still holds only the two older hashes.
isTrustedBundledLock (skills-governance.ts:246) builds the trusted set from both fields. A user who installed before this change is pinned at 64aa2ef2…, now in neither, so :139 returns metadataError('unsupported_schema', 'Skill lock source is not trusted in this Maka version.') and governance reports a skill they never touched as failing validation.
I checked how far it goes: the metadata_error branches in skill-catalog-repository.ts all sit on managed-source paths, so a bundled skill is not disabled by it. The damage is the reported state, not the capability.
The list is hand-maintained at scripts/gen-bundled-skill-catalog.mjs:37, and computer-use already carries two entries there from earlier body edits, so appending sha256:64aa2ef2d608e15792cc04eff7204731671b6b18818964ba95c65f53c694db62 and regenerating should be the whole fix.
[P2, and I would rather it were its own PR] Keyboard synthesis turns on here
dispatchKey used to open with if (opts.allowCompatibilityInputDispatch !== true) return compatibilityInputBlocked(...), and that flag is false in every shipping wiring, so type, key and press_key were unreachable in production. This removes the guard, the option and the pass-through.
I checked the shipped executor rather than its current source: bundled-tools.json pins maka-cu at 4a9787d2, whose HostProtocolWire.swift:592 has keyActions: ["type", "key"]. So keystroke synthesis goes live on merge.
It is not a hole. All four constraints exist on that same path: bound to one observation, verified focus with focusPolicy defaulting to require, the physical-input fence at maka-cu-backend.ts:2081, and keyboard_mutation approval. The worst reachable outcome needs the user to approve it themselves.
What I am raising is scope. Maka loses its own switch, the only remaining decision being the executor's keyActions advertisement in another repository. The fence has never actually run in production, since keyboard and coordinate dispatch were both hard off and element dispatch deliberately does not use it, so this is its first execution. And the title still says semantic-only, which is the opposite direction; the body admits it in one bullet, but anyone reading the diff against the title will misread it.
Coordinate removal stands entirely on its own and its acceptance does not change either way. Your call.
The companion contract is half shipped
maka-cu#6 merged on 09-02 and sets pointActions: [], but bundled-tools.json still pins 4a9787d2 from 08-10 where it is non-empty, and this does not move the pin. Not a hole, Maka stops sending dispatch.point so the advertisement stops mattering and both orders are safe. But the description reads as though both halves are in place.
[P2] Coverage the newly live path is missing
Nothing asserts type, key or press_key reach an approval prompt through tool.permissionArgs; every such assertion uses click_element or observe. The classifier is unit-tested to return keyboard_mutation but is not pinned to the tool path, and the product answer to "can the model type" just flipped. The !target && !snapshot.focused branch at maka-cu-backend.ts:2072 has no test either, and it is now the first thing a model hits sending key at an unfocused window.
The flip side is a real improvement: the focus and fence tests had to pass allowCompatibilityInputDispatch: true to reach dispatch.key, so they were testing a configuration that never shipped. Now they test the shipping one.
[P2] Two obligations left with the deleted model-loop case
That a tool refusal's text reaches the model's next prompt, and that a turn still reaches complete after a failed tool call. Both still hold. unsupported_action is still produced at HEAD, and nothing now asserts a refusal reaches the prompt (the tool-level tests assert result.text, one layer below). The surviving model-loop case is the all-success path. Rewriting the deleted case with a semantic action, driven by a schema or backend refusal, keeps both.
Separately, collapsing for (const semantic of [false, true]) drops the only coverage of the backend.run() branch's rule that clearSession cannot mask a delivered mutation outcome, and that branch still ships for type and key.
[P3] Residue
CuRegion, CU_SCROLL_DIRECTIONS/CuScrollDirection, CuPoint and stringsIn have no consumers left. The comment at computer-use.ts:688 still refers to the POINTER_ACTIONS this PR deleted, which is the drift the deleted comment warned about. computer-use-overlay-hook.ts:154's 'drag' branch is unreachable and :27 still declares 'move' | 'drag'. SAFE_DISPATCH_TOOLS still lists drag and zoom whose only producer is gone, though scroll should stay. maka-cu-service.ts:969 parses capabilities.pointActions with no reader left, defensible as protocol fidelity but worth a comment saying so. Since the test job does not cover them, please run npm run format and npm run lint after.
Next step
The skill hash is the one to fix, a one-line append plus a regenerate. Then the keyboard scope decision, since it is the only remaining one that changes the diff's shape; the model-loop rewrite and residue can follow in the same round.
The skill-hash check is to install on a build without this change, upgrade to this branch, and confirm governance still reports it as unmodified. If the keyboard stays, four manual checks have no automated substitute: type and key raise a keyboard_mutation approval, refusing produces no keystroke, an approved keystroke lands only in the verified focused control, and acting while the user types returns user_intervened.
Evidence boundary: read 64e2e6b10 against merge base 15e4b6b9e. I verified the decode gate and the absence of any production writer for permission requests, hold_key's unreachability, the four keyboard constraints, the pinned maka-cu build's capabilities, and the trusted-hash set. I ran no test suites, did not exercise Computer Use on a real machine, and did not reproduce the governance state on an upgraded install.
AI-assisted review: drafted with Maka.
hqhq1025
changed the title
|
Addressed the complete review round on head Changes made:
The title and body now state that keyboard synthesis is intentionally enabled and document all four constraints: observation/focus binding, executor capability, recent physical-input fencing, and Validation on this head: full @Astro-Han, please re-review head |
…ded keyboard input (apache#4497) * refactor(computer-use): make the model action space semantic-only * test(computer-use): close semantic action-space residue * fix(computer-use): remove unsupported hold key action * fix(computer-use): close keyboard activation review gaps Generated-by: GLM-5.3-Flash (ZCode)
3 participants
Why
Maka exposed coordinate mutation names to the model even though shipping configurations refused them. Models could therefore plan pointer actions that no production executor would run, while platform integrations could drift toward separate schemas or pixel fallbacks.
The production Computer Use surface should express intent through observed elements, windows, or verified focus. Presentation geometry may animate the cursor, but it must never become executor input. Every model-facing action must have a reachable production path.
This PR also intentionally enables the supported keyboard path. That is a product capability change, not incidental cleanup.
What changes
maka-cubackend path that converted model coordinates intodispatch.pointhold_key, which had no reachable shipping implementation; retain only historical transcript rendering and negative schema/codec testsclick_element,element_sequence.steps[].click,secondary_action, standalone screenshots, and semantic window geometry unchangedtype,key, andpress_keywhen the executor advertises the required key capabilitykeyboard_mutationapprovaldispatch.keyThis establishes one model-facing action space for macOS, Windows, and future executors. A platform backend may use a PID/HWND-bound native mechanism to implement an already observation-bound action, but it must not accept model-provided coordinates or silently fall back from semantic intent to pixels.
Protocol boundary
coordinate-backgroundremains an executor result tier in the closedmaka.cu/2protocol. Renaming that result vocabulary is outside this action-space change.The handshake still validates
pointActionsfor complete protocol fidelity, even though Maka no longer consumes that capability or callsdispatch.point.CuPointalso remains because presentation geometry still uses it.Companion executor contract PR
maka-agent/maka-cu#6is merged and makesdispatch.pointfail closed. The currently pinned bundled executor predates that revision and may still advertise point actions, but this is not an execution gap because this PR removes every Maka producer ofdispatch.point; merge order is therefore safe.Coverage added in the final review round
type,key, andpress_keyflow throughtool.permissionArgsas observation-boundkeyboard_mutationrequestsdispatch.keyonly through a bound target or verified focus ownerdispatch.keyclearSessioncannot mask a delivered keyboard mutation outcomeValidation
Validated on head
93152755fafter mergingorigin/maincd4aa3d8f:npm run build:testnpm run typechecknpm run lintnpm run format:checknpm run check:asf-headersgit diff --checkA full parallel
npm run test:distcompleted the Computer Use suite successfully but exposed six unrelated process/timing flakes in existing Runtime shell, Runtime Host candidate, Eval relay, and Desktop Rive tests under concurrent load. Every failed test passed when rerun serially. Hosted CI is the final clean-environment check for this head.Refs #4318