feat(computer-use): move overlay cursor for set_value and press_key - #5051
feat(computer-use): move overlay cursor for set_value and press_key#5051wutongyuonce wants to merge 2 commits into
Conversation
claimBoundAction already computes a presentation point for any semantic action with an element frame. kindOf only treated click/select/secondary and scroll as movable, so set_value and element-targeted press_key only ensured the session cursor. Map those two to the click cursor; a press_key without a point still only ensures. Generated-by: pi-coding-agent
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
The behavioral change is correct and minimal: set_value and press_key now present a 'click' cursor, and I verified the seam produces a point for them — claimBoundAction treats both as semantic actions and bindCuaSemanticActionToObservation derives the element-centre point from the observed frame. The only issue is that one of the two new cases is not pinned by a test, and the switch that caused the original omission is still not closed.
P3 (Nice-to-have) — the press_key half has no test that can fail, so it can silently regress.
// packages/computer-use/src/__tests__/computer-use-overlay-hook.test.ts:215-221
test('press_key without a presentation point still only ensures', () => {
const { controller, moves, ensured } = fakeController();
const hook = createComputerUseOverlayHook(controller as never);
hook.onActionBegin({ type: 'press_key' }, { sessionId: 's1', toolCallId: 'a1' });
assert.deepEqual(moves, []);
assert.deepEqual(ensured, ['s1']);
});This action carries no presentationScreenPoint, so kindOf returning 'click' or undefined is unobservable — the hook takes the ensure-only path either way. The test therefore passes on origin/main and on this head, and it cannot detect press_key being dropped from kindOf. The set_value case is properly pinned (:198-213 asserts kind: 'click' and the x coordinate, and fails on base), so the asymmetry is only in the new press_key half.
The positive path is legal and reachable — press_key accepts an elementId, and the binding layer supplies a point when the element has a frame — so a discriminating case can be written directly. Add a press_key case with a presentationScreenPoint asserting move with kind: 'click' followed by complete, or drive the real tool with { action: 'press_key', element_id: '5', text: 'Tab' } and assert ['move', 'complete'].
P3 (Nice-to-have) — kindOf is still a hand-maintained allow-list, so the next element-bound action falls out silently.
// packages/computer-use/src/computer-use-overlay-hook.ts:74-87
function kindOf(action: CuPresentationAction): CursorActionKind | undefined {
switch (action.type) {
case 'click_element':
case 'select_text':
case 'secondary_action':
case 'set_value':
case 'press_key':
return 'click';
case 'scroll_element':
return 'scroll';
default:
return undefined;
}
}The default returns undefined for anything unmatched, and nothing makes the compiler flag a new member of the union — strict is on but noImplicitReturns is not, and the switch is not exhaustiveness-checked. That is precisely how set_value and press_key came to be missing, and this file's own comment records an earlier instance of the same class of defect (:223-232: every element action reached the sink as ensure then cancel because nothing produced the point). The runtime, not this list, is the source of truth for "this action is aimed at an element" — it computes a point for every bound semantic action.
Concrete current instance: window_action is in the semantic list in claimBoundAction and carries an elementId (the window itself), so it receives the same kind of point when the window element has a frame, yet kindOf returns undefined and it stays ensure-only with no comment saying that is deliberate. I did not execute a real window observation, so whether the window element carries a frame in practice is unverified — but there is no stated reason for the exclusion either way.
The zero-risk fix is to make the switch exhaustive so a future action type fails to compile instead of silently not moving the cursor: list the remaining members explicitly (window_action, screenshot, type, key, wait) with a one-line rationale each, and end with default: { const unhandled: never = action.type; return undefined; }. This has no runtime effect today; it converts a silent omission into a build error. Scoping it out is a legitimate maintainer call, but it should be an explicit one.
Review-relevant risks. No public contract, wire shape, security boundary, dependency, licensing or release effect was identified. The change is presentation-only: it affects where the agent cursor is drawn, not what is dispatched.
Required conclusion.
- Optimal for the actual problem? Adequate and minimal for the reported defect — two lines, correct kind, correct target. Not optimal against the root cause, which is the allow-list rather than the presence of a runtime-computed point.
- Production code that can be deleted?
none identified— the change is purely additive, andCursorActionKind/kindOfare still needed to choose between'click'and'scroll'. - Low-quality tests to delete or replace? Nothing should be deleted. Strengthen the
press_keycase above so it discriminates. - Deeper refactor required? No. Optional hardening: the exhaustive switch, or moving the "does this action present a point" decision next to
presentationScreenPointin the runtime. - Ready to merge? Yes for the stated scope; the two P3s are cheap and can land in this PR or a follow-up.
- Residual risks / verification gaps: I did not run the suite, and I could not observe a real window action, so the
window_actioninstance is reasoned rather than executed. Whether any given element yields a point still depends on the executor supplying a frame for it.
Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
kindOf is now an exhaustive switch so a new CuPresentationAction type fails to compile instead of silently ensuring. A press_key with a point is tested at the hook and the seam, including the landing pulse. A window_action with a point still only ensures. Generated-by: pi-coding-agent
|
Addressed the two P3s:
Also locked two holes the review did not name:
|
me2seeks
left a comment
There was a problem hiding this comment.
Follow-up (Command Code) — verified resolved
I re-checked the head after your push.
Both P3s are addressed. kindOf now decides explicitly for every presentation type and ends in a never exhaustiveness check, so the next new action type fails to compile instead of silently falling out of an allow-list — which is exactly the class of bug that dropped set_value and press_key in the first place. The added coverage pins press_key and window_action rather than only set_value.
Thanks for closing the root cause, not just the instances. (Automated review; not an approval.)
Summary
set_valueand element-targetedpress_keyalready get apresentationScreenPointfromclaimBoundAction. OverlaykindOfonly treated click/select/secondary/scroll as movable, so those two actions only ensured the session cursor.This PR maps
set_valueandpress_keyto the click cursor. Apress_keywithout a point still only ensures. Coordinate-dialecttype/key/screenshot/waitstay unmoved.window_actionis out of scope.Fixes #5049
Does this PR entail a change in behavior?
Verification
npm --workspace @maka/core run buildnpm --workspace @maka/storage run buildnpm --workspace @maka/runtime run buildnpm --workspace @maka/computer-use run buildnpm --workspace @maka/computer-use run test:dist— 120 pass, including hookset_valuemove,press_keywithout a point still ensure, anddriveRealToolset_value→ move+completelint/format:check/ fulltypecheckAI use
Select exactly one:
Tool(s) and scope:
pi-coding-agent drafted the
kindOfmapping, tests, and this description. Commits carryGenerated-by: pi-coding-agent.Checklist