Uh oh!
There was an error while loading. Please reload this page.
fix(cu): make the agent cursor land where it is aimed, and stop it curling - #1883
Conversation
6b146ae to
f29ce0aComparehqhq1025
commented
Aug 3, 2026
Pushed fixes for all four findings. Every one of them reproduced against the code first. Finding 1, the fix could not fire. Confirmed: nothing in packages/ or apps/ ever set targetWindowId or targetStacking, so keepElevated returned true on every action, mayRest stayed false, and applyLevel('rest') with moveAbove inside it was unreachable. I brought the producer in rather than splitting the level half out, because the data it needs was already there: context.boundAction.target.windowId, guarded against the { pid: -1, windowId: -1 } miss sentinel. That is the whole producer. targetStacking is deleted instead. Its only possible producer needs a per-observation record of what covers the target, which the runtime does not collect on this base, and a context field that nothing can set is exactly how this defect got here. With it gone, keepElevated collapses to one rule that is fully backed: a window to order against replaces the level, and no window means stay up. Finding 2, the thresholds and the fence disagreed. Confirmed by simulating the actual stepSpring integrator rather than trusting the envelope: at springResponseMax the gate opens at 1937ms against a 1000ms fence, and a 2000px move sits at progress 0.910 when the fence fires, about 179px short. The two constants are now one relationship. The close-enough thresholds moved into the engine module next to the spring they are read against, cursorPresentationReadyDeadlineMs derives the worst case from both, the fence carries it, and the runtime takes the larger of the producer's deadline and its own. The tradeoff is explicit: a full cross-display move now waits for the glyph instead of dispatching mid-flight. Finding 3, complete() dropped the window id. Confirmed. It also turned out that assigning it was not enough on its own: resting is a level plus a position in another window's z-order, and applyLevel treated "already at rest" as a no-op even when the window to rest above had changed, so the reorder never happened. Both are fixed, with the ordering tracked separately from the level. Finding 4, no straight candidate. Confirmed numerically: arcCount was 20 at rest and 4 on interrupt, both even, so arc = 0 was never generated. Minimum forced bulge 5.82pt at rest and 36.87pt on an interrupted 400pt move. The count is odd now, so the midpoint is in the grid. I left the multi-display coordinate math alone. On the devicePixelRatio caveat I fixed it rather than commenting: the overlay spans every display, so crossing a scale-factor boundary changes devicePixelRatio without changing innerWidth, which fires no resize event, and the sameRect guard no longer rebuilds the window for it either. A resolution media query is the one signal that fires for it, and it fires while the render loop is idle. Negative controls, each reverted and rebuilt. Remove the targetWindowId producer: not ok 7, presentation receives the window the action is bound to. 71 pass, 1 fail. Restored, everything green: 49 pass in the two desktop cursor suites, 140 pass in packages/computer-use, 86 pass across the three runtime Computer Use suites. What I could not verify locally: packages/ui cannot be built on this machine, so 36 desktop main test files fail on a missing @maka/ui/dist. None of them are cursor, computer-use or overlay files, and the failure is a module resolution error rather than an assertion. The probe script in this PR needs a real desktop and was not run. One note for whoever merges: this branch is based on 5e782d9 and origin/main has moved two commits past it, including an export added to computer-use-tools.ts by #1870. It does not touch anything here, so it is a clean rebase, not a conflict. |
hqhq1025
commented
Aug 3, 2026
Second-round review addressed. Findings verified before changing anything; one is rejected with evidence and one is confirmed but deliberately not covered by a new test. The semantic path really was still cancelled. Reproduced by driving the real tool with the real hook and a recording sink, exactly as reported: bindCuaSemanticActionToObservation passed only {elementId}, so presentationScreenPoint() returned undefined and the hook returned at its first guard — before targetWindowId, which this PR does now produce, was ever read. The suggested direction turned out to be one transform too many. Element frames are already in the same screen coordinates as the window's bounds — validateSemanticElementVisibility compares one directly against the other — so there is nothing to map through sourceBoundsPx. The binding now carries the element centre as presentationScreenPoint, set only when that centre lies inside the target window, which is the same condition the executor validates before dispatching. Outside it, no point is offered at all: flying the cursor to somewhere the action will be refused from is worse than not moving it. It cannot dispatch anything either, since boundWindowPoint refuses any binding without coordinateSpace and a semantic binding sets none. After the fix the same probe gives move and complete at (220, 110), the centre of the element's own frame. The deadline's frame-rate dependency is confirmed and fixed at the source rather than documented. tick clamped its step to the integrator's stability bound and dropped the remainder, so a frame longer than 50ms advanced the simulation by less time than had passed. A long frame is now walked in sub-steps of that bound instead of truncated:
Simulated time now equals wall clock down to one frame per second; below that the overlay has stopped rather than slowed and the fence's backstop covers it. The docstring no longer claims more than it delivers. presentationFinishedTimeoutMs I am rejecting as stated. waitForPresentationFinished runs after onActionEnd, so the two backstops bound sequential intervals rather than the same one: readyTimeoutMs covers the whole motion up to the release gate, this one covers only the tail left after it, which complete() truncates in practice. Comparing 1500 against 2402 is not comparing two measurements of the same thing. What was genuinely missing is that nothing said so and nothing measured the tail, so the default is now a named export and a desktop test measures the longest tail the real engine can leave — 1283ms at the worst case — against it. The two numbers are related by an assertion now instead of by luck. On mouse_move: it is reachable in the shipping configuration. It is the one pointer action with no allowCompatibilityInputDispatch gate — cua-driver-backend.ts resolves the window and returns ok with tier coordinate-background, dispatching nothing at all, because it is purely a presentation action. So the planner, CURSOR_CLOSE_ENOUGH and the deadline chain do have a reachable consumer and this PR's timing half is not dead code. What follows is narrower than it looks, though: every other action still teleports, including click_element, which after the fix above reaches move() for the first time. Whether a click should glide the way Codex's does is now a live question rather than an unreachable one, but it is a behaviour change with its own argument and it is not in this PR. The moveTo rest-heading guard is confirmed a no-op for the behaviour its comment claimed: removing it leaves the departure alignment at 1.0000 measured through the engine and all 27 tests green. I did not add a test for it. I tried, and the test I wrote could not fail — reverting the guard left it green, and the only thing that reddened when I inverted SCORE_DETOUR_WEIGHT was the existing scorer unit test. Shipping an assertion that cannot fail is the failure mode this review round is about, so the guard stays for the reason it actually earns its place — it decides the candidate budget, see DEPARTURE_FAN — and the comment now says that instead of claiming to be what prevents the up-and-right launch. The dpr watcher's premise is corrected. The overlay is movable: false, resizable: false and already spans the union of every display, so it is never dragged anywhere; the real trigger is a scale-factor change leaving the union rect identical, which is what the guard before it suppresses. Negative controls, each run: dropping presentationScreenPoint from the binding reddens the new hook test with actual ['ensure','cancel'], expected ['move','complete']. Restoring the single clamped tick step reddens with "15fps: the gate opened after 2600ms of wall clock, past the 2402ms the runtime waits". Lowering the finished default to 1000 reddens with "the longest tail after the release gate is 1283ms, past the 1000ms the runtime waits". apps/desktop cursor suites 51/51, packages/computer-use 144/144, packages/core 697/697, packages/runtime CU suites 124/124. The other runtime failures on this machine are shell/PTY timing under load average 54 and pass in isolation; they touch nothing here. |
3e49712 to
d08fc3cComparehqhq1025
commented
Aug 3, 2026
Rebased onto origin/main at 623fcf5, cleanly, and fixed both findings. The blocker is confirmed and reproduced exactly. Driving the built engine through the production loop's own expression, a cross-display move (20,20) to (3800,1500) in a 3840x1600 viewport opened the release gate at 1950ms at 20fps, 2600ms at 15fps, 3900ms at 10fps and 7800ms at 5fps, against the 2402ms deadline, where the same sweep with an unclamped delta read 1950/2000/2000/2000. The tail after the gate measured 1733ms at 15fps and 2600ms at 10fps against the 1500ms default. Math.min(0.05, ...) was in the shipped bundle at dist/overlay/cursor-overlay.js:664. I did not just delete the clamp. A delta in the caller's hands is a delta the caller can bound, and that is how this happened once already, so the engine now owns the frame clock: tickTo(nowMs) takes the frame timestamp and derives the delta itself, and resetFrameClock marks the gap a loop that blocks on idle should not replay. The overlay loop is one line, engine.tickTo(now), with no arithmetic to get wrong, and tickTo is now the only .tick( caller in the whole of apps/desktop outside the engine. The tests moved to that seam. Both frame-rate tests drive tickTo with timestamps instead of calling tick(1/fps), and the tail test is swept over 60/30/20/15/10/5fps instead of fixed at 60 — at 60fps a frame is inside the stability bound, which is exactly why a fixed-60fps tail test could not see this. Negative control, re-adding the clamp inside tickTo: "15fps: the gate opened after 2600ms of wall clock, past the 2402ms the runtime waits" and "15fps: the longest tail after the release gate is 1733ms, past the 1500ms the runtime waits for a presentation to report it has finished". Both green on restore, 26/26. After the fix the sweep reads 1933-2000ms to gate and 1300-1400ms of tail at every rate from 60 down to 5. On the test that could not fail: confirmed, and it was worse than described. sample(1) is end for any cubic, so that assertion was true by construction. But on that particular move, (400,400) to (800,420), the fan is not merely unasserted — it is unused: swept across all headings the scorer picks the unfanned candidate every time, and the plan is bit-identical to the from-rest one. So the test had picked the one geometry where the mechanism it is named after does nothing. I checked whether the mechanism is dead before writing a test for it. Over 2688 plans swept across distances, directions, headings and both viewport modes, the carried heading changes the chosen path in 632 of them, by up to 39.3pt of control point. It is load-bearing, so it gets a real test rather than removal. The test now uses (1000,700) to (-500,-200) with a carried heading of -135 degrees, and asserts three things: that the departure handle actually moved (24.1pt), that the initial tangent leans toward the carried heading rather than merely differing from the fresh plan, and the complement, that a heading already on the direct line collapses to the from-rest plan exactly. Negative control, setting departureCount = 1 as you did: "the carried heading moved the departure handle by 0.00pt, which means the departure fan was never consulted". The landing equality is still stated, but as a remark about the cubic, never as evidence. Nothing else was touched. The deadline formula, the two timeout constants, the multi-display geometry and elementPresentationPoint are all as you left them. Verification: @maka/desktop 1369 pass. The single failure was "kills login-shell descendants when capture times out" in shell-env, unrelated and passing in isolation (18/18) — load was 11-80 through this run. biome check clean on the three touched files, check-console passes. |
3e49712 to
07296b8Compare…rling Four fixes to the cursor overlay, all found by measuring against Codex's own build rather than by looking at the result and adjusting until it seemed right. The curling was the big one, and the cause was a note in this file. It said the candidate-path scoring function had been stripped from the binary, so a placeholder scorer had been written against that claim — one that rewarded the most curved candidate. The scorer had not been stripped: it is inlined into the planner at 0x1000972ec, and every term is a cost, so the straightest candidate that meets the constraints wins. Reproducing it term for term takes a 654px path from 71px of chord deviation to 2.4px. `dubins.ts` goes with it. It planned a minimum-turning-radius arc–straight–arc path, which is a different planner from the one Codex uses, and keeping both meant two answers to "where does the cursor go". The other three: - The cursor is drawn as an arrow rather than a dot. A dot has no hotspot a person can read, and the thing it is imitating is a pointer. - `close enough` uses the recovered thresholds (0.995 progress, 3.157px), so the action is released once the cursor has effectively landed. Releasing earlier makes the click visibly fire while the glyph is still travelling. - The overlay orders itself above the target window by media source id rather than sitting at a fixed always-on-top level. A comment claimed Electron could not do this; it can, through `orderWindow(.above, relativeTo:)`. `cu-cursor-accuracy-real.mjs` measures landing error against a real window, which is how the first of these was caught: it looked fine and measured wrong.
CI caught a dependency the split dropped. `cursor-overlay-window.ts` reads `keepElevated` off `CursorMoveInput`, and the overlay hook that sets it reads `targetStacking` and `targetWindowId` off the hook context — none of which came along with the cursor changes, so the tests referenced fields that did not exist. Both fields are additive on `CuOverlayHookContext`, and the rule they carry is the one this PR's engine change depends on: a covered target is a reason for the cursor to stay elevated, not to disappear. With no stacking evidence at all it stays elevated, because an unseen cursor is the failure the whole path exists to avoid.
…ts in
Four defects, all of which survived because the tests supplied by hand what
production does not supply.
The level change could not fire at all. `keepElevated()` reads `targetWindowId`
and `targetStacking` off the hook context, and nothing in `packages/` or `apps/`
ever set either one, so it returned true on every action, `mayRest` stayed
false, and `applyLevel('rest')` — with `moveAbove` inside it — was unreachable.
Shipped, the cursor stayed pinned above every application window for the whole
session: exactly the arrow-over-what-you-are-reading this was written to remove.
The runtime now produces `targetWindowId` from the action's own binding, which
is data it already had. `targetStacking` is deleted rather than kept: its only
possible producer needs a per-observation record of what covers the target,
which the runtime does not collect, and a context field nothing can set is how
this got here.
The new release thresholds were unreachable inside the runtime's fence, for
precisely the long moves they were raised for. The gate moved to progress 0.995
while the fence stayed at a separately-chosen 1000ms; the spring needs about
1940ms at its slowest response, so any move past ~1000px both stalled for a
fixed second and then dispatched anyway with the glyph still travelling — 179px
short on a 2000px cross-display move. The thresholds now live next to the spring
they are read against, one function derives the deadline from both, and the
fence carries it.
`complete()` was dropping the `targetWindowId` the hook goes out of its way to
attach, and resting is not a level alone but a level plus a position in another
window's order — so "already at rest" was treated as a no-op even when the
window to rest above had changed. An action whose begin never ran left the
cursor ordered above the previous action's window, behind the one it points at.
The candidate grid contained no straight path. `arc` is spread over an even
number of steps, so `arc = 0` was never generated: ~5.8pt of forced bulge at
rest, and 36.9pt on an interrupted 400pt move, in a planner whose stated job is
to pick the straightest candidate that clears the viewport. The count is odd
now, so the midpoint is in the grid.
Also refreshes the canvas backing store on a resolution change. The overlay
spans every display, so moving across a scale-factor boundary changes
devicePixelRatio without changing innerWidth — no resize event, and the new
teardown guard no longer rebuilds the window for it either.The comment this PR added says the fallback "is not a guess:
`presentationScreenPoint` is the point this same action was addressed to,
computed from the element's own frame". Nothing computed that.
`bindCuaSemanticActionToObservation` called `bindCuaAction` with only
`{elementId}`, so `presentationScreenPoint()` returned `undefined` for every
element action and the hook returned at its first guard. Driving the real tool
with the real hook and a recording sink:
SEMANTIC click_element: ensure "s1" ; cancel {}
COORDINATE left_click: move {"x":300,"y":200,…,"targetWindowId":4321}
complete {"x":300,"y":200,…,"pulse":true}
For `click_element` the cursor never moved, nothing landed, nothing pulsed,
and `targetWindowId` — which this PR does now produce — was never read,
because the function had already returned. That is the whole accessibility
path, which is the only path Maka dispatches on by default. The hook test
wrote `presentationScreenPoint` into the context by hand and both new runtime
tests drove `left_click` with an explicit coordinate, so nothing could see it.
An element action is aimed at an element, so it now carries the point
directly rather than recovering one from a coordinate it never had. Element
frames already arrive in the same screen coordinates as the window's bounds —
`validateSemanticElementVisibility` compares one against the other — so the
centre needs no transform, only the containment check that validator applies.
Outside the window the frame is stale and no point is offered at all, which
is better than flying the cursor to somewhere the action will be refused from.
It cannot dispatch anything: `boundWindowPoint` refuses a binding without
`coordinateSpace`, and a semantic binding sets none.
The tests now drive `buildComputerUseTools` with the real hook and record
what the sink is told, so `move` and `complete` are asserted where they are
produced rather than where a fixture can supply them.
Second: the deadline stopped depending on the frame rate. The derivation is
right and matches the engine at 60fps, but `tick` clamped its step to the
integrator's stability bound and dropped the remainder, so a frame longer
than 50ms advanced the simulation by less time than had passed. Measured
through the real engine against the real release gate:
fps= 30 gate open @ 1967 ms
fps= 20 gate open @ 1950 ms
fps= 15 gate open @ 2600 ms past the 2402ms deadline
fps= 10 gate open @ 3900 ms past the 2402ms deadline
17ms of headroom covers one dropped frame, not sustained throttling, and the
docstring claimed the bound held regardless of frame rate. A long frame is
now walked in sub-steps of the stability bound instead of truncated, so
simulated time equals wall clock at any rate down to one frame per second;
below that the overlay has stopped rather than slowed and the fence's backstop
is what covers it. Every rate from 240fps to 1fps now opens the gate at
1938-2000ms. The existing test integrates its own spring at a fixed 1/240 and
measures simulated time, so it could not see this; the new one drives
`CursorEngine.tick` and measures wall clock.
Third, `presentationFinishedTimeoutMs`. Review read it as `readyTimeoutMs`
chosen a second time and smaller. It is not, and the wait it starts is not
reachable as a defect: `waitForPresentationFinished` runs after `onActionEnd`,
so the two bound sequential intervals — the ready deadline covers the whole
motion up to the release gate, this one covers only the tail left after it.
What was missing is that nothing said so and nothing measured the tail. The
default is now a named export and a desktop test measures the longest tail the
real engine can leave (1283ms) against it, so the two numbers are related by an
assertion rather than by luck.
Also two comments that describe something other than what the code does. The
`moveTo` rest-heading guard does not stop a fresh move launching up-and-right:
removing it leaves the departure alignment at 1.0000 and every test green,
because the candidate grid and the scorer already deliver the direct
departure. It still decides the candidate budget, which is why it stays, and
the comment now says that instead. And the dpr watcher's premise — "dragging
it onto a screen with a different scale factor" — cannot happen: the overlay
is `movable: false`, `resizable: false` and already spans the union of every
display. The mechanism was right and the justification was not; the real
trigger is a scale-factor change that leaves the union rect identical, which
is what the guard before it suppresses.…es production `tick` was taught to sub-step a long frame instead of truncating it, and the only production caller went on truncating first. `cursor-overlay.ts` computed `Math.min(0.05, (now - last) / 1000)` one call earlier — the same clamp, at the same 50ms bound — so the engine was never handed a long frame to sub-step, and `cursorPresentationReadyDeadlineMs` was still a claim about frame rate rather than about the spring. The engine now owns the clock. `tickTo(nowMs)` takes the frame timestamp and derives the delta behind the call, so there is no delta on the caller's side for anything to clamp. `tick` stays public for tests that want to state a step outright. The clock is dropped whenever the presentation comes to rest, which is exactly when the overlay stops asking for frames. Without that, a cursor idle for a minute resumes with a minute of arrears and integrates `MAX_CATCH_UP` of a motion it was handed a millisecond ago: measured, 3647px of a 4059px move on the first frame, gate open, click dispatched with the glyph nowhere near the target. The cost is that the frame which restarts the clock advances nothing, which is one frame of latency the presentation fence's backstop already covers. Two tests, both shown to go red on the defect they pin rather than only green on the fix: - `cursor-engine.test.ts` drives `tickTo` with a frame clock instead of calling `tick` with a step it computed itself. The old shape is the one thing the overlay does not do, which is why the suite stayed green while production truncated. - `cursor-overlay-frame-clock-contract.test.ts` reads the overlay source, the precedent already in that directory for a renderer entry point that cannot be imported under `node --test`. It pins both halves: the timestamp goes to the engine whole, and no delta is computed on this side to clamp.
07296b8 to
c21e82cCompareUh oh!
There was an error while loading. Please reload this page.
The curling had a cause, and the cause was a comment
cursor-engine.tscarried this note:A placeholder scorer was written against that claim. It rewarded the most curved candidate — which is why the cursor looped on its way to a target.
The scorer had not been stripped. It is inlined into the planner at
0x1000972ec, and every term in it is a cost, so the straightest candidate meeting the constraints wins.dubins.tsis removed with it: it planned a minimum-turning-radius arc–straight–arc path, a different planner from the one Codex uses, and keeping both meant two answers to "where does the cursor go".Three smaller ones
close enoughuses the recovered thresholds (0.995 progress, 3.157px). Releasing the action earlier makes the click visibly fire while the glyph is still travelling.orderWindow(.above, relativeTo:)by media source id, instead of a fixed always-on-top level. A comment in the file claimed Electron could not do this. It can.Verification
cu-cursor-accuracy-real.mjsmeasures landing error against a real window. That is how the scorer bug was caught — it looked fine and measured wrong, which is the whole reason this ships with a measurement script rather than a screenshot.Local desktop typecheck cannot run on this machine (upstream's
@astryxdesign/core@0.2.0does not resolve here); CI is the check for the desktop build.