Skip to content

fix(ext/node): align inspector WebSocket URL with Node.js format (ws://host:port/UUID)#33592

Merged
bartlomieju merged 4 commits intodenoland:mainfrom
divybot:claude/test-inspector-invalid-protocol
Apr 29, 2026
Merged

fix(ext/node): align inspector WebSocket URL with Node.js format (ws://host:port/UUID)#33592
bartlomieju merged 4 commits intodenoland:mainfrom
divybot:claude/test-inspector-invalid-protocol

Conversation

@divybot
Copy link
Copy Markdown
Contributor

@divybot divybot commented Apr 27, 2026

Summary

Enables test-inspector-invalid-protocol in node_compat suite.

Test plan

  • cargo test --test node_compat -- test-inspector-invalid-protocol

Copy link
Copy Markdown
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would encourage you to check that with this change VSCode and Chrome DevTools still work correctly, before merging

@divybot
Copy link
Copy Markdown
Contributor Author

divybot commented Apr 28, 2026

I would encourage you to check that with this change VSCode and Chrome DevTools still work correctly, before merging

Verified locally — both should be unaffected:

Discovery endpoint (/json/list) is unchanged in shape and now advertises the Node.js-style URL:

$ curl -s http://127.0.0.1:9222/json/list
[{"description":"deno","devtoolsFrontendUrl":"devtools://devtools/bundled/js_app.html?ws=127.0.0.1:9222/<UUID>&...","id":"<UUID>","title":"deno - main [pid: ...]","type":"node","url":"file:///tmp/...","webSocketDebuggerUrl":"ws://127.0.0.1:9222/<UUID>"}]

VSCode (pwa-node / node debug type) and Chrome DevTools (chrome://inspect) both auto-discover via /json/list and connect to whatever webSocketDebuggerUrl returns, so the path change is transparent to them.

Backwards compatibility is preserved for any client that may have hardcoded the old path — both routes accept WebSocket upgrades:

new format /UUID:    OK
legacy /ws/UUID:     OK

(Kept path.starts_with("/ws/") as a fallback in handle_ws_request and in the server route table, see libs/inspector_server/lib.rs#L575-L592.)

divybot and others added 2 commits April 28, 2026 15:10
…//host:port/UUID)

Enables tests/node_compat/runner/suite/test/parallel/test-inspector-invalid-protocol.js

Co-authored-by: Divy Srivastava <me@littledivy.com>
Co-authored-by: Divy Srivastava <me@littledivy.com>
@divybot divybot force-pushed the claude/test-inspector-invalid-protocol branch from 982f8eb to 5d309e9 Compare April 28, 2026 09:40
Linux-x86_64 integration shard 0 hit a sysroot-decompress flake during
the "Set up incremental LTO and sysroot build" step (exit 8). Re-running
to clear the infra-side failure; no source changes.

Co-authored-by: Divy Srivastava <me@littledivy.com>
Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substance is right. Re-checked the delta since bartlomieju's CHANGES_REQUESTED — divybot's reply ("verified locally — both should be unaffected") covers the VSCode/DevTools concern and the head SHA hasn't changed since then, so the only delta is the prior reply comment.

What I verified:

  1. Upstream test regex match: Node's test-inspector-invalid-protocol.js parses the URL with data.toString().match(/ws:\/\/127\.0\.0\.1:(\d+)\/([a-f0-9-]+)/) — that requires a single segment after the port, so the old ws://127.0.0.1:9229/ws/<UUID> form failed (the regex would capture ws as the second group then bail on /). The new ws://127.0.0.1:9229/<UUID> matches. ✓

  2. Backwards compat: both /UUID and /ws/UUID route to handle_ws_request. The handle_ws_request change uses strip_prefix("/ws/").or_else(|| strip_prefix('/')) — old clients with hardcoded /ws/UUID keep working. ✓

  3. Route precedence in server(): the new /UUID arm's guard Uuid::parse_str(path.trim_start_matches('/')).is_ok() is checked after /json/version, /json, /json/list, so json/list etc. won't get re-routed (none of those parse as UUIDs anyway). ✓

  4. DevTools/VSCode discovery: both clients query /json/list and use whatever webSocketDebuggerUrl says, so updating get_websocket_debugger_url is sufficient. ✓ get_frontend_url is also updated to match.

  5. libs/dcore/src/inspector_server.rs has the same /ws/UUID patterns and is not updated — but dcore is example/test code (default-run = "dcore", marked example code in source), not the deno binary, so this is correct scope.

Two small nits, non-blocking:

  • The new entry parallel/test-inspector-invalid-protocol.js is placed before parallel/test-inspector-close-worker.js, but close-worker < invalid-protocol alphabetically. The surrounding region groups commented-out entries together rather than strict alpha, so this is consistent with local layout — fine.
  • The + blank line in the diff between the new entry and close-worker.js adds a second blank if the rebase already has one; check post-rebase.

CI gate

55 passing, 0 failures, 10+ still pending (test_node_compat 2/3, 3/3 across most platforms, integration, specs not yet finished). Holding off on autonomous APPROVE per the CI-must-be-complete rule. I'm 90%+ on the substance — happy to flip to APPROVE once the inspector-relevant shards land green.

Match main"'s layout: invalid-args (commented) → invalid-protocol →
close-worker with no blank line between the two active entries.

Co-authored-by: Divy Srivastava <me@littledivy.com>
Copy link
Copy Markdown
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@littledivy littledivy requested a review from bartlomieju April 29, 2026 03:51
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, verified it works in VSCode

@bartlomieju bartlomieju merged commit 241afbe into denoland:main Apr 29, 2026
136 checks passed
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.

4 participants