fix(rivetkit): fix routing of /websocket

[NathanFlurry]

No description provided.

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(rivetkit/next-js): fix request handler #4001
• fix(rivetkit): handle undefined options with scheduling #4000
• chore(engine): add force shutdown timeout #3999
• fix(rivetkit): fix fetch handler in nextjs serverless #3998
• fix(rivetkit): add back dev hot reloading in next #3997
• fix(rivetkit): fix routing of /websocket #3995 👈 (View in Graphite)
• fix(rivetkit): add config parameters to spawned serverless engine for local dev #3994
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Code Review - PR #3995: fix routing of /websocket

Summary

This PR fixes a bug where WebSocket routing failed to match paths like /websocket?token=abc because query parameters were included in the path comparison. The fix strips query parameters before routing decisions while preserving them for the actual request handling.

Strengths

1. Correct Root Cause Analysis: The fix properly identifies that query parameters should be stripped for routing comparisons while being preserved for the actual request.

2. Comprehensive Test Coverage: Unit tests verify the routing logic in isolation, integration tests verify end-to-end behavior, and tests cover the critical edge case: base path with query params.

3. Minimal Impact: The change is surgical - only affecting path comparison logic, not the actual request handling.

4. Good Documentation: The inline comments explain the why clearly.

Observations and Suggestions

1. Query Parameter Handling Method: The current implementation uses split on question mark which is simple but has edge cases. Consider using the URL API for consistency with truncateRawWebSocketPathPrefix (lines 407-416). However, the current approach is likely sufficient since WebSocket paths are controlled upstream.

2. Unit Test Isolation: The unit test duplicates the routing logic rather than testing the actual implementation. Consider extracting a testable helper function or relying on the integration tests which already validate this end-to-end.

3. Error Message Consistency: Line 146 throws an error with the original requestPath which shows query parameters. Consider showing the normalized path for better debugging.

4. Test Assertion Specificity: The driver test assertions use toContain for query parameters. More specific assertions using URL.searchParams would be more robust, but current assertions are sufficient.

Security Considerations

Query Parameter Injection: No issues identified - query parameters are preserved correctly and routing logic only affects path matching.

Path Traversal: No new risks - the change does not affect path validation.

Performance Considerations

String splitting: The split operation is very lightweight and happens once per WebSocket connection. No performance concerns.

Test Coverage

Excellent coverage including base path without query, base path with query (the bug being fixed), subpath with query, trailing slash, and integration test validating query params are preserved.

Code Style and Conventions

Follows CLAUDE.md conventions (hard tabs, lowercase log messages), comments are complete sentences, variable names are clear and descriptive, and test descriptions follow existing patterns.

Final Verdict

LGTM with minor suggestions. This is a solid bug fix with good test coverage. The implementation is correct and minimal. The suggestions above are optional improvements for consistency and robustness, not blockers.

Recommended next steps:

1. Merge as-is (fully functional)
2. Optional: Consider the URL API approach for consistency
3. Optional: Make error messages show normalized paths for better debugging

Great work on the thorough testing, especially the unit tests documenting the expected behavior!

[claude]

PR Review: Fix WebSocket routing with query parameters

Overview

This PR fixes a bug in WebSocket routing where requests to /websocket?query=value would fail to route correctly. The fix strips query parameters before path comparison in the routing logic.

✅ Strengths

1. Excellent bug identification: The PR correctly identifies that while the FileSystem driver happened to work (because it strips query params before calling routeWebSocket), the Engine driver passes requestPath directly, making it vulnerable to this bug (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:569).

2. Comprehensive test coverage:

   • Added unit tests to verify the routing logic independently (router-websocket-endpoints.test.ts)
   • Added integration test to verify the end-to-end behavior (raw-websocket.ts)
   • Tests cover all the edge cases: base path with query, subpaths with query, trailing slashes

3. Clear documentation: The comments explain the fix well and note the distinction between drivers that were already working vs. the actual bug in routeWebSocket.

4. Minimal, focused fix: The change is surgical - only strips query params where needed for routing decisions, preserving the full path for actual handler usage.

🔍 Code Quality Observations

Good:

• Consistent application of the fix to all path comparisons (PATH_CONNECT, PATH_WEBSOCKET_BASE/PREFIX, PATH_INSPECTOR_CONNECT)
• Uses simple .split("?")[0] which handles both cases (with and without query params)
• Test helper function matchesWebSocketPath matches the actual implementation logic

Minor Suggestions:

1. Potential edge case in query splitting: The current implementation uses requestPath.split("?")[0]. This works correctly but could be slightly more explicit. Consider what happens with:

   • Multiple ? characters: /websocket?foo=bar?baz=qux → would give /websocket (correct behavior)
   • Hash fragments: /websocket#section → would keep the hash (but this is unlikely in WebSocket URLs)

   The current implementation is fine, but if you want to be more defensive, you could use:

   const requestPathWithoutQuery = requestPath.split("?")[0].split("#")[0];

   However, this is likely unnecessary for WebSocket requests.

2. Test coverage suggestion: The test comment mentions "NOTE: The driver-file-system end-to-end tests pass because..." - this is excellent context. Consider adding a test that verifies query parameters are still accessible to the WebSocket handler after routing (to ensure we're only stripping for routing purposes, not losing the data).

🐛 Potential Issues

None identified. The fix is sound and correctly addresses the root cause.

⚡ Performance

No performance concerns. The .split("?")[0] operation is trivial and happens once per WebSocket connection.

🔒 Security

No security concerns. The fix actually improves security by ensuring routing works consistently regardless of query parameter presence, which could otherwise lead to unexpected routing failures.

📊 Test Coverage

Excellent test coverage:

• ✅ Unit tests for routing logic
• ✅ Integration tests for end-to-end behavior
• ✅ Edge cases covered (base path, subpaths, query params, trailing slashes)

📝 Documentation

The inline comments are clear and explain the "why" behind the fix. The test comment provides valuable context about the different driver behaviors.

✨ Overall Assessment

Strong approval. This is a well-crafted fix that:

• Solves a real bug
• Includes comprehensive tests
• Is minimal and focused
• Has clear documentation

The fix correctly handles the routing issue while preserving query parameters for the actual handlers to use.

Recommendation

Approve and merge - This fix is ready to go. The minor suggestions above are optional improvements, not blockers.

[pkg-pr-new]

More templates

• actor-actions
• example-ai-agent
• example-ai-and-user-generated-actors-freestyle
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• cross-actor-actions
• example-cursors
• example-cursors-raw-websocket
• custom-serverless
• example-sqlite
• example-elysia
• example-experimental-durable-streams-ai-agent
• hello-world
• example-hono
• hono-react
• example-kitchen-sink
• multi-region
• native-websockets
• next-js
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• scheduling
• state
• example-stream
• example-trpc

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3995

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3995

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3995

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3995

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3995

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3995

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3995

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3995

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3995

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3995

commit: b7a6ac3

[graphite-app]

The imports are not properly sorted. Biome linter typically expects imports to be sorted with internal imports (starting with '@/') coming before external module imports (like 'vitest'). Swap the order of these import statements to fix the linting error.

Spotted by Graphite Agent (based on CI logs)



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Merge activity

• Jan 22, 6:58 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Jan 22, 6:59 AM UTC: CI is running for this pull request on a draft pull request (#4007) due to your merge queue CI optimization settings.
• Jan 22, 7:00 AM UTC: Merged by the Graphite merge queue via draft PR: #4007.