chore(guard): add support for routing runner ws to `/runners/connect` #3340

NathanFlurry · 2025-11-04T17:45:35Z

No description provided.

vercel · 2025-11-04T17:45:38Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 4, 2025 8:26pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 4, 2025 8:26pm
rivet-inspector	Ignored	Preview	Nov 4, 2025 8:26pm
rivet-site	Ignored	Preview	Nov 4, 2025 8:26pm

NathanFlurry · 2025-11-04T17:46:21Z

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

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 · 2025-11-04T17:47:08Z

Code Review: Add support for routing runner ws to `/runners/connect`

Summary

This PR adds path-based routing for runner WebSocket connections via /runners/connect, complementing the existing header-based routing (X-Rivet-Target: runner). The implementation refactors common logic into a shared internal function and updates the TypeScript SDK to use the new path-based endpoint.

Code Quality & Best Practices

✅ Strengths

Good code organization: The refactoring extracts common logic into route_runner_internal(), eliminating duplication between header-based and path-based routing
Clear separation of concerns: Path-based and header-based routing are now distinct functions with clear responsibilities
Consistent patterns: The implementation follows the same pattern used for actor routing (see parse_actor_path and pegboard_gateway::route_request_path_based)
Proper structured logging: Uses tracing with structured fields (e.g., ?host, path) rather than formatting into the message, following the codebase conventions from CLAUDE.md
Documentation improvements: Updated comments are clearer (e.g., "MARK: Path-based routing" vs the previous comments)

🔍 Observations

TypeScript SDK Changes (engine/sdks/typescript/runner/src/mod.ts:454-469)

The removal of rivet_target.runner protocol makes sense since we're now using path-based routing
URL construction properly handles trailing slashes
Query parameters are preserved correctly

Routing Logic (engine/packages/guard/src/routing/runner.rs)

The path matching is strict (path != "/runners/connect"), which is appropriate
Both routing methods share authentication, host validation, and WebSocket detection logic via route_runner_internal()

Potential Issues

⚠️ Minor Concerns

Path matching strictness
- Line 37: The path comparison if path != "/runners/connect" is exact match only
- This means /runners/connect/ (with trailing slash) won't match
- Consider if this is intentional or if you should handle trailing slashes
```
// Current:
if path != "/runners/connect" {

// Consider:
if !path.trim_end_matches('/').eq("/runners/connect") {
```
Query parameter handling
- The path check doesn't account for query parameters
- If a request comes to /runners/connect?foo=bar, it won't match
- However, looking at line 462 in the TS SDK, query params are added, so the actual path should be parsed differently
- Recommendation: Extract the path portion before the query string, similar to how parse_actor_path handles it (lines 156-174 in mod.rs)
```
// Suggested approach:
let base_path = path.split('?').next().unwrap_or(path);
if base_path != "/runners/connect" {
    return Ok(None);
}
```
Backward compatibility
- The header-based routing (X-Rivet-Target: runner) is still supported, which is good
- However, older SDK versions that used rivet_target.runner protocol won't work anymore
- Question: Is there a deprecation plan or version compatibility matrix documented?

Performance Considerations

✅ Good:

Path-based routing happens before header parsing, which is efficient
No regex or complex parsing needed for this simple path match
Shared internal function avoids code duplication and keeps binary size down

Security Concerns

✅ Security looks solid:

Host validation: route_runner_internal() validates the host against the current datacenter's valid hosts (line 54-75)
Authentication: Token validation is preserved for both routing methods (lines 88-124)
WebSocket protocol handling: Properly extracts tokens from WebSocket protocols or headers depending on connection type
No new attack surface: The path-based routing goes through the same validation as header-based routing

🔒 Recommendations:

The current path matching is not vulnerable to path traversal since it's an exact match
Consider logging authentication failures for monitoring (currently only logs success at line 123)

Test Coverage

⚠️ Missing Tests:

No test files found in the guard package
Recommendation: Add integration tests covering:
1. Path-based routing with /runners/connect
2. Path-based routing with trailing slash /runners/connect/
3. Path-based routing with query parameters /runners/connect?foo=bar
4. Invalid paths that shouldn't match
5. Backward compatibility with header-based routing
6. Authentication failure scenarios

Example test structure:

#[cfg(test)]
mod tests {
    use super::*;
    
    #[tokio::test]
    async fn test_route_runner_path_based_exact_match() {
        // Test exact path match
    }
    
    #[tokio::test]
    async fn test_route_runner_path_based_with_query_params() {
        // Test path with query parameters
    }
    
    #[tokio::test]
    async fn test_route_runner_path_based_trailing_slash() {
        // Test path with trailing slash
    }
}

Overall Assessment

This is a solid refactoring that improves the routing architecture by:

Adding more flexible path-based routing
Reducing code duplication
Following established patterns in the codebase

Action Items:

High Priority: Fix query parameter handling in path matching
Medium Priority: Clarify trailing slash handling behavior
Medium Priority: Add test coverage for the new routing logic
Low Priority: Document backward compatibility considerations for SDK versions

Recommendation: ✅ Approve with minor fixes

The core logic is sound and follows best practices. The query parameter handling should be addressed before merging to ensure the SDK's URL construction (which includes query params) works correctly with the routing logic.

MasterPtato

Is the token in path not supported for runners like it is for actors?

claude · 2025-11-04T18:40:19Z

PR Review: Add support for routing runner WebSocket to `/runners/connect`

Summary

This PR adds path-based routing for runner WebSocket connections alongside the existing header-based routing. The changes allow runners to connect via the path /runners/connect instead of requiring the x-rivet-target header or WebSocket protocol.

Code Quality ✅

Strengths:

Good refactoring: The extraction of common logic into route_runner_internal() follows the DRY principle and makes the code more maintainable
Consistent patterns: The implementation mirrors the existing pegboard_gateway path-based routing structure
Clear documentation: Function comments clearly describe the routing methods and matched paths
Proper error handling: All error cases are handled consistently with existing patterns

Style adherence:

✅ Uses anyhow::* import as specified in CLAUDE.md
✅ Returns Result<T> appropriately
✅ Structured logging with tracing::debug!(?host, path, ...) instead of formatting into messages
✅ Comments use MARK annotations for code organization

Potential Issues & Suggestions

1. Path validation could be more defensive (Minor)

In runner.rs:37, the path parsing logic:

let path_without_query = path.split('?').next().unwrap_or(path);

While unwrap_or(path) makes this safe, consider if fragment handling is needed (similar to the actor path parsing which handles both ? and #). The current implementation is likely fine for runner connections, but worth confirming that fragments are never present or should be ignored.

2. WebSocket protocol removal impact (Requires verification)

In mod.ts:467-469, the removal of rivet_target.runner from the protocols array:

const protocols = ["rivet"];
// Previously: ["rivet", "rivet_target.runner"]

Question: Are there any existing deployments or clients that expect the rivet_target.runner protocol? This could be a breaking change if:

Old runners are still deployed using header-based routing
The system needs to support both routing methods during a transition period

Recommendation: Verify that this change is deployed atomically or that there's no transition period where old runners might fail.

3. URL construction edge case (Minor)

In mod.ts:458-462, the URL normalization:

const baseUrl = wsEndpoint.endsWith("/")
    ? wsEndpoint.slice(0, -1)
    : wsEndpoint;
return `${baseUrl}/runners/connect?...`;

This correctly handles trailing slashes, but consider if wsEndpoint could contain a path already (e.g., wss://example.com/some/path/). The current implementation would create wss://example.com/some/path/runners/connect. If this is intentional, add a comment explaining it.

Security Considerations ✅

Authentication preserved: The token validation logic remains intact in route_runner_internal()
Host validation maintained: Regional host checking is still enforced
No new attack surface: Path-based routing uses the same security checks as header-based routing

Performance Considerations ✅

Minimal overhead: Path-based check happens early in the routing pipeline
Efficient string comparison: Uses simple equality check on the path without complex parsing
No regression: Header-based routing fallback is preserved

Test Coverage ⚠️

Missing tests:
The guard package appears to have no test files (checked with glob pattern). Consider adding:

Unit tests for route_request_path_based():
- Valid path: /runners/connect
- Invalid paths: /runners, /runners/connect/extra, /runner/connect
- Path with query params: /runners/connect?foo=bar
- Path with trailing slash: /runners/connect/
Integration tests:
- Path-based routing takes precedence over header-based
- WebSocket upgrade works with new path
- Authentication enforcement with path-based routing
TypeScript SDK tests:
- URL construction with various endpoint formats
- Protocol negotiation without rivet_target.runner

Recommendation: Add test coverage in a follow-up PR or before merging.

Documentation 📝

Consider updating:

API documentation: Document the new /runners/connect endpoint
Migration guide: If this changes client behavior, document the transition
Architecture docs: Update any routing diagrams or documentation

Questions for Author

Is the removal of rivet_target.runner protocol intentional and backwards-compatible?
Should /runners/connect/ (with trailing slash) be supported?
Are there any OpenAPI/Swagger specs that need updating?
Is there a plan to deprecate header-based routing eventually?

Verdict: LGTM with minor suggestions ✅

The implementation is solid and follows established patterns. The main concerns are:

Confirming backwards compatibility of protocol removal
Adding test coverage (can be follow-up)
Documenting the new endpoint

The code is well-structured, maintains security properties, and integrates cleanly with the existing routing infrastructure.

claude · 2025-11-04T19:07:47Z

PR Review: Add support for routing runner ws to `/runners/connect`

Summary

This PR adds path-based routing for runner WebSocket connections to complement the existing header-based routing. The changes allow runners to connect via the path /runners/connect instead of requiring the X-Rivet-Target: runner header or rivet_target.runner WebSocket protocol.

Code Quality & Best Practices ✅

Positive aspects:

Good separation of concerns with the new route_runner_internal function that extracts shared logic
Clear documentation in comments explaining the routing methods
Follows existing patterns in the codebase (similar to pegboard_gateway.rs which has both route_request and route_request_path_based)
Proper use of structured logging with tracing::debug!
Consistent error handling using Result<Option<RoutingOutput>>

Suggestions:

The TypeScript SDK change removes the rivet_target.runner protocol from the protocols array (line 467), which is good for simplifying the connection, but ensure this is backward compatible if any other services rely on this protocol being present.

Potential Issues 🔍

Minor concerns:

Path parsing: The path-based routing uses .split('?').next().unwrap_or(path) to strip query strings (line 37 in runner.rs). This is fine, but be aware that:
- Fragments (#) are not handled, though they typically don't reach the server
- The query parameters are still passed through to route_runner_internal, which is correct
Routing order: In mod.rs, the runner path-based routing happens after actor path-based routing but before header-based routing. This is appropriate, but ensure:
- No conflicts with existing paths (✅ /runners/connect is distinct from /gateway/...)
- The order is documented somewhere if routing logic becomes more complex
Missing validation: The path-based routing doesn't validate that it's a WebSocket upgrade request. While route_runner_internal will handle it correctly for both HTTP and WebSocket, you might want to add a check or comment explaining that /runners/connect should typically be WebSocket-only.

Performance Considerations ⚡

Good:

Early returns in routing functions prevent unnecessary processing
String operations are minimal (single .split() call)
No new allocations in hot path beyond what was already there

Neutral:

The routing now checks path-based runner routes for every request, but this is a simple string comparison and won't impact performance

Security Concerns 🔒

Positive:

Authentication is properly handled in route_runner_internal for both routing methods
Host validation is performed before routing
Token extraction works for both WebSocket protocol and HTTP headers

No concerns identified - the security model is unchanged and the new path-based routing reuses the same authentication logic.

Test Coverage 📋

Missing:

No unit tests found for the guard routing logic
Consider adding tests for:
- Path-based routing to /runners/connect
- Path parsing with query strings (e.g., /runners/connect?protocol_version=1)
- Negative cases (paths that shouldn't match)
- Both header-based and path-based routing with and without auth

Recommendation: Add integration tests to ensure both routing methods work correctly and that the TypeScript SDK can successfully connect using the new path.

Additional Notes

Documentation: Consider updating any API documentation or SDK guides that reference runner connection methods to include the new path-based approach.
Consistency: The MARK comments (// MARK: Path-based routing) are a nice touch for code organization.
TypeScript SDK: The URL construction in mod.ts (lines 458-462) properly handles trailing slashes, which is good defensive programming.

Verdict

Approve with minor suggestions ✅

The PR is well-implemented and follows the existing patterns in the codebase. The refactoring to extract route_runner_internal improves code maintainability. The main recommendation is to add tests to cover the new routing path.

The changes align well with the simplification goal mentioned in the base branch name and provide a cleaner API for runner connections.

vercel · 2025-11-04T19:24:57Z

Deployment failed with the following error:

Too many requests - try again in 4 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-04T19:38:22Z

Code Review for PR #3340

Summary

This PR adds path-based routing support for runner WebSocket connections via /runners/connect, complementing the existing header-based routing. The changes affect both the Rust guard routing logic and the TypeScript runner SDK.

Code Quality & Best Practices

✅ Strengths:

Good code organization: The refactoring extracts common logic into route_runner_internal(), eliminating duplication between path-based and header-based routing
Consistent patterns: The new route_request_path_based() follows the same pattern used in pegboard_gateway.rs for actor routing
Clear documentation: Function comments accurately describe the routing behavior
Proper structured logging: Uses tracing with context fields (?host, path) following the codebase conventions from CLAUDE.md

⚠️ Suggestions:

Query string parsing (runner.rs:37): Using split('?').next().unwrap_or(path) is fine, but consider edge cases with fragments
Comment style (mod.rs:54, 82): The // MARK: comments are helpful for navigation but aren't a standard Rust convention

Potential Bugs or Issues

🔴 Critical:

Path matching is too strict (runner.rs:37-38): The exact match means /runners/connect/ with trailing slash will NOT match. Consider if this is intentional or if trailing slashes should be stripped.
WebSocket protocol breaking change (mod.ts:467-469): The removal of rivet_target.runner protocol creates a breaking change. Old guards expecting this protocol will reject connections from updated SDK clients. Consider supporting both during a transition period.

Performance Considerations

✅ Good:

Routing order is optimized: path-based checks happen before header parsing (mod.rs:75-80)
String splitting for query parameters is lightweight

Security Concerns

✅ Authentication is preserved:

The route_runner_internal() function maintains authentication checks (runner.rs:88-125)
Regional host validation prevents cross-datacenter routing attacks (runner.rs:54-76)

⚠️ Consideration:
Ensure the /runners/connect path is not cached by CDNs and load balancers properly forward WebSocket upgrade headers.

Test Coverage

❌ Missing tests:
No new tests were added for runner path-based routing functionality. Recommend adding tests similar to tests/parse_actor_path.rs to cover:

Exact path matching
Query parameter handling
Trailing slash behavior
Invalid path rejection

Recommendations Summary

Must address:

Consider backward compatibility for the protocol change in TypeScript SDK
Add test coverage for runner path-based routing
Document or fix trailing slash behavior

Should consider:

Fragment handling consistency with actor path parsing
Document the migration path for existing deployments
Review infrastructure security for the new endpoint

Overall, this is a solid refactoring that improves the routing architecture. The main concerns are around backward compatibility and test coverage.

claude · 2025-11-04T19:40:52Z

PR Review: Add path-based routing for runner WebSocket connections

Summary

This PR adds path-based routing support for runner WebSocket connections to /runners/connect, complementing the existing header-based routing (X-Rivet-Target: runner). The TypeScript SDK has been updated to use the new path-based endpoint.

Code Quality & Best Practices ✅

Strengths:

Clean code refactoring with good separation of concerns - the route_runner_internal function properly extracts shared logic between header-based and path-based routing
Consistent with existing patterns (similar to the actor path-based routing in pegboard_gateway.rs)
Clear, descriptive comments using // MARK: annotations
Follows repository conventions from CLAUDE.md (proper error handling with anyhow::*, structured logging with tracing)
Good use of structured logging: tracing::debug!(?host, path, "routing to runner via path") instead of formatting into the message

Minor improvements to consider:

TypeScript SDK - Potential path duplication: The SDK now always appends /runners/connect to the endpoint. If pegboardEndpoint or endpoint already contains /runners/connect, this could result in /runners/connect/runners/connect. Consider adding validation:

const baseUrl = wsEndpoint.endsWith("/runners/connect")
    ? wsEndpoint.slice(0, -"/runners/connect".length)
    : wsEndpoint.endsWith("/")
    ? wsEndpoint.slice(0, -1)
    : wsEndpoint;

Hardcoded path string: The path /runners/connect appears in multiple places (routing logic and SDK). Consider defining it as a constant for maintainability:

// In routing/runner.rs or routing/mod.rs
pub(crate) const RUNNER_CONNECT_PATH: &str = "/runners/connect";

Potential Bugs 🔍

Low severity:

The path matching uses exact string comparison (path_without_query != "/runners/connect"), which is correct, but doesn't handle trailing slashes. If a client sends /runners/connect/, it won't match. Consider whether this is intentional or if you should normalize paths.

Security Considerations 🔒

Good security practices observed:

Authentication is properly enforced through route_runner_internal for both routing methods
Token extraction from WebSocket protocols maintains the same security model
Host validation for datacenter routing prevents potential routing attacks
No security regressions introduced

Observation:

The removal of rivet_target.runner from the WebSocket protocols list (in mod.ts:468) means the SDK now relies entirely on path-based routing. Ensure this is a breaking change that's properly communicated if any existing runners use the old protocol-based approach.

Performance Considerations ⚡

Positive:

Path-based routing is checked early in the routing chain (lines 76-80 in mod.rs)
Minimal overhead - simple string comparison before delegation
No additional allocations in the hot path

Note:

The split('?') operation creates a temporary allocation for path_without_query, but this is negligible and standard practice.

Test Coverage 🧪

Concerns:

✅ Good test coverage exists for actor path-based routing (tests/parse_actor_path.rs)
⚠️ No corresponding tests for runner path-based routing

Recommended additions:

Unit tests for route_request_path_based:
- Valid path: /runners/connect
- Valid path with query params: /runners/connect?protocol_version=1
- Invalid paths: /runners/connect/, /runner/connect, /api/runners/connect
- Path without query should match
Integration tests to verify:
- Path-based routing takes precedence over header-based
- Authentication works correctly with path-based routing
- WebSocket upgrade succeeds on the new endpoint

Example test structure:

#[test]
fn test_route_runner_path_based_valid() {
    let path = "/runners/connect?protocol_version=1";
    // Test that route_request_path_based returns Some(RoutingOutput)
}

#[test]
fn test_route_runner_path_based_invalid() {
    assert!(matches_runner_path("/api/runners/connect").is_none());
    assert!(matches_runner_path("/runners/connect/").is_none());
}

Documentation 📚

Consider adding a comment explaining why both routing methods exist (backwards compatibility? migration path?)
The SDK change is silent - consider documenting the endpoint change in a changelog or migration guide

Overall Assessment

Rating: 8/10 - Well-implemented feature with good code quality. Main areas for improvement are test coverage and potential edge case handling.

Recommendation: Approve with minor suggestions - The code is production-ready, but adding tests would significantly improve confidence in the implementation.

Action Items:

Add unit tests for runner path-based routing (similar to actor path tests)
Consider the trailing slash edge case
Evaluate if removing rivet_target.runner protocol is a breaking change requiring documentation
Optional: Extract path constant for maintainability

🤖 Generated with Claude Code

claude · 2025-11-04T20:28:33Z

Code Review for PR #3340: Add support for routing runner ws to `/runners/connect`

Summary

This PR adds path-based routing for runner WebSocket connections via /runners/connect, complementing the existing header-based routing mechanism. The changes span both the Rust guard package and the TypeScript runner SDK.

✅ Positives

Good Code Organization: The refactoring properly extracts shared logic into route_runner_internal(), following DRY principles and making the code more maintainable.
Consistent Pattern: The implementation follows the existing pattern established by actor path-based routing (e.g., pegboard_gateway::route_request_path_based), maintaining consistency across the codebase.
Clean Query String Handling: The query string parsing in route_request_path_based at line 37 correctly splits on '?' to match only the path portion.
TypeScript SDK Update: The client-side changes properly construct the new URL format and simplify the protocol list.
Logging Improvements: Added differentiation between "routing via header" and "routing via path" which will aid in debugging.

🔍 Observations & Suggestions

1. Protocol Change Impact (TypeScript SDK)

In engine/sdks/typescript/runner/src/mod.ts:467, the protocol list was changed from:

const protocols = ["rivet", `rivet_target.runner`];

to:

const protocols = ["rivet"];

Question: Is this a breaking change? If existing deployments expect the rivet_target.runner protocol, this could cause compatibility issues during rolling updates. Consider:

Adding a migration guide if this is breaking
Supporting both protocols temporarily for backwards compatibility
Documenting the reasoning for this removal

2. Test Coverage

The PR doesn't include tests for the new route_request_path_based function. Given that there's excellent test coverage for parse_actor_path in tests/parse_actor_path.rs, consider adding similar tests for runner routing:

#[test]
fn test_route_runner_path_based_valid() {
    // Test /runners/connect matches
}

#[test]
fn test_route_runner_path_based_with_query_params() {
    // Test /runners/connect?protocol_version=1&namespace=foo&runner_key=bar
}

#[test]
fn test_route_runner_path_based_invalid_paths() {
    // Test /runners, /runner/connect, /runners/connect/extra don't match
}

3. Path Validation Strictness

At engine/packages/guard/src/routing/runner.rs:37, the path matching is very strict:

if path_without_query != "/runners/connect" {
    return Ok(None);
}

Consider: Should this also reject trailing slashes? For example, should /runners/connect/ be handled differently? The current implementation would reject it, which may be correct but worth documenting or handling explicitly.

4. Error Handling & Security

The route_runner_internal function properly validates:

Host against datacenter configuration
Authentication tokens (when enabled)
WebSocket vs HTTP headers

This looks good, but ensure that the new path-based route is documented in any security/firewall configurations, as it opens a new endpoint.

5. Routing Order in mod.rs

In engine/packages/guard/src/routing/mod.rs:76-80, the runner path-based routing is checked after actor routing but before header-based routing. This ordering is logical and should prevent conflicts, but consider:

What happens if someone tries to access /gateway/runners/connect? (Should be handled as actor routing, which seems correct)
Document the routing precedence order in code comments or documentation

6. Minor Style Consistency

The code follows the CLAUDE.md conventions well:

✅ Uses structured logging: tracing::debug!(?host, path, "...")
✅ Uses anyhow::* for error handling
✅ Follows existing patterns
✅ Hard tabs (as per rustfmt.toml)

7. Documentation

Consider adding documentation about:

Why path-based routing was added (was header-based insufficient?)
Migration guide for clients using the old SDK
API documentation for the new /runners/connect endpoint

🎯 Recommended Actions

Before Merging:

✅ Verify the protocol removal in TypeScript isn't breaking (or document if intentional)
📝 Add unit tests for route_request_path_based
📚 Add inline documentation explaining the routing precedence

Post-Merge (if not blocking):

Consider integration tests validating the full flow
Update any API documentation or OpenAPI specs
Monitor logs for both "via header" and "via path" routing to understand usage patterns

Overall Assessment

This is a well-structured change that follows existing patterns and maintains code quality. The main concerns are around test coverage and potential breaking changes in the TypeScript SDK protocol list. With those addressed, this should be good to merge.

Recommendation: Request clarification on the protocol change impact, add tests, then approve.

Review generated with attention to the repository's CLAUDE.md conventions

claude · 2025-11-04T20:32:01Z

Pull Request Review - PR #3340

Summary

This PR adds support for path-based routing of runner WebSocket connections to /runners/connect, moving away from protocol-based routing using rivet_target.runner. The changes affect both the Rust guard routing logic and the TypeScript runner SDK.

Code Quality ✅

Strengths

Good code organization: The refactoring extracts common logic into route_runner_internal(), following the DRY principle
Consistent pattern: Mirrors the existing route_request_path_based pattern used for actor routing in pegboard_gateway.rs
Clear documentation: Function comments clearly explain the routing logic
Maintains backward compatibility: Header-based routing still works alongside the new path-based routing

Code Structure

The changes follow established patterns in the codebase:

Path-based routing is checked before header-based routing (lines 75-80 in mod.rs)
Both routing methods delegate to a shared internal function
Proper use of Option<RoutingOutput> return types for routing functions

Potential Issues 🔍

1. Query Parameter Handling (Minor)

In runner.rs:37, the path is split on ? to extract the base path:

let path_without_query = path.split('?').next().unwrap_or(path);

This is correct, but consider edge cases:

Multiple ? characters (though unlikely in valid URLs)
Fragment identifiers (#) - currently not handled, but probably fine for WebSocket connections

Recommendation: This is likely fine, but could add a comment explaining the query string handling.

2. WebSocket Protocol Simplification (Question)

In mod.ts:467-469, the protocols array changed from:

const protocols = ["rivet", `rivet_target.runner`];

to:

const protocols = ["rivet"];

Questions:

Is this a breaking change for existing deployments?
Should there be a deprecation period where both are supported?
Are there any clients still using rivet_target.runner?

Recommendation: Verify this doesn't break existing runner connections, or document the migration path.

3. Path Matching Strictness (Minor)

The path matching in runner.rs:38 is strict:

if path_without_query != "/runners/connect" {

This means:

✅ Matches: /runners/connect
❌ Doesn't match: /runners/connect/
❌ Doesn't match: /Runners/Connect

Recommendation: Consider if trailing slashes should be handled, though the current strict matching is probably intentional and correct.

Performance Considerations ✅

Early returns: Both routing functions return early when not applicable, minimizing overhead
String allocation: Minimal - only splits the query string which is efficient
No regex: Uses simple string comparison which is fast

Security Concerns ✅

Authentication Flow

The authentication logic in route_runner_internal() (lines 88-125) properly:

✅ Validates the host against the datacenter configuration
✅ Extracts tokens from both WebSocket protocols and headers
✅ Validates tokens against the configured admin token
✅ Returns appropriate error messages for missing or invalid authentication

Potential Concerns

Token in URL: The TypeScript SDK puts the runner_key in query parameters (line 462 in mod.ts). While this is common for WebSocket connections, ensure:
- These URLs are not logged in plaintext
- The connection uses TLS/WSS in production
Host validation: The datacenter host validation (lines 54-76) is good, but ensure the is_valid_regional_host() function is robust against various attack vectors.

Test Coverage ⚠️

Major Concern: I don't see test files included in this PR.

Recommendations:

Add integration tests for the new path-based routing:
- Test /runners/connect successfully routes to runner service
- Test that /runners/connect/ (with trailing slash) behaves as expected
- Test with various query parameters
- Test authentication with valid/invalid tokens
Add tests for backward compatibility:
- Verify header-based routing still works
- Test that the new SDK can connect successfully
Test error cases:
- Invalid paths
- Missing authentication
- Wrong datacenter host

Additional Observations

TypeScript Changes

URL construction (lines 458-462): The new URL construction properly handles trailing slashes, which is good
Base64 in query params: The namespace and runner_key are URL-encoded (line 462), which is correct

Documentation

The PR description is empty. Consider adding:

The motivation for this change
Any breaking changes
Migration guide if needed
Related issues or PRs

Recommendations Summary

Must Address

⚠️ Add test coverage for the new routing functionality
📝 Verify backward compatibility - ensure removing rivet_target.runner from protocols doesn't break existing runners

Should Consider

📝 Add a PR description explaining the motivation and any breaking changes
🔍 Verify that runner_key in query parameters doesn't create security issues in your logging/monitoring setup
📝 Consider adding inline comments for query parameter handling logic

Nice to Have

📝 Add documentation about the new /runners/connect endpoint
🧪 Add more edge case tests (trailing slashes, case sensitivity, etc.)

Conclusion

Overall, this is a well-structured refactoring that follows existing patterns in the codebase. The main concerns are around test coverage and verifying backward compatibility. The security posture appears sound, assuming proper TLS usage and careful logging practices.

Status: ✅ Approve with minor recommendations

graphite-app · 2025-11-04T20:53:07Z

Merge activity

Nov 4, 8:53 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 4, 8:53 PM UTC: CI is running for this pull request on a draft pull request (#3353) due to your merge queue CI optimization settings.
Nov 4, 8:56 PM UTC: Merged by the Graphite merge queue via draft PR: #3353.

…#3340)

NathanFlurry requested a review from MasterPtato November 4, 2025 17:45

NathanFlurry mentioned this pull request Nov 4, 2025

fix(guard): include method in cache key #3338

Closed

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 17:47 View deployment

MasterPtato approved these changes Nov 4, 2025

View reviewed changes

MasterPtato reviewed Nov 4, 2025

View reviewed changes

NathanFlurry force-pushed the 11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_ branch from 0fb4cee to 90a20c3 Compare November 4, 2025 18:38

NathanFlurry mentioned this pull request Nov 4, 2025

chore(guard): handle cors on the gateway #3341

Closed

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 18:39 View deployment

NathanFlurry force-pushed the 11-04-chore_guard_simplify_gateway_routing branch from 02256ed to 7ee6720 Compare November 4, 2025 18:59

NathanFlurry force-pushed the 11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_ branch from 90a20c3 to 50142b7 Compare November 4, 2025 18:59

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 19:00 View deployment

NathanFlurry force-pushed the 11-04-chore_guard_simplify_gateway_routing branch from 7ee6720 to 4a27884 Compare November 4, 2025 19:24

NathanFlurry force-pushed the 11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_ branch from 50142b7 to 23c26cd Compare November 4, 2025 19:24

This was referenced Nov 4, 2025

chore(rivetkit): debounce ws message ack #3345

Closed

chore(rivetkit): auto-update auto-start engine version #3344

Closed

ci: trigger rust ci for generated bare artifacts #3346

Closed

chore(guard): add support for routing runner ws to /runners/connect

e86c567

NathanFlurry force-pushed the 11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_ branch from 23c26cd to e86c567 Compare November 4, 2025 20:25

NathanFlurry force-pushed the 11-04-chore_guard_simplify_gateway_routing branch from 4a27884 to dc31f94 Compare November 4, 2025 20:25

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 20:26 View deployment

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025

chore(guard): add support for routing runner ws to /runners/connect (…

f9dd2a8

…#3340)

graphite-app bot closed this Nov 4, 2025

graphite-app bot deleted the 11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_ branch November 4, 2025 20:56

chore(guard): add support for routing runner ws to /runners/connect #3340

chore(guard): add support for routing runner ws to /runners/connect #3340

Uh oh!

Conversation

NathanFlurry commented Nov 4, 2025

Uh oh!

vercel bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 4, 2025

Code Review: Add support for routing runner ws to /runners/connect

Summary

Code Quality & Best Practices

✅ Strengths

🔍 Observations

Potential Issues

⚠️ Minor Concerns

Performance Considerations

Security Concerns

✅ Security looks solid:

🔒 Recommendations:

Test Coverage

⚠️ Missing Tests:

Overall Assessment

Action Items:

Recommendation: ✅ Approve with minor fixes

Uh oh!

MasterPtato left a comment

Choose a reason for hiding this comment

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Add support for routing runner WebSocket to /runners/connect

Summary

Code Quality ✅

Potential Issues & Suggestions

1. Path validation could be more defensive (Minor)

2. WebSocket protocol removal impact (Requires verification)

3. URL construction edge case (Minor)

Security Considerations ✅

Performance Considerations ✅

Test Coverage ⚠️

Documentation 📝

Questions for Author

Verdict: LGTM with minor suggestions ✅

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Add support for routing runner ws to /runners/connect

Summary

Code Quality & Best Practices ✅

Potential Issues 🔍

Performance Considerations ⚡

Security Concerns 🔒

Test Coverage 📋

Additional Notes

Verdict

Uh oh!

vercel bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Code Review for PR #3340

Summary

Code Quality & Best Practices

Potential Bugs or Issues

Performance Considerations

Security Concerns

Test Coverage

Recommendations Summary

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Add path-based routing for runner WebSocket connections

Summary

Code Quality & Best Practices ✅

Potential Bugs 🔍

Security Considerations 🔒

Performance Considerations ⚡

Test Coverage 🧪

chore(guard): add support for routing runner ws to `/runners/connect` #3340

chore(guard): add support for routing runner ws to `/runners/connect` #3340

vercel bot commented Nov 4, 2025 •

edited

Loading

NathanFlurry commented Nov 4, 2025 •

edited

Loading

Code Review: Add support for routing runner ws to `/runners/connect`

PR Review: Add support for routing runner WebSocket to `/runners/connect`

PR Review: Add support for routing runner ws to `/runners/connect`

Code Review for PR #3340: Add support for routing runner ws to `/runners/connect`

graphite-app bot commented Nov 4, 2025 •

edited

Loading