Skip to content

js repl: surface thrown errors, skip action read-back on macOS - #79

Merged
iFurySt merged 2 commits into
iFurySt:mainfrom
hyprcat:fix/js-repl-adapter
Sep 22, 2026
Merged

iFurySt merged 2 commits into
iFurySt:mainfrom
hyprcat:fix/js-repl-adapter

Conversation

@hyprcat

@hyprcat hyprcat commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the numbers in #78: keeps the adapter-over-native-MCP design, fixes the two things that made it slow and fragile.

1. Thrown errors settle the call

A throw inside js code (sync, or after an await) never reached the REPL eval callback: Node prints it and moves on, so the call sat until timeout_ms (30 s) and then reset the session, taking app bindings with it. The session now settles the active evaluation from the REPL's error path (handleError on Node 26, the domain error event on Node 22) and returns isError with the message. Bindings survive; only timeouts reset.

Also: errors thrown inside the REPL context are from another realm and failed instanceof Error, which produced Error: Error: ....

2. Actions skip the read-back when a program drives them

Every native action settled ~150 ms and returned a fresh AX snapshot that app.click() etc. discarded. New OPEN_COMPUTER_USE_ACTION_READ_BACK=0: macOS actions return a short ok, no settle, no snapshot. All action-result sites route through one actionResult(for:); the settles that exist only for the read-back go through pauseBeforeReadBack(_:). Read per call so the app agent's per-request env forwarding applies. The adapter sets the variable when launching the native runtime. Default nine-tool behaviour is unchanged.

Calculator.app, one js call per row, 5 reps, median ms, display verified:

scenario before after
clear×2 + click "7" + read 2295 402
clear×2 + click "7" ×10 + read 6469 405
thrown exception → error returned 30000 + session reset 0, bindings intact

Raw native click with the flag: 270 ms → 4 ms. What's left in the rows above is the adapter's own getAXState() reads.

Linux and Windows runtimes ignore the variable and still read back after each action; the same switch would go in their actionResult paths and runtime scripts. Left out so you can decide whether you want it there.

Validation

  • node --test scripts/node-repl/open-computer-use-repl.test.mjs — 13/13 on Node 26.4 and Node 22.23 (new test fails with a 1 s timeout before the fix)
  • swift test — 168 passed, 1 opt-in live test skipped
  • ./scripts/run-tool-smoke-tests.sh — native nine-tool and cursor-idle smoke passed (default read-back)
  • make check-docs, git diff --check
  • Bench script: https://gist.github.com/hyprcat/8997d42d2f226fbac9ce13e49b46ec27

@iFurySt

iFurySt commented Sep 22, 2026

Copy link
Copy Markdown
Owner

@hyprcat Thanks for the fix! One edge case may still be problematic: if a completed js call schedules an async callback that throws later, the shared rejectActive can reject the next active call instead, and subsequent calls may hang until timeout. I reproduced this on Node 22 and 25 with setTimeout(() => { throw new Error("late") }, 50). Could you confirm and add a regression test for this case before we merge?

A throw inside js code never reached the REPL eval callback, so the call sat
until timeout_ms and then reset the session. Settle the evaluation from the
REPL's error path (handleError on Node 26, the domain on Node 22) and return
isError with the message; bindings survive.

The adapter discards every action's native result, yet each action paid a
150 ms settle plus a full AX snapshot. OPEN_COMPUTER_USE_ACTION_READ_BACK=0
makes macOS actions return a short status and skip both; the adapter sets it
when launching the native runtime. Default nine-tool behaviour is unchanged.

Calculator, one js call, median ms: clear x2 + click 7 + read 2295 -> 402;
clear x2 + click 7 x10 + read 6469 -> 405; thrown exception 30000 -> 0.
Settling whichever call was active let a late throw from a finished call
(setTimeout(() => { throw ... }, 50)) reject the next call, and the call it
rejected kept running and wrote into the call after it. Run each evaluation
inside an AsyncLocalStorage owner; the REPL error path settles only the call
that threw, and nodeRepl output is refused to code whose call has returned.
Late errors from finished calls are dropped, as the REPL did before.

Regression test covers both the in-process and Worker sessions; passes on
Node 26 and 22, fails on the previous commit.
@hyprcat
hyprcat force-pushed the fix/js-repl-adapter branch from 4c3ed5b to 32b8ec0 Compare September 22, 2026 12:10
@hyprcat

hyprcat commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@iFurySt confirmed on Node 22 and 26. A's late throw rejected B. The same bug had a second symptom: B kept running after the early rejection and its nodeRepl.write landed in C's output.

Fixed in 32b8ec0. Each evaluation now runs inside an AsyncLocalStorage owner, and timers and promises carry it. The error path settles only the call that threw, and nodeRepl output is refused to code whose call has already returned. A late error from a finished call is dropped, same as the REPL did before this PR. If you'd rather surface it in the next call's output as a note, that's a small change.

Regression test (<Session>: a late throw from a finished call does not reach the next call) runs on both PersistentJavaScriptSession and WorkerJavaScriptSession. It fails on the previous commit and passes on Node 26.4 and 22.23, with all 21 scripts/node-repl tests green on both. I didn't hit the follow-on hang in my runs, but the misattribution it would come from is gone.

Also rebased onto main after #80. The only conflict was the release-notes row, and I kept both.

@iFurySt

iFurySt commented Sep 22, 2026

Copy link
Copy Markdown
Owner

cool. lgtm. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants