fix(core): clean up websocket protocol handling

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Sep 26, 2025 0:42am

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: fix(core): fix websocket protocol for rivetkit

I've completed a comprehensive review of this pull request. Here's my analysis:

Review Progress:

• Read repository guidelines (CLAUDE.md)
• Analyze OpenAPI specification changes
• Review websocket protocol modifications
• Examine router updates
• Analyze actor driver changes
• Review package.json and dependency changes
• Provide comprehensive feedback and recommendations

---

🔍 Summary

This PR fixes the WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across multiple components including the Cloudflare Workers adapter, core router, and engine driver.

✅ Positive Changes

1. Correct Protocol Parsing: The fix for Sec-WebSocket-Protocol parsing in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All three components (Cloudflare Workers, router, engine driver) now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly in router.ts:98 and actor-driver.ts:308.

⚠️ Issues & Concerns

Critical: Missing Error Handling

1. JSON Parsing Vulnerability: In multiple files, JSON.parse(connParamsRaw) is called without try-catch blocks:

   • packages/rivetkit/src/actor/router.ts:105
   • packages/rivetkit/src/drivers/engine/actor-driver.ts:318

   Impact: Malformed JSON in connection parameters will crash the WebSocket connection.

   Recommendation: Add proper error handling:

   const connParams = connParamsRaw ? (() => {
     try {
       return JSON.parse(connParamsRaw);
     } catch (e) {
       logger().warn("invalid connection parameters", { connParamsRaw, error: e });
       return undefined;
     }
   })() : undefined;

Security Concerns

2. Protocol Injection: The protocol parsing loops don't validate protocol format:

   • router.ts:94-100 and actor-driver.ts:304-315

   Risk: Malicious clients could send malformed protocols that might cause issues.

   Recommendation: Add validation:

   // Validate protocol format before processing
   if (protocol.includes('.') && protocol.indexOf('.') > 0) {
     // Process protocol
   }

Code Quality Issues

3. Code Duplication: The protocol parsing logic is nearly identical across three files with slight variations. This violates DRY principles and makes maintenance harder.

   Recommendation: Extract to a shared utility function:

   // In common/websocket-protocol-parser.ts
   export function parseWebSocketProtocols(protocols: string | null) {
     // Shared parsing logic
   }

4. Inconsistent Variable Naming: The token variable is parsed in actor-driver.ts:311 and router.ts:199 but not used, suggesting incomplete implementation.

5. Missing Type Safety: The protocol constants are imported but there's no runtime validation that received protocols match expected formats.

Performance Considerations

6. Repeated String Operations: Multiple .startsWith() calls and string manipulations in loops could be optimized with a single regex or lookup table for better performance with many protocols.

API Design Issues

7. OpenAPI Inconsistency: The OpenAPI change making key parameter optional (clients/openapi/openapi.json:27) seems unrelated to WebSocket protocol fixes and lacks context in this PR.

🧪 Test Coverage Concerns

The changes affect core WebSocket functionality but I found limited test coverage:

• Only 3 test files contain WebSocket-related tests
• No tests specifically validate the protocol parsing logic
• Missing edge case testing for malformed protocols, invalid JSON, etc.

Recommendation: Add comprehensive tests for:

• Protocol parsing with various input formats
• Error handling for malformed JSON
• Security validation of protocol inputs
• Cross-browser compatibility

📋 Recommendations

1. Immediate Fixes (should be addressed before merge):

   • Add try-catch around all JSON.parse() calls
   • Add basic protocol format validation

2. Follow-up Improvements:

   • Extract shared protocol parsing logic
   • Add comprehensive test coverage
   • Document the protocol format expectations
   • Consider adding protocol versioning for future compatibility

3. Documentation: The PR lacks a description. Consider adding context about why this change was needed and what issue it fixes.

🏆 Overall Assessment

While the core fix for protocol parsing is sound and necessary, the implementation has several reliability and security concerns that should be addressed. The architectural approach is good, but execution needs strengthening around error handling and code organization.

Recommendation: Request changes to address critical error handling before merge, with follow-up work for code organization improvements.

---

[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

• chore(core): reimpl reconnect logic #1310
• fix(core): fix getters on ActorConn proxy #1311
• chore(core): simplify connection handling & fix graceful reconnect #1309
• chore(core): clean up token configs #1298
• fix(core): clean up websocket protocol handling #1297 👈 (View in Graphite)
• chore(core): add token & optional headers to client #1296
• feat: serverless #1288
• 09-16-fix_next-js_fix_example_for_next-js #1250
• fix(core): fix missing ManagerDriver impls #1308
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

[pkg-pr-new]

More templates

• example-ai-agent
• example-better-auth-external-db
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• counter
• example-crdt
• example-database
• example-sqlite
• example-elysia
• example-express
• example-game
• example-hono
• example-hono-react
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler-proxy
• example-react
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: c87f833

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: fix(core): fix websocket protocol for rivetkit

I'll provide a comprehensive review focusing on code quality, bugs, performance, security, and test coverage.

Review Progress:

• Read and analyze all changed files
• Review code quality and best practices
• Identify potential bugs and issues
• Assess performance considerations
• Evaluate security concerns
• Analyze test coverage
• Provide comprehensive feedback and recommendations

---

🔍 Summary

This PR fixes WebSocket protocol handling by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across multiple components including the Cloudflare Workers adapter, core router, and engine driver.

✅ Positive Changes

1. Correct Protocol Parsing: The fix for Sec-WebSocket-Protocol parsing in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All components now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly.

---

⚠️ Critical Issues Found

1. Missing Error Handling for JSON.parse() - HIGH PRIORITY

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:324

const connParams = connParamsRaw ? JSON.parse(connParamsRaw) : undefined;

Issue: Malformed JSON in connection parameters will crash the WebSocket connection with an uncaught exception.

Impact: DoS vulnerability - malicious clients can crash connections by sending invalid JSON.

Recommendation: Add proper error handling:

const connParams = connParamsRaw ? (() => {
  try {
    return JSON.parse(connParamsRaw);
  } catch (e) {
    logger().warn("invalid connection parameters", { connParamsRaw, error: e });
    return undefined;
  }
})() : undefined;

2. Duplicate Variable Declaration - COMPILATION ERROR

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:298-303

The variable protocols is declared twice:

const protocols = request.headers.get("sec-websocket-protocol"); // Line 298
// ... 
const protocols = request.headers.get("sec-websocket-protocol"); // Line 303

Impact: This code will fail to compile due to duplicate variable declaration.

Fix: Remove the duplicate line 303 and the unnecessary null check on line 299-300.

---

🔒 Security Concerns

3. Protocol Injection Risk

Locations: Multiple files parsing protocols without validation

Issue: No validation of protocol format before processing. Malicious clients could send specially crafted protocols.

Risk: Potential for injection attacks or unexpected behavior.

Recommendation: Add protocol format validation:

// Validate protocol format before processing
if (protocol.includes('.') && protocol.indexOf('.') > 0) {
  // Process protocol
}

4. Missing Input Sanitization

The decoded connection parameters are passed directly to JSON.parse() without any sanitization or size limits, which could lead to memory exhaustion attacks with large payloads.

---

🏗️ Code Quality Issues

5. Code Duplication - DRY Violation

Locations: Protocol parsing logic is nearly identical across three files:

• packages/rivetkit/src/actor/router.ts:93-102 & 199-212
• packages/rivetkit/src/drivers/engine/actor-driver.ts:309-321

Impact: Makes maintenance harder, increases bug surface area.

Recommendation: Extract to shared utility function:

// In common/websocket-protocol-parser.ts
export function parseWebSocketProtocols(protocols: string | null) {
  // Shared parsing logic
}

6. Inconsistent Variable Usage

The token variable is parsed but never used in actor-driver.ts:318, suggesting incomplete implementation.

---

⚡ Performance Considerations

7. Repeated String Operations

Locations: Multiple .startsWith() calls in loops could be optimized

Impact: Inefficient for connections with many protocols.

Recommendation: Use lookup table or single regex for better performance:

const PROTOCOL_PARSERS = {
  [WS_PROTOCOL_ENCODING]: (protocol) => protocol.substring(WS_PROTOCOL_ENCODING.length),
  [WS_PROTOCOL_CONN_PARAMS]: (protocol) => decodeURIComponent(protocol.substring(WS_PROTOCOL_CONN_PARAMS.length)),
  [WS_PROTOCOL_TOKEN]: (protocol) => protocol.substring(WS_PROTOCOL_TOKEN.length)
};

---

🧪 Test Coverage Analysis

Current Coverage: Limited WebSocket-related tests found, mostly in driver test suites.

Missing Coverage:

• No tests for the new protocol parsing logic
• No error handling tests for malformed JSON
• No security validation tests for protocol inputs
• No tests for edge cases (empty protocols, malformed formats)

Recommendations:

1. Add unit tests for protocol parsing utility functions
2. Add integration tests for WebSocket connection with various protocol combinations
3. Add negative test cases for malformed inputs
4. Add performance tests for protocol parsing with many protocols

---

📋 Recommendations by Priority

Immediate (Must Fix Before Merge):

1. Fix duplicate protocols variable declaration in actor-driver.ts:298-303
2. Add try-catch around JSON.parse() calls to prevent crashes
3. Add basic protocol format validation

High Priority (Should Address Soon):

4. Extract shared protocol parsing logic to reduce duplication
5. Add comprehensive test coverage for new functionality
6. Add input sanitization and size limits for connection parameters

Medium Priority (Follow-up):

7. Optimize protocol parsing performance
8. Add protocol versioning for future compatibility
9. Document protocol format expectations

---

🏆 Overall Assessment

Core Fix Quality: ✅ The fundamental fix for protocol parsing is sound and addresses real issues.

Implementation Quality: ⚠️ Has critical bugs that will prevent compilation and runtime stability.

Security: ⚠️ Missing essential error handling and input validation.

Maintainability: ❌ Significant code duplication that should be addressed.

Recommendation: Request changes - Address compilation errors and critical error handling before merge. The architectural direction is correct, but execution needs improvement.

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: WebSocket Protocol Fix

Review Progress:

• Read and analyze all changed files
• Review WebSocket protocol parsing changes
• Check for code quality issues and best practices
• Identify potential bugs and edge cases
• Assess security concerns
• Evaluate performance implications
• Check test coverage
• Provide comprehensive feedback with recommendations

---

🔧 Summary

This PR fixes WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes address proper parsing of comma-separated protocol lists and create architectural consistency across all components.

✅ Positive Changes

1. Critical Protocol Parsing Bug Fix

Fixed in cloudflare-workers/src/websocket.ts:66-71:

• Before: protocols.includes(WS_PROTOCOL_STANDARD) would incorrectly match partial strings
• After: Proper comma-separated parsing with protocols.split(",").map((x) => x.trim()).includes(WS_PROTOCOL_STANDARD)

This prevents false positives where a protocol containing the target protocol as a substring would incorrectly match.

2. Architectural Consistency

All three components now consistently parse WebSocket subprotocols instead of headers:

• Cloudflare Workers adapter: Uses subprotocol parsing for standard protocol selection
• Core router: Parses encoding and connection parameters from subprotocols (router.ts:88-102)
• Engine driver: Parses encoding, connection params, and token from subprotocols (actor-driver.ts:308-321)

3. Proper Error Handling

The router correctly handles JSON parsing with appropriate error handling (though this appears to be outside the scope of current changes).

⚠️ Issues & Concerns

Code Quality Issues

1. Variable Redeclaration Bug: In actor-driver.ts:298-303, the variable protocols is declared twice:

   // Line 298: First declaration from header
   const protocols = request.headers.get("sec-websocket-protocol");
   if (protocols === null) throw new Error(`Missing sec-websocket-protocol header`);
   
   // Line 303: Second declaration - overwrites the first!
   const protocols = request.headers.get("sec-websocket-protocol");

   Impact: This is redundant and indicates potential copy-paste error.

2. Code Duplication: The protocol parsing logic is repeated across three files with slight variations:

   • router.ts:93-102 (encoding + conn params)
   • router.ts:199-211 (encoding + conn params + token)
   • actor-driver.ts:309-321 (encoding + conn params + token)

3. Unused Variables: The token variable is parsed in multiple locations but not used in the function calls, suggesting incomplete implementation.

Security Concerns

1. No Protocol Format Validation: The parsing loops don't validate protocol format before processing
2. DoS Potential: Large protocol lists or deeply nested parsing could consume excessive resources
3. URL Decoding: decodeURIComponent() is called without error handling - malformed URI encoding could cause crashes

Performance Issues

1. Repeated String Operations: Multiple .startsWith() calls and string manipulations in tight loops
2. Inefficient Parsing: Same protocol parsing logic runs multiple times for each connection

🧪 Test Coverage Analysis

Critical Gap: No tests cover the new WebSocket protocol parsing logic. Found only 3 WebSocket-related test files, none testing protocol parsing.

Missing Test Cases:

• Protocol parsing with various input formats
• Comma-separated protocol handling
• Edge cases (empty protocols, malformed formats)
• Security validation of protocol inputs
• Error handling for malformed URI encoding

📦 Dependency Analysis

The dependency updates look reasonable:

• Engine runner: Updated to newer commit hash
• Hono: Downgraded from ^4.9.8 to ^4.7.0 (unusual - typically upgrades, needs explanation)

🎯 Recommendations

Immediate (Fix before merge):

1. Remove duplicate variable declaration in actor-driver.ts:303
2. Add basic protocol format validation to prevent malformed inputs
3. Add error handling around decodeURIComponent() calls

Short-term improvements:

1. Extract shared protocol parsing logic into a utility function to eliminate duplication
2. Add comprehensive tests for protocol parsing edge cases
3. Document the protocol format expectations in code comments
4. Explain the Hono downgrade - this seems unusual and needs justification

Long-term considerations:

1. Performance optimization using regex or lookup tables instead of multiple string operations
2. Protocol versioning for future compatibility
3. Rate limiting for protocol parsing to prevent DoS

📊 Overall Assessment

The core architectural change is sound and addresses real WebSocket standard compliance issues. The main protocol parsing bug fix is critical and necessary. However, the implementation has code quality issues (duplicate declarations, code duplication) and lacks comprehensive testing.

Verdict: ⚠️ Request Changes - Address code quality issues and add basic error handling before merge. Follow-up with testing improvements.

Priority Issues:

1. Fix duplicate variable declaration (critical)
2. Add error handling for URI decoding (important)
3. Extract shared parsing logic (important for maintainability)

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: WebSocket Protocol Fix

I've completed a thorough review of this pull request that fixes WebSocket protocol handling by migrating from HTTP headers to WebSocket subprotocols. Here's my comprehensive analysis:

Review Progress:

• Read and analyze all changed files
• Review WebSocket protocol parsing changes
• Check for code quality issues and best practices
• Identify potential bugs and edge cases
• Assess security concerns
• Evaluate performance implications
• Check test coverage
• Provide comprehensive feedback with recommendations

---

🔍 Summary

This PR fixes the WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across multiple components including the Cloudflare Workers adapter, core router, and engine driver.

✅ What's Good

1. Correct Protocol Parsing Fix: The fix in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All three components (Cloudflare Workers, router, engine driver) now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly (router.ts:98, actor-driver.ts:324).

4. Dependency Updates: The package.json changes appear to be related dependency updates.

🚨 Critical Issues

1. Missing Error Handling for JSON Parsing

Locations:

• packages/rivetkit/src/actor/router.ts:107
• packages/rivetkit/src/actor/router.ts:216
• packages/rivetkit/src/drivers/engine/actor-driver.ts:333

Issue: JSON.parse(connParamsRaw) is called without try-catch blocks.

Impact: Malformed JSON in connection parameters will crash the WebSocket connection.

Fix Required:

const connParams = connParamsRaw ? (() => {
  try {
    return JSON.parse(connParamsRaw);
  } catch (e) {
    logger().warn("invalid connection parameters", { connParamsRaw, error: e });
    return undefined;
  }
})() : undefined;

2. Bug in Engine Driver

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:307-312

Issue: There are two identical variable declarations for protocols:

const protocols = request.headers.get("sec-websocket-protocol");
if (protocols === null)
  throw new Error(`Missing sec-websocket-protocol header`);

// Parse configuration from Sec-WebSocket-Protocol header
const protocols = request.headers.get("sec-websocket-protocol");  // ← Duplicate!

Fix: Remove the duplicate line 312.

⚠️ Security & Quality Concerns

3. Protocol Injection Vulnerability

Locations: All protocol parsing loops

Issue: No validation of protocol format before processing.

Risk: Malicious clients could send malformed protocols.

Recommendation: Add basic validation:

if (protocol.includes('.') && protocol.indexOf('.') > 0) {
  // Process protocol
}

4. Code Duplication

Issue: Nearly identical protocol parsing logic exists in three files with slight variations. This violates DRY principles.

Recommendation: Extract to shared utility function:

// In common/websocket-protocol-parser.ts
export function parseWebSocketProtocols(protocols: string | null) {
  // Shared parsing logic
}

5. Unused Variables

Locations:

• router.ts:197 (token variable parsed but not used)
• actor-driver.ts:315 (token variable parsed but not used)

Issue: Variables are parsed but never used, suggesting incomplete implementation or dead code.

🧪 Test Coverage Issues

Current State:

• Found comprehensive WebSocket tests in packages/rivetkit/src/driver-test-suite/tests/raw-websocket.ts
• Tests cover basic functionality, echoing, binary data, connection tracking
• Missing: No tests specifically for the protocol parsing logic introduced in this PR

Gaps:

• No validation of protocol parsing with malformed protocols
• No tests for JSON parsing error handling
• No edge case testing (empty protocols, invalid JSON, etc.)
• No security validation testing

Recommendation: Add tests for:

test("should handle malformed WebSocket protocols gracefully", () => {
  // Test invalid protocol formats
  // Test malformed JSON in connection parameters
  // Test protocol injection attempts
});

🏎️ Performance Considerations

Minor Issues:

• Multiple .startsWith() calls in loops could be optimized with regex or lookup table
• Repeated string operations for protocol parsing

Impact: Low - only affects WebSocket connection establishment, not message throughput.

📊 Overall Assessment

Severity Breakdown:

• 🔴 Critical: 2 issues (JSON parsing error handling, duplicate variable bug)
• 🟡 Medium: 3 issues (code duplication, protocol validation, unused variables)
• 🟢 Minor: 2 issues (test coverage, performance optimizations)

🎯 Recommendations

Must Fix Before Merge:

1. Add try-catch around all JSON.parse() calls
2. Remove duplicate protocols variable declaration in engine driver
3. Add basic protocol format validation

Should Fix Soon:
4. Extract shared protocol parsing logic to eliminate duplication
5. Add comprehensive tests for new protocol parsing
6. Remove or use the parsed token variables

Nice to Have:
7. Performance optimizations for protocol parsing
8. Enhanced security validation

Verdict: Request changes to address critical issues before merge, with follow-up work for code organization improvements.

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: WebSocket Protocol Fix

I've completed a comprehensive review of this pull request. Here's my analysis:

Review Progress:

• Read and analyze all changed files
• Review WebSocket protocol parsing changes
• Check for code quality issues and best practices
• Identify potential bugs and edge cases
• Assess security concerns
• Evaluate performance implications
• Check test coverage
• Provide comprehensive feedback with recommendations

---

🔍 Summary

This PR fixes the WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across the Cloudflare Workers adapter (packages/cloudflare-workers/src/websocket.ts:66-71), core router (packages/rivetkit/src/actor/router.ts), and engine driver (packages/rivetkit/src/drivers/engine/actor-driver.ts).

✅ Positive Changes

1. Correct Protocol Parsing: The fix in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All three components now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly in router.ts:98 and actor-driver.ts:324.

⚠️ Critical Issues

1. Missing Error Handling for JSON Parsing

Location: Multiple files contain unprotected JSON.parse() calls:

• packages/rivetkit/src/actor/router.ts:107
• packages/rivetkit/src/actor/router.ts:216
• packages/rivetkit/src/drivers/engine/actor-driver.ts:333

Impact: Malformed JSON in connection parameters will crash the WebSocket connection.

Recommendation: Add proper error handling:

const connParams = connParamsRaw ? (() => {
  try {
    return JSON.parse(connParamsRaw);
  } catch (e) {
    logger().warn("invalid connection parameters", { connParamsRaw, error: e });
    return undefined;
  }
})() : undefined;

2. Duplicate Variable Declaration

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:307-312

There's a duplicate declaration of protocols variable that will cause a compilation error:

const protocols = request.headers.get("sec-websocket-protocol");
// ... 
const protocols = request.headers.get("sec-websocket-protocol"); // Line 312 - duplicate!

3. Code Duplication

Issue: The protocol parsing logic is nearly identical across three files with slight variations, violating DRY principles and making maintenance harder.

Recommendation: Extract to a shared utility function in common/websocket-protocol-parser.ts.

🔒 Security Concerns

1. Protocol Injection: The protocol parsing loops don't validate protocol format, allowing potential injection attacks.
2. No Input Validation: Missing validation that received protocols match expected formats.

📈 Performance Issues

1. Repeated String Operations: Multiple .startsWith() calls and string manipulations in loops could be optimized with regex or lookup tables.
2. Inefficient Protocol Parsing: Each protocol list is split and parsed multiple times.

🧪 Test Coverage Analysis

Current State: Limited test coverage for WebSocket protocol parsing

• Found multiple JSON.parse() calls in test files but no specific tests for the new protocol parsing logic
• No edge case testing for malformed protocols, invalid JSON, or security validation
• WebSocket tests exist but don't cover the protocol parsing changes

Recommendation: Add comprehensive tests for:

• Protocol parsing with various input formats
• Error handling for malformed JSON
• Security validation of protocol inputs
• Cross-browser compatibility

📋 Detailed Findings by File

packages/cloudflare-workers/src/websocket.ts

• ✅ Good: Fixed protocol parsing with proper comma handling
• ✅ Good: Maintains backward compatibility

packages/rivetkit/src/actor/router.ts

• ⚠️ Issue: Unprotected JSON.parse() calls (lines 107, 216)
• ✅ Good: Consistent protocol parsing approach
• ℹ️ Note: Code duplication with other files

packages/rivetkit/src/drivers/engine/actor-driver.ts

• 🚨 Critical: Duplicate variable declaration (lines 307, 312)
• ⚠️ Issue: Unprotected JSON.parse() call (line 333)
• ✅ Good: URL decoding for parameters

🎯 Recommendations

Immediate Fixes (should be addressed before merge):

1. Fix duplicate variable declaration in actor-driver.ts:312
2. Add try-catch around all JSON.parse() calls
3. Add basic protocol format validation

Follow-up Improvements:

1. Extract shared protocol parsing logic to reduce code duplication
2. Add comprehensive test coverage for protocol parsing
3. Implement input validation for security
4. Consider adding protocol versioning for future compatibility

🏆 Overall Assessment

The core fix for protocol parsing is sound and addresses a real issue. However, the implementation has several critical bugs and reliability concerns that must be addressed before merging. The architectural approach is good, but execution needs strengthening around error handling and code organization.

Recommendation: ⚠️ Request changes to address critical issues, particularly the compilation error and missing error handling.

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: WebSocket Protocol Fix

I've completed a comprehensive review of this pull request that fixes WebSocket protocol handling by migrating from HTTP headers to WebSocket subprotocols. Here's my detailed analysis:

Review Progress:

• Read repository guidelines (CLAUDE.md)
• Analyze OpenAPI specification changes
• Review websocket protocol modifications
• Examine router updates
• Analyze actor driver changes
• Review package.json and dependency changes
• Provide comprehensive feedback and recommendations

---

🔍 Summary

This PR fixes the WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across multiple components including the Cloudflare Workers adapter, core router, and engine driver.

✅ Positive Changes

1. Correct Protocol Parsing: The fix in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All three components (Cloudflare Workers, router, engine driver) now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly in router.ts:98 and actor-driver.ts:324.

⚠️ Critical Issues

1. Missing Error Handling for JSON Parsing

Locations:

• packages/rivetkit/src/actor/router.ts:107
• packages/rivetkit/src/drivers/engine/actor-driver.ts:333

Issue: JSON.parse(connParamsRaw) is called without try-catch blocks.

Impact: Malformed JSON in connection parameters will crash the WebSocket connection.

Fix Required:

const connParams = connParamsRaw ? (() => {
  try {
    return JSON.parse(connParamsRaw);
  } catch (e) {
    logger().warn("invalid connection parameters", { connParamsRaw, error: e });
    return undefined;
  }
})() : undefined;

2. Bug in Engine Driver

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:307-312

Issue: There are two identical variable declarations for protocols:

const protocols = request.headers.get("sec-websocket-protocol");
if (protocols === null)
  throw new Error(`Missing sec-websocket-protocol header`);

// Parse configuration from Sec-WebSocket-Protocol header
const protocols = request.headers.get("sec-websocket-protocol");  // ← Duplicate!

Fix: Remove the duplicate line 312.

🔒 Security & Quality Concerns

3. Protocol Injection Vulnerability

Locations: All protocol parsing loops

Issue: No validation of protocol format before processing.

Risk: Malicious clients could send malformed protocols.

Recommendation: Add basic validation:

if (protocol.includes('.') && protocol.indexOf('.') > 0) {
  // Process protocol
}

4. Code Duplication

Issue: Nearly identical protocol parsing logic exists in three files with slight variations. This violates DRY principles.

Recommendation: Extract to shared utility function:

// In common/websocket-protocol-parser.ts
export function parseWebSocketProtocols(protocols: string | null) {
  // Shared parsing logic
}

5. Unused Variables

Locations:

• router.ts:197 (token variable parsed but not used)
• actor-driver.ts:315 (token variable parsed but not used)

Issue: Variables are parsed but never used, suggesting incomplete implementation or dead code.

🧪 Test Coverage Analysis

Current State:

• Found comprehensive WebSocket tests in packages/rivetkit/src/driver-test-suite/tests/raw-websocket.ts
• Tests cover basic functionality, echoing, binary data, connection tracking
• Missing: No tests specifically for the protocol parsing logic introduced in this PR

Gaps:

• No validation of protocol parsing with malformed protocols
• No tests for JSON parsing error handling
• No edge case testing (empty protocols, invalid JSON, etc.)
• No security validation testing

Recommendation: Add tests for:

test("should handle malformed WebSocket protocols gracefully", () => {
  // Test invalid protocol formats
  // Test malformed JSON in connection parameters
  // Test protocol injection attempts
});

🏎️ Performance Considerations

Minor Issues:

• Multiple .startsWith() calls in loops could be optimized with regex or lookup table
• Repeated string operations for protocol parsing

Impact: Low - only affects WebSocket connection establishment, not message throughput.

📋 Dependency Analysis

Changes in package.json:

• Engine runner: Updated to newer commit hash (@rivetkit/engine-runner)
• Hono: Dependency version appears consistent with peer dependencies

📊 Overall Assessment

Severity Breakdown:

• 🔴 Critical: 2 issues (JSON parsing error handling, duplicate variable bug)
• 🟡 Medium: 3 issues (code duplication, protocol validation, unused variables)
• 🟢 Minor: 2 issues (test coverage, performance optimizations)

🎯 Recommendations

Must Fix Before Merge:

1. Add try-catch around all JSON.parse() calls
2. Remove duplicate protocols variable declaration in engine driver
3. Add basic protocol format validation

Should Fix Soon:
4. Extract shared protocol parsing logic to eliminate duplication
5. Add comprehensive tests for new protocol parsing
6. Remove or use the parsed token variables

Nice to Have:
7. Performance optimizations for protocol parsing
8. Enhanced security validation

Verdict: ⚠️ Request changes to address critical issues before merge, with follow-up work for code organization improvements.

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: WebSocket Protocol Fix

I've completed a comprehensive review of this WebSocket protocol handling fix, focusing on code quality, bugs, performance, security, and test coverage.

Review Progress:

• Read repository guidelines (CLAUDE.md)
• Analyze WebSocket protocol parsing changes
• Review code quality and best practices
• Identify potential bugs and issues
• Assess performance considerations
• Evaluate security concerns
• Check test coverage
• Provide comprehensive feedback and recommendations

---

🔍 Summary

This PR fixes the WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across multiple components including the Cloudflare Workers adapter (packages/cloudflare-workers/src/websocket.ts:66-71), core router (packages/rivetkit/src/actor/router.ts), and engine driver (packages/rivetkit/src/drivers/engine/actor-driver.ts).

✅ Positive Changes

1. Critical Protocol Parsing Fix: The fix in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All three components now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly in router.ts:98 and actor-driver.ts:324.

---

🚨 Critical Issues

1. Missing Error Handling for JSON Parsing

Locations:

• packages/rivetkit/src/actor/router.ts:107
• packages/rivetkit/src/drivers/engine/actor-driver.ts:333

Issue: JSON.parse(connParamsRaw) is called without try-catch blocks.

Impact: Malformed JSON in connection parameters will crash the WebSocket connection.

Fix Required:

const connParams = connParamsRaw ? (() => {
  try {
    return JSON.parse(connParamsRaw);
  } catch (e) {
    logger().warn("invalid connection parameters", { connParamsRaw, error: e });
    return undefined;
  }
})() : undefined;

2. Bug in Engine Driver

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:307-312

Issue: There are two identical variable declarations for protocols:

const protocols = request.headers.get("sec-websocket-protocol");
if (protocols === null)
  throw new Error(`Missing sec-websocket-protocol header`);

// Parse configuration from Sec-WebSocket-Protocol header
const protocols = request.headers.get("sec-websocket-protocol");  // ← Duplicate!

Fix: Remove the duplicate line 312.

---

🔒 Security & Quality Concerns

3. Protocol Injection Vulnerability

Locations: All protocol parsing loops

Issue: No validation of protocol format before processing.

Risk: Malicious clients could send malformed protocols.

Recommendation: Add basic validation:

if (protocol.includes('.') && protocol.indexOf('.') > 0) {
  // Process protocol
}

4. Code Duplication

Issue: Nearly identical protocol parsing logic exists in three files with slight variations. This violates DRY principles.

Recommendation: Extract to shared utility function:

// In common/websocket-protocol-parser.ts
export function parseWebSocketProtocols(protocols: string | null) {
  // Shared parsing logic
}

5. Unused Variables

Locations:

• router.ts:197 (token variable parsed but not used)
• actor-driver.ts:315 (token variable parsed but not used)

Issue: Variables are parsed but never used, suggesting incomplete implementation or dead code.

---

🧪 Test Coverage Analysis

Current State:

• Found comprehensive WebSocket tests in packages/rivetkit/src/driver-test-suite/tests/raw-websocket.ts
• Tests cover basic functionality, echoing, binary data, connection tracking
• Missing: No tests specifically for the protocol parsing logic introduced in this PR

Gaps:

• No validation of protocol parsing with malformed protocols
• No tests for JSON parsing error handling
• No edge case testing (empty protocols, invalid JSON, etc.)
• No security validation testing

Recommendation: Add tests for:

test("should handle malformed WebSocket protocols gracefully", () => {
  // Test invalid protocol formats
  // Test malformed JSON in connection parameters
  // Test protocol injection attempts
});

---

🏎️ Performance Considerations

Minor Issues:

• Multiple .startsWith() calls in loops could be optimized with regex or lookup table
• Repeated string operations for protocol parsing

Impact: Low - only affects WebSocket connection establishment, not message throughput.

---

📋 Dependency Analysis

Changes in package.json:

• Engine runner: Updated to newer commit hash (@rivetkit/engine-runner)
• Hono: Dependency version appears consistent with peer dependencies

---

📊 Overall Assessment

Severity Breakdown:

• 🔴 Critical: 2 issues (JSON parsing error handling, duplicate variable bug)
• 🟡 Medium: 3 issues (code duplication, protocol validation, unused variables)
• 🟢 Minor: 2 issues (test coverage, performance optimizations)

🎯 Recommendations

Must Fix Before Merge:

1. Add try-catch around all JSON.parse() calls
2. Remove duplicate protocols variable declaration in engine driver
3. Add basic protocol format validation

Should Fix Soon:
4. Extract shared protocol parsing logic to eliminate duplication
5. Add comprehensive tests for new protocol parsing
6. Remove or use the parsed token variables

Nice to Have:
7. Performance optimizations for protocol parsing
8. Enhanced security validation

Verdict: ⚠️ Request changes to address critical issues before merge, with follow-up work for code organization improvements.

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: WebSocket Protocol Fix

I've completed a comprehensive review of this pull request that fixes WebSocket protocol handling by migrating from HTTP headers to WebSocket subprotocols. Here's my detailed analysis:

Review Progress:

• Read repository guidelines (CLAUDE.md)
• Analyze all changed files
• Review code quality and best practices
• Identify potential bugs and issues
• Assess performance considerations
• Evaluate security concerns
• Check test coverage
• Provide comprehensive feedback and recommendations

---

🔍 Summary

This PR fixes the WebSocket protocol handling in RivetKit by migrating from HTTP headers to WebSocket subprotocols for passing connection parameters. The changes span across the Cloudflare Workers adapter (packages/cloudflare-workers/src/websocket.ts:66-71), core router (packages/rivetkit/src/actor/router.ts), and engine driver (packages/rivetkit/src/drivers/engine/actor-driver.ts).

✅ Positive Changes

1. Correct Protocol Parsing: The fix in packages/cloudflare-workers/src/websocket.ts:66-71 is excellent. Previously using protocols.includes() would match partial strings incorrectly. The new approach with split(",").map(x => x.trim()).includes() properly handles comma-separated protocol lists.

2. Consistent Architecture: All three components now consistently parse WebSocket subprotocols instead of headers, creating architectural alignment.

3. Proper URL Decoding: The use of decodeURIComponent() for connection parameters ensures special characters are handled correctly in router.ts:98 and actor-driver.ts:324.

⚠️ Critical Issues

1. Missing Error Handling for JSON Parsing

Location: Multiple files contain unprotected JSON.parse() calls:

• packages/rivetkit/src/actor/router.ts:107
• packages/rivetkit/src/drivers/engine/actor-driver.ts:333

Impact: Malformed JSON in connection parameters will crash the WebSocket connection.

Recommendation: Add proper error handling:

const connParams = connParamsRaw ? (() => {
  try {
    return JSON.parse(connParamsRaw);
  } catch (e) {
    logger().warn("invalid connection parameters", { connParamsRaw, error: e });
    return undefined;
  }
})() : undefined;

2. Duplicate Variable Declaration

Location: packages/rivetkit/src/drivers/engine/actor-driver.ts:307-312

There's a duplicate declaration of protocols variable that will cause a compilation error:

const protocols = request.headers.get("sec-websocket-protocol");
// ... 
const protocols = request.headers.get("sec-websocket-protocol"); // Line 312 - duplicate!

3. Code Duplication

Issue: The protocol parsing logic is nearly identical across three files with slight variations, violating DRY principles and making maintenance harder.

Recommendation: Extract to a shared utility function in common/websocket-protocol-parser.ts.

🔒 Security Concerns

1. Protocol Injection: The protocol parsing loops don't validate protocol format, allowing potential injection attacks.
2. No Input Validation: Missing validation that received protocols match expected formats.

📈 Performance Issues

1. Repeated String Operations: Multiple .startsWith() calls and string manipulations in loops could be optimized with regex or lookup tables.
2. Inefficient Protocol Parsing: Each protocol list is split and parsed multiple times.

🧪 Test Coverage Analysis

Current State: Limited test coverage for WebSocket protocol parsing

• Found comprehensive WebSocket tests in packages/rivetkit/src/driver-test-suite/tests/raw-websocket.ts
• Tests cover basic functionality, echoing, binary data, connection tracking
• Missing: No tests specifically for the protocol parsing logic introduced in this PR

Gaps:

• No validation of protocol parsing with malformed protocols
• No tests for JSON parsing error handling
• No edge case testing (empty protocols, invalid JSON, etc.)
• No security validation testing

Recommendation: Add comprehensive tests for:

• Protocol parsing with various input formats
• Error handling for malformed JSON
• Security validation of protocol inputs
• Cross-browser compatibility

📋 Detailed Findings by File

packages/cloudflare-workers/src/websocket.ts

• ✅ Good: Fixed protocol parsing with proper comma handling
• ✅ Good: Maintains backward compatibility

packages/rivetkit/src/actor/router.ts

• ⚠️ Issue: Unprotected JSON.parse() call (line 107)
• ✅ Good: Consistent protocol parsing approach
• ℹ️ Note: Code duplication with other files

packages/rivetkit/src/drivers/engine/actor-driver.ts

• 🚨 Critical: Duplicate variable declaration (lines 307, 312)
• ⚠️ Issue: Unprotected JSON.parse() call (line 333)
• ✅ Good: URL decoding for parameters

🎯 Recommendations

Immediate Fixes (should be addressed before merge):

1. Fix duplicate variable declaration in actor-driver.ts:312
2. Add try-catch around all JSON.parse() calls
3. Add basic protocol format validation

Follow-up Improvements:

1. Extract shared protocol parsing logic to reduce code duplication
2. Add comprehensive test coverage for protocol parsing
3. Implement input validation for security
4. Consider adding protocol versioning for future compatibility

🏆 Overall Assessment

The core fix for protocol parsing is sound and addresses a real issue. However, the implementation has several critical bugs and reliability concerns that must be addressed before merging. The architectural approach is good, but execution needs strengthening around error handling and code organization.

Recommendation: ⚠️ Request changes to address critical issues, particularly the compilation error and missing error handling.

---

[graphite-app]

Merge activity

• Sep 26, 7:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 26, 7:06 AM UTC: CI is running for this pull request on a draft pull request (#1312) due to your merge queue CI optimization settings.
• Sep 26, 7:07 AM UTC: Merged by the Graphite merge queue via draft PR: #1312.