fix(events): use the WebSocket global and drop the ws dependency

[georgeglarson]

HUMAN:

Human verified, Screenshot attached.

---

AGENT:

Ported from OpenHands/typescript-client#370 (closed unmerged when that repo was archived and the client moved here; the moved files are byte-identical to the pre-fix versions, so the bug crossed repos untouched). Verified end-to-end from clients/typescript:

• Reproduced the issue on main first: npm ci && npm run build, then the repro from the linked issue — onError receives WebSocket implementation not available on Node 22.22.2 despite globalThis.WebSocket existing.
• Same repro on this branch: the error is gone; the client reads the global and attempts the connection.
• npx jest src/__tests__/package-import.test.ts → 8 passed, including new cases asserting each client opens the exact expected URL against a fake globalThis.WebSocket.
• npm test → 311 passed, 18 suites. npm run lint → 0 errors (and two fewer pre-existing any warnings in these files). npm run format:check → clean. npm run build → clean.

Why

The package is emitted as ESM, and Node 22 provides a standards-compatible globalThis.WebSocket. The conversation and bash event clients only checked window.WebSocket, then attempted require('ws') — which is unavailable in ESM — so starting either client in a Node ESM process reported WebSocket implementation not available even though the global exists. Full repro in the linked issue.

Summary

• Read globalThis.WebSocket directly in both event clients and drop the ws dependency (every supported runtime supplies the global: browsers, and Node.js since 22.4).
• Keep the "no implementation" condition deferred to connect() time and surfaced via the existing onError callback, so importing the package barrel still never throws.
• Rewrite the package-import tests to delete the global for real instead of mocking ws, and add coverage pinning the URL each client opens.

Issue Number

Fixes #4846.

How to Test

cd clients/typescript
npm ci
npm run build
node --input-type=module -e "const {WebSocketCallbackClient}=await import('./dist/events/websocket-client.js'); const client=new WebSocketCallbackClient({host:'http://127.0.0.1:9',conversationId:'test',callback:()=>{},onError:(e)=>console.error('ONERROR:',e.message)}); client.start(); client.stop()"

On main this prints ONERROR: ... WebSocket implementation not available; on this branch it does not. Then npm test and npx jest src/__tests__/package-import.test.ts.

Video/Screenshots

Design Doc

N/A — small runtime-detection fix.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

The same change was previously reviewed as OpenHands/typescript-client#370 and went unmerged only because that repository was archived on 2026-08-30 when the client moved into this repo (OpenHands/typescript-client#371). Consumers on Node < 22.4 would lose the ws fallback, but the package already requires Node 22-era tooling throughout its CI and the global has been unflagged since Node 22.4.

Jev-Fast-Audit

⚡ Jev fast audit · estimates · 0.53s · commit b39586a
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: ⚠️ reduced context — partial coverage; 8/8 hunks, 5/5 files (missing context: 4).

All estimates and evidence

Estimate	Likelihood / value	Direct evidence
SQL injection	3.0%	No direct hunk selected
Command injection	4.0%	No direct hunk selected
Weakened authentication	4.0%	No direct hunk selected
Weakened authorization	4.0%	No direct hunk selected
Contract regression	21.0%	No direct hunk selected
Data loss	3.0%	No direct hunk selected
Sensitive data disclosure	3.0%	No direct hunk selected
Unexpected data transfer	3.0%	No direct hunk selected
Credential misuse	4.0%	No direct hunk selected
Untrusted instruction authority	2.0%	No direct hunk selected
Package source redirection	3.0%	No direct hunk selected
Unverified remote execution	3.0%	No direct hunk selected
Privileged environment access	2.0%	No direct hunk selected
Security assessment bypass	4.0%	No direct hunk selected
Prohibited workload	2.0%	No direct hunk selected
Primary concern	None selected; confidence 47.0%	No primary concern to locate

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Review summary

The runtime-detection fix itself is correct and the reported bug is real: main still has the window.WebSocket / require('ws') block in both events/websocket-client.ts and events/bash-websocket-client.ts, which fails in Node ESM even when globalThis.WebSocket exists. Reading globalThis.WebSocket is equivalent to window.WebSocket in browsers and to the Node 22.4+ global in Node, so the change removes the crash and correctly keeps the "no implementation" condition deferred to connect() time. Locally I confirmed npm ci, npm run build (exit 0), npm run lint (0 errors), and npx jest src/__tests__/package-import.test.ts (8/8 pass) on this head.

However, the PR is not currently mergeable and its new tests will not run after a rebase:

1. Merge conflict with main. GitHub reports mergeable_state: "dirty" for head b39586a; a local merge-tree shows conflicts in clients/typescript/package-lock.json and clients/typescript/src/__tests__/package-import.test.ts. This branch is stale — main has moved on (the TS client Jest→Vitest migration #5075, dependency/image bumps, and a large Python-side delta) since this head's base. A rebase is required before merge.

2. Tests target Jest, but main now uses Vitest. The rewritten package-import.test.ts uses jest.isolateModules / jest.resetModules, and the head still ships jest.config.cjs. On current main those Jest config files are gone and npm test runs vitest. After rebasing, these new cases must be ported to Vitest (vi.resetModules, dynamic import, vi.spyOn(Module.prototype, 'require') style mocking as main uses) or they will simply not execute. This is the same file that is conflicted, so the port should happen as part of resolving it.

3. Stale ws guidance in the deferred error. With ws removed from dependencies and the fallback deleted, the connect() error still tells users to Install the ws package (events/websocket-client.ts line 83 and events/bash-websocket-client.ts line 70 — both unchanged by this diff, so not inline-anchored). Installing ws can no longer fix anything; the message should point at the runtime requirement instead. Non-blocking, but the PR leaves the guidance inconsistent with the new behavior.

4. Informational (acknowledged in the PR body): removing the ws fallback is a silent behavior change for consumers on Node < 22.4, and package.json declares no engines bound while CI only exercises Node 22.12 / 24.x. Worth making the requirement explicit, but not a blocker given the mature-Node posture.

Nothing here changes the correctness of the fix, but the branch needs a rebase and a test-runner port before it can land.

🔄 CHANGES REQUESTED

[all-hands-bot]

Dropping the ws fallback is a silent breaking change for Node < 22.4: package.json declares no engines bound and CI only exercises Node 22.12 / 24.x, so older-Node consumers that relied on the require('ws') path now fall through to the onError "WebSocket implementation not available" at start(). Consider declaring engines.node (e.g. >=22.4) so the new runtime floor is explicit, or retain the fallback if older Node must stay supported. Same applies to events/bash-websocket-client.ts.

[all-hands-bot]

This rewritten suite is Jest-based (jest.isolateModules / jest.resetModules), but main has migrated the TS client to Vitest (#5075): jest.config.cjs no longer exists and npm test runs vitest. After rebasing, these cases will not run as written — and this file is one of the two conflicted paths against main. Please port them to Vitest (vi.resetModules + dynamic import / vi.spyOn-based require interception, matching the current main version) while resolving the conflict.