chore(guard): simplify gateway routing #3339

NathanFlurry · 2025-11-04T17:37:29Z

No description provided.

vercel · 2025-11-04T17:37:32Z

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:38:16Z

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:39:33Z

PR Review: Simplify Gateway Routing

Overview

This PR successfully simplifies the gateway routing pattern from a verbose multi-segment approach to a cleaner, more intuitive @ syntax. The changes are consistent across both Rust (guard) and TypeScript (RivetKit) implementations.

Old format: /gateway/actors/{actor_id}/tokens/{token}/route/{path}
New format: /gateway/{actor_id}@{token}/{path}

Code Quality ✅

Rust Implementation (guard/src/routing/mod.rs)

Strengths:

Clean refactoring with significantly reduced complexity
Proper error handling for edge cases (empty actor_id, empty token, double slashes)
Maintains backward compatibility with token-less routing
Consistent string manipulation using find('@') for parsing
Good prefix length calculation logic

Potential Issues:

Multiple @ signs ambiguity (line 194): The code uses find('@') which finds the FIRST occurrence. This means actor-123@token@extra would parse as:
- actor_id: actor-123
- token: token@extra
While this is handled correctly in tests (test_parse_actor_path_multiple_at_signs), it might be unexpected behavior. Consider documenting this or potentially validating against multiple @ signs if tokens shouldn't contain them.
No validation of actor_id or token format: The code accepts any non-empty string after parsing. Consider whether you need validation for:
- Valid DNS subdomain format for actor_ids (per CLAUDE.md conventions)
- Token format restrictions (special characters, length limits)

TypeScript Implementation (rivetkit/src/manager/gateway.ts)

Strengths:

Mirror implementation of Rust logic - excellent consistency
Proper handling of query strings and fragments
Clean refactoring removing header-based routing complexity

Potential Issues:

Same @ sign ambiguity (line 399): Uses indexOf('@') consistently with Rust, but same concern applies
Type safety (line 86, 286): Uses as any for encoding parameter:
```
encoding as any, // Will be validated by driver
```
While the comment explains it, consider using proper type guards or assertions instead of as any

Test Coverage ✅

Rust Tests (parse_actor_path.rs)

Excellent coverage:

✅ Basic paths with/without tokens
✅ UUIDs as actor IDs
✅ Query parameters and fragments
✅ Trailing slashes
✅ URL encoding
✅ Special characters
✅ Multiple @ signs edge case
✅ Empty value validation
✅ Double slash rejection
✅ Case sensitivity

Test quality: Very thorough, with good edge case coverage

TypeScript Tests (parse-actor-path.test.ts)

Good coverage matching Rust tests:

✅ All major scenarios covered
✅ Consistent with Rust test patterns
✅ Good organization with describe blocks

Minor gap: No test for multiple @ signs like Rust has

Potential Bugs 🔍

1. No Encoding Validation for @ in URLs

The @ symbol is a reserved character in URLs (RFC 3986). While it's valid in the path component, clients might inadvertently encode it as %40. Consider:

Testing with encoded @ signs: /gateway/actor-123%40token/path
Current code would NOT recognize this as a token pattern
Recommendation: Add test cases and potentially handle URL-encoded @ signs

2. Token Security Consideration

Tokens are now exposed in URL paths, which means:

They'll appear in server logs
They'll be in browser history
They'll be in any URL-based monitoring/analytics
Recommendation: Document this security consideration and ensure tokens are short-lived or have appropriate scope

3. Prefix Length Calculation Edge Case

In both implementations (Rust line 210, TS line 417-420):

let prefix_len = 1 + segments[0].len() + 1 + segments[1].len();

This assumes the path starts with /. If somehow a malformed path without leading / reaches this code, the calculation would be incorrect. However, the segment parsing already handles this implicitly, so this is likely safe.

Performance Considerations 👍

Improvements:

✅ Reduced string operations (no longer need to parse 6+ segments)
✅ Simpler path prefix calculation
✅ Single find('@') operation vs multiple segment comparisons

No concerns - the simplification improves performance

Security Considerations ⚠️

URL Injection: The code properly validates for:
- ✅ Empty values
- ✅ Double slashes
- ✅ Case-sensitive keywords
Token Exposure: As mentioned above, tokens in URLs is inherently less secure than headers. The comment "NOTE: Token validation implemented in EE" suggests proper validation exists elsewhere.
No Path Traversal Risk: The remaining path is extracted using string slicing based on prefix length, not joined with user input, so no risk here.

Client Updates ✅

actor-http-client.ts:

✅ Correctly builds new URL format: /gateway/${actorId}${url.pathname}
✅ Removes now-unused HEADER_RIVET_TARGET header
✅ Properly handles query strings

actor-websocket-client.ts:

✅ Correctly builds new URL format
✅ Removes WS_PROTOCOL_TARGET from protocol list
✅ Maintains other WebSocket protocols

Example update:

✅ cursors-raw-websocket example updated correctly

Recommendations

High Priority:

Document token security implications - Add comments about token exposure in URLs
Consider URL-encoded @ handling - Add test cases at minimum, potentially support decoding

Medium Priority:

Add TypeScript test for multiple @ signs - Match Rust test coverage
Validate actor_id format - Consider DNS subdomain validation per naming conventions
Replace as any with proper types - Improve type safety in TS code

Low Priority:

Document @ sign behavior - Explicitly document that first @ is the token delimiter
Consider token format validation - If tokens have specific format requirements

Summary

✅ Approve with minor suggestions

This is a well-executed refactoring that significantly simplifies the routing logic while maintaining correctness. The dual implementation in Rust and TypeScript is consistent, and test coverage is excellent. The main concerns are around documentation and security considerations for tokens in URLs, which should be addressed but don't block the PR.

The code quality follows the project's conventions (proper error handling, structured logging, workspace dependencies), and the simplification will improve maintainability going forward.

Diff Stats: +155 -224 lines (net -69 lines of code removed) 🎉

claude · 2025-11-04T19:01:15Z

Pull Request Review

Summary

This PR simplifies the gateway routing by reducing URL verbosity. The changes replace the verbose routing pattern /gateway/actors/{actor_id}/tokens/{token}/route/{path} with the more concise /gateway/{actor_id}@{token}/{path} pattern.

Positive Aspects

1. Code Quality & Simplification

Excellent simplification that reduces URL complexity
The @ syntax for tokens is intuitive and commonly used in URLs (similar to user@host patterns)
Both Rust and TypeScript implementations are consistent and mirror each other well
Comprehensive test coverage with edge cases properly handled

2. Test Coverage

Tests cover all important scenarios: tokens, query params, fragments, trailing slashes, special characters
Negative test cases properly validate error conditions
Tests added for multiple @ signs and token special characters

3. Documentation

Clear inline comments documenting the new URL patterns
Function-level documentation updated to reflect new patterns

Code Quality Observations

1. Rust Implementation (engine/packages/guard/src/routing/mod.rs)

The implementation is clean and efficient. The code at line 194 uses find('@') which only finds the first occurrence - this is correct behavior (tokens can contain @ characters), and the test validates this. Good defensive programming.

2. TypeScript Implementation (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts)

The TypeScript implementation mirrors the Rust logic perfectly, which is excellent for consistency.

3. Path Calculation Logic

Both implementations use the same approach for calculating remaining path. The Rust version uses direct calculation while TypeScript uses a loop - both are correct, though the Rust approach is slightly more efficient.

Potential Issues & Considerations

1. Backward Compatibility

The old routing pattern appears to be completely removed
No migration path or deprecation period mentioned
Question: Are there existing clients using the old URL pattern? If so, this is a breaking change that should be documented.

2. Token Security Consideration

Tokens are now part of the URL path rather than a separate segment
While the code has a comment "NOTE: Token validation implemented in EE", ensure that:
- Tokens in URLs don't get logged in access logs
- URL encoding is properly handled for special characters in tokens
- Length limits are enforced if needed

3. URL Encoding

The code doesn't explicitly handle URL encoding of the @ character or special characters in actor_id/token
If an actor_id legitimately contains @, it should be URL-encoded as %40
Recommendation: Add validation or documentation about what characters are allowed in actor_id and token

4. Removed Headers

HEADER_RIVET_TARGET is no longer set in actor-http-client.ts
WS_PROTOCOL_TARGET is no longer used in actor-websocket-client.ts
Question: Are these still needed for the header-based routing fallback?

Minor Suggestions

1. Consider Edge Cases

What happens if actor_id or token contains URL-unsafe characters?
Should there be length limits enforced on these segments?
Consider adding a test for extremely long actor_id or token values

2. Documentation

Update any API documentation or user guides that reference the old URL pattern
Document the character restrictions for actor_id and token (if any)

3. Error Messages
When path parsing fails, the function returns None/null silently. Consider whether more specific error messages would help debugging.

Security Considerations

Good:

Empty actor_id and token are properly rejected
Double slashes are rejected (prevents path traversal issues)
Fragment identifiers are properly stripped
Query parameters are preserved correctly

Consider:

Tokens in URLs may appear in:
- Browser history
- Server access logs
- Referrer headers
- Browser developer tools
Ensure these risks are acceptable for your use case or add warnings to documentation

Performance

No performance concerns. The new implementation is actually more efficient:

Fewer string operations needed
Simpler parsing logic
Shorter URLs mean less data transfer

Recommendations

High Priority: Clarify the backward compatibility story. If this breaks existing clients, document the migration path.
Medium Priority: Add explicit documentation about:
- Character restrictions for actor_id and token
- URL encoding requirements
- Security implications of tokens in URLs
Low Priority: Consider adding these test cases:
- URL-encoded @ in actor_id (%40)
- Very long actor_id/token values
- International characters in actor_id

Overall Assessment

This is a well-implemented refactoring that simplifies the routing logic significantly. The code is clean, well-tested, and consistent across both Rust and TypeScript implementations. The main concern is around backward compatibility and ensuring existing clients won't break.

Recommendation: Approve pending clarification on backward compatibility impact.

Review generated with Claude Code

claude · 2025-11-04T19:36:58Z

PR Review: Simplify Gateway Routing

Summary

This PR simplifies the gateway routing by changing from a verbose path structure (/gateway/actors/{actor_id}/tokens/{token}/route/{...path}) to a cleaner format (/gateway/{actor_id}@{token}/{...path}). The changes are consistent across both Rust (guard) and TypeScript (rivetkit) implementations.

Positive Aspects

Cleaner API: The new URL structure is significantly more concise and user-friendly:
- Before: /gateway/actors/actor-123/tokens/my-token/route/api/endpoint
- After: /gateway/actor-123@my-token/api/endpoint
Consistent Implementation: Both Rust and TypeScript implementations are updated in parallel with identical logic, reducing the chance of behavioral differences.
Comprehensive Test Coverage: Excellent test updates in both languages covering:
- Valid paths with and without tokens
- Edge cases (empty values, double slashes, case sensitivity)
- Query parameters and fragments
- Special characters and URL encoding
- Multiple @ signs in tokens
- Trailing slashes
Backward Compatibility Removed Cleanly: The old routing pattern is completely removed, making the codebase simpler without maintaining multiple path formats.

Code Quality Issues

1. Potential Security Issue: Token Exposure in URLs

Using @ syntax for tokens in the URL path (/gateway/{actor_id}@{token}/...) means tokens appear in:

Browser address bars
Server logs
Proxy logs
Referrer headers

Recommendation: Consider if this is intentional. If tokens are meant to be secrets, they should be in headers (existing HEADER_RIVET_TOKEN) or query parameters with appropriate logging safeguards. If they're not secrets (e.g., public room identifiers), this is fine but should be documented.

2. Code Inconsistency: Redundant Check in TypeScript

In rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts:391-393:

// Check for empty actor segment
if (actorSegment.length === 0) {
	return null;
}

This check is redundant because the segment comes from splitting on / and filtering out empty strings (line 375). An empty segment would never reach this point.

Note: The Rust version has the same check (line 187-189), so this is consistent but still redundant in both implementations.

3. Performance: Multiple @ Sign Handling

The test test_parse_actor_path_multiple_at_signs shows that actor-123@token@with@ats treats everything after the first @ as the token. This is handled correctly with find('@') / indexOf('@'), but consider if this could cause confusion for users.

Recommendation: Document this behavior or consider validating that tokens don't contain @ characters to avoid ambiguity.

4. Missing URL Encoding Considerations

While there's a test for URL-encoded characters in the remaining path, there's no handling or documentation for:

What if the actor_id contains @ that's URL-encoded as %40?
What if the token contains / that's URL-encoded as %2F?

The current implementation would parse /gateway/actor%40123@token/path as:

actor_id: actor%40123
token: token

This might be correct, but it's worth explicitly documenting.

5. Rust-Specific: String Allocation

In engine/packages/guard/src/routing/mod.rs:203-206:

(aid.to_string(), Some(tok.to_string()))
} else {
	(actor_id_segment.to_string(), None)
};

These to_string() calls allocate new strings. Consider if there's a way to use lifetimes to return borrowed data instead, though this might not be worth the complexity given the typical short lifetime of these values.

Potential Bugs

6. Edge Case: Leading Slash in Remaining Path

The logic at lines 220-224 (Rust) and 432-436 (TypeScript) tries to ensure the remaining path starts with /. However, the logic seems overly defensive:

let remaining_path = if remaining_base.is_empty() || !remaining_base.starts_with('/') {
	format!("/{}{}", remaining_base, query_string)
} else {
	format!("{}{}", remaining_base, query_string)
};

Since remaining_base is extracted from base_path[prefix_len..], it will either:

Be empty (when no remaining path exists) → handled by returning /
Start with / (when there are more segments)
Never start with a non-/ character (because prefix_len points right after a segment)

The condition !remaining_base.starts_with('/') should never be true unless there's a bug in the prefix_len calculation. Consider adding an assertion or simplifying this logic.

Performance Considerations

The changes should have positive performance impact:

Fewer string splits (2 segments instead of 4-6)
Simpler parsing logic
No performance concerns identified

Security Concerns

Token in URL path (mentioned above) - assess if this is appropriate for your threat model
No input validation - The code doesn't validate:
- Max length of actor_id or token
- Character restrictions beyond emptiness
- Potential DoS via extremely long paths

Recommendation: Consider adding reasonable length limits (e.g., 1024 chars for actor_id/token).

Test Coverage

Excellent - The tests are comprehensive and cover:

✅ Happy paths with and without tokens
✅ Query parameters and fragments
✅ Trailing slashes
✅ Special characters
✅ Empty values
✅ Case sensitivity
✅ Multiple @ signs
✅ URL encoding

Missing tests to consider adding:

Very long actor_id/token values (DoS prevention)
Non-ASCII characters in actor_id
Tab, newline, or other whitespace in segments
Path traversal attempts (e.g., ../ in actor_id)

Style & Convention Adherence

✅ Follows CLAUDE.md conventions:

Proper use of lowercase log messages (though no new logging added)
Consistent error handling patterns
Test structure matches existing patterns

Recommendations Summary

Must Fix:

None - code is functional

Should Consider:

Document security implications of tokens in URL paths
Remove redundant empty checks or add a comment explaining why they're kept for safety
Add length validation to prevent potential DoS
Document URL encoding behavior for @ in actor_ids

Nice to Have:
5. Add tests for extremely long inputs
6. Simplify the remaining_path logic if the edge case is truly impossible

Overall Assessment

This is a well-executed refactor that significantly improves the API ergonomics. The code is clean, well-tested, and consistent across implementations. The main concern is the security implications of tokens in URL paths, which may be acceptable depending on the use case but should be explicitly considered.

Recommendation: Approve with minor considerations for the security/documentation items mentioned above.

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

Pull Request Review

Summary

This PR simplifies the gateway routing by changing the URL pattern from the verbose /gateway/actors/{actor_id}/tokens/{token}/route/{...path} to the more concise /gateway/{actor_id}@{token}/{...path}. This is a great simplification that improves the API ergonomics significantly.

Code Quality - APPROVED

Positive aspects:

Consistent implementation: Both Rust and TypeScript implementations follow the same logic and handle edge cases identically
Well-tested: Comprehensive test coverage with 30+ test cases covering valid paths, invalid paths, edge cases, and special characters
Clean refactoring: The code is more readable and maintainable than the previous implementation
Proper error handling: All edge cases (empty actor_id, empty token, malformed paths) are properly validated

Security Considerations - APPROVED

Token handling:

The new @ syntax for tokens is secure and well-handled
Empty token validation prevents bypass attempts (actor-123@/ is correctly rejected)
Multiple @ signs are handled gracefully (first @ splits, rest become part of token)
Token extraction preserves the full token string including special characters

Path traversal:

Double slash detection prevents path normalization attacks
Fragment stripping prevents URL fragment-based bypasses
Query parameters are properly preserved without introducing injection risks

Recommendations:

Consider documenting the token format restrictions in the code comments
The behavior with multiple @ signs may be unexpected - consider adding a comment explaining this is intentional

Test Coverage - EXCELLENT

Comprehensive test coverage includes:

Basic path parsing with and without tokens
UUID actor IDs
Query parameters and fragments
Trailing slashes
Special characters in actor IDs and tokens
URL-encoded characters
Empty values validation
Double slashes
Case sensitivity
Multiple @ signs

Performance - IMPROVED

Improvements:

Simpler parsing logic reduces computational overhead
Single pass through the path string for segment splitting
Efficient string slicing instead of multiple iterator operations
The prefix length calculation is now O(1) instead of iterating through segments

Potential Issues

1. Breaking change handling:
This is a breaking API change. Ensure:

Documentation is updated with migration guide
Old clients using the previous URL format will break
Consider if a deprecation period or dual-support is needed

2. Token visibility in URLs:
Tokens in URLs can appear in browser history, server logs, proxy logs, and referrer headers. Consider:

Documenting that tokens in URLs are for convenience/testing
Recommending header-based auth for production (x-rivet-token)

3. Character restrictions:
The code does not explicitly validate what characters are allowed in actor_id or token. Consider if URL-encoded characters should be handled differently.

Code Style - FOLLOWS CONVENTIONS

Follows repository conventions:

Hard tabs in Rust
Proper use of anyhow for error handling
Structured logging with tracing
Consistent naming conventions

Client Update Verification - COMPLETE

All client code updated correctly:

examples/cursors-raw-websocket - Updated WebSocket URL construction
rivetkit-typescript HTTP and WebSocket clients - Updated routing
Removed unnecessary HEADER_RIVET_TARGET and WS_PROTOCOL_TARGET (cleaner!)

Overall Assessment

Verdict: APPROVE with minor suggestions

This is a well-implemented refactoring that:

Simplifies the API significantly
Maintains security posture
Has excellent test coverage
Improves performance slightly
Follows repository conventions
Introduces a breaking change (ensure communication is provided)

The code is production-ready. Main concern is ensuring this breaking change is properly communicated to users.

Recommended actions before merge:

Add comments explaining multiple @ sign handling behavior
Update public API documentation with the new URL format
Add migration guide for users of the old format
Document token-in-URL security considerations

claude · 2025-11-04T20:37:26Z

PR Review: Gateway Routing Simplification

Summary

This PR simplifies the gateway routing by changing from a verbose path structure (/gateway/actors/{actor_id}/tokens/{token}/route/{path}) to a more concise format (/gateway/{actor_id}@{token}/{path}). The changes are well-implemented and include comprehensive test coverage.

✅ Strengths

Excellent Test Coverage: Both Rust and TypeScript test suites are comprehensive with 20+ test cases covering:
- Valid paths with/without tokens
- Query parameters and fragments
- Special characters and URL encoding
- Edge cases (empty values, double slashes, case sensitivity)
- New test cases for multiple @ signs and tokens with special characters
Consistent Implementation: The logic is consistently implemented across:
- Rust (engine/packages/guard/src/routing/mod.rs:147-231)
- TypeScript (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts:340-437)
- Both implementations handle the same edge cases identically
Good Documentation: Clear comments explaining the routing patterns and implementation details
Backward Compatibility: The change maintains fallback to header-based routing, ensuring smooth migration

🔍 Code Quality Observations

1. Potential Security Consideration - Multiple `@` Signs

The implementation allows multiple @ signs in the token portion:

Rust (routing/mod.rs:194):

let (actor_id, token) = if let Some(at_pos) = actor_id_segment.find('@') {
    let aid = &actor_id_segment[..at_pos];
    let tok = &actor_id_segment[at_pos + 1..];  // Takes everything after first @

TypeScript (gateway.ts:399-401):

const atPos = actorSegment.indexOf("@");
if (atPos !== -1) {
    actorId = actorSegment.slice(0, atPos);
    token = actorSegment.slice(atPos + 1);  // Takes everything after first @

Test case (parse_actor_path.rs:213-220):

#[test]
fn test_parse_actor_path_multiple_at_signs() {
    let path = "/gateway/actor-123@token@with@ats/endpoint";
    let result = parse_actor_path(path).unwrap();
    assert_eq!(result.token, Some("token@with@ats".to_string()));
}

Question: Is allowing @ signs within tokens intentional? This could potentially lead to:

Confusion if users accidentally include @ in actor IDs
URL encoding issues if not properly handled downstream
Ambiguity in parsing if not well-documented

Recommendation: Consider either:

Explicitly documenting that @ is allowed in tokens but not actor IDs
Validating that tokens don't contain @ for clarity
Adding URL encoding/decoding if needed

2. No URL Decoding in Parser

The path parser doesn't URL-decode the actor_id or token segments. While there's a test for encoded characters in the remaining path (test_parse_actor_path_encoded_characters), the actor_id and token are used as-is.

Example:

// If someone passes: /gateway/actor%2D123@tok%40en/endpoint
// actor_id will be "actor%2D123" (not "actor-123")
// token will be "tok%40en" (not "tok@en")

Question: Should the parser URL-decode the actor_id and token segments?

3. TypeScript Header Removal

In actor-http-client.ts:78-81, the code removes the HEADER_RIVET_TARGET header but keeps HEADER_RIVET_ACTOR:

// Add guard-specific headers
headers.set(HEADER_RIVET_ACTOR, actorId);
if (runConfig.token) {
    headers.set(HEADER_RIVET_TOKEN, runConfig.token);
}

Similarly, in actor-websocket-client.ts:71-72, WS_PROTOCOL_TARGET is removed but WS_PROTOCOL_ACTOR is kept:

protocols.push(WS_PROTOCOL_RIVETKIT);
protocols.push(`${WS_PROTOCOL_ACTOR}${actorId}`);

Question: Are these headers still needed for backward compatibility or fallback routing? If path-based routing is now primary, are these redundant?

🐛 Potential Issues

1. Missing Validation for Actor ID Format

The parser doesn't validate the actor_id format beyond checking it's not empty. Consider whether actor IDs should:

Match a specific pattern (e.g., UUID, alphanumeric-only, DNS-safe)
Have length limits
Exclude certain special characters

Current test (parse_actor_path.rs:99-105):

#[test]
fn test_parse_actor_path_special_characters() {
    let path = "/gateway/actor_id-123.test/endpoint";
    let result = parse_actor_path(path).unwrap();
    assert_eq!(result.actor_id, "actor_id-123.test");
}

This accepts . and _ and -, which may or may not be desired.

2. Edge Case: Empty Remaining Path Handling

When there's no path after the actor segment, the code adds /:

Rust (routing/mod.rs:213-224):

let remaining_base = if prefix_len < base_path.len() {
    &base_path[prefix_len..]
} else {
    "/"
};

TypeScript (gateway.ts:423-433):

const remainingBase = prefixLen < basePath.length 
    ? basePath.slice(prefixLen) 
    : "/";

This is consistent and good, but worth documenting that /gateway/actor-123 becomes remaining_path: "/".

⚡ Performance Considerations

String Allocations: Both implementations create new String allocations for actor_id, token, and remaining_path. This is fine for typical usage but could be optimized with Cow<str> in Rust if this becomes a hot path.
No Caching: Each request re-parses the path. If the same paths are frequently accessed, consider caching parsed results (though this may be premature optimization).

🔒 Security Concerns

No Input Sanitization: The parser accepts any non-empty string as actor_id/token. Downstream code should validate these values before using them in database queries, file paths, or other security-sensitive contexts.
Path Traversal: The remaining_path preserves user input without validation. Ensure downstream handlers properly sanitize paths to prevent traversal attacks (e.g., /gateway/actor-123/../../../etc/passwd).
Fragment Handling: Fragments are correctly stripped (routing/mod.rs:148-156), which is good for security as they should never reach the server.

📝 Suggestions

Add Validation Tests: Consider adding tests for:
- Very long actor IDs/tokens (to test buffer limits)
- Unicode characters in actor IDs
- Control characters
Documentation: Update API documentation to clearly specify:
- Valid actor_id and token formats
- Whether URL encoding is required/supported
- Migration guide from old routing format
Error Messages: The parser returns None for all invalid cases. Consider returning Result<ActorPathInfo, ParseError> with specific error messages for easier debugging.

Constants: Consider extracting magic strings:

const GATEWAY_PREFIX: &str = "gateway";
const TOKEN_SEPARATOR: char = '@';

✅ Final Verdict

Overall: Excellent work! The implementation is:

✅ Clean and well-tested
✅ Consistent across Rust and TypeScript
✅ Properly handles edge cases
✅ Net reduction in code complexity (-69 lines)

The concerns raised are mostly about validation, documentation, and edge cases that should be addressed before merging to production. Consider the security implications of allowing arbitrary characters in actor IDs and tokens.

Recommendation: Approve with minor suggestions to address validation and documentation.

🤖 Generated with Claude Code

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

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.

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

This was referenced Nov 4, 2025

chore(engine): move smoke test to engine/tests/ #3337

Closed

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

Closed

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

NathanFlurry mentioned this pull request Nov 4, 2025

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

Closed

MasterPtato approved these changes Nov 4, 2025

View reviewed changes

NathanFlurry mentioned this pull request Nov 4, 2025

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

Closed

NathanFlurry force-pushed the 11-04-fix_guard_include_method_in_cache_key branch from 0454643 to d9f9f0c Compare November 4, 2025 18:59

NathanFlurry force-pushed the 11-04-chore_guard_simplify_gateway_routing branch from 02256ed to 7ee6720 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-fix_guard_include_method_in_cache_key branch from d9f9f0c to 705c6ca Compare November 4, 2025 19:24

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

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

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): simplify gateway routing

dc31f94

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): simplify gateway routing (#3339)

c6161fd

graphite-app bot closed this Nov 4, 2025

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(guard): simplify gateway routing #3339

chore(guard): simplify gateway routing #3339

Uh oh!

NathanFlurry commented Nov 4, 2025

Uh oh!

vercel bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 4, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

graphite-app bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore(guard): simplify gateway routing #3339

chore(guard): simplify gateway routing #3339

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

PR Review: Simplify Gateway Routing

Overview

Code Quality ✅

Rust Implementation (guard/src/routing/mod.rs)

TypeScript Implementation (rivetkit/src/manager/gateway.ts)

Test Coverage ✅

Rust Tests (parse_actor_path.rs)

TypeScript Tests (parse-actor-path.test.ts)

Potential Bugs 🔍

1. No Encoding Validation for @ in URLs

2. Token Security Consideration

3. Prefix Length Calculation Edge Case

Performance Considerations 👍

Security Considerations ⚠️

Client Updates ✅

Recommendations

High Priority:

Medium Priority:

Low Priority:

Summary

Uh oh!

claude bot commented Nov 4, 2025

Pull Request Review

Summary

Positive Aspects

Code Quality Observations

Potential Issues & Considerations

Minor Suggestions

Security Considerations

Performance

Recommendations

Overall Assessment

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Simplify Gateway Routing

Summary

Positive Aspects

Code Quality Issues

1. Potential Security Issue: Token Exposure in URLs

2. Code Inconsistency: Redundant Check in TypeScript

3. Performance: Multiple @ Sign Handling

4. Missing URL Encoding Considerations

5. Rust-Specific: String Allocation

Potential Bugs

6. Edge Case: Leading Slash in Remaining Path

Performance Considerations

Security Concerns

Test Coverage

Style & Convention Adherence

Recommendations Summary

Overall Assessment

Uh oh!

claude bot commented Nov 4, 2025

Pull Request Review

Summary

Code Quality - APPROVED

Security Considerations - APPROVED

Test Coverage - EXCELLENT

Performance - IMPROVED

Potential Issues

Code Style - FOLLOWS CONVENTIONS

Client Update Verification - COMPLETE

Overall Assessment

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Gateway Routing Simplification

vercel bot commented Nov 4, 2025 •

edited

Loading

NathanFlurry commented Nov 4, 2025 •

edited

Loading

1. Potential Security Consideration - Multiple `@` Signs

graphite-app bot commented Nov 4, 2025 •

edited

Loading