feat(core): allow routing traffic to actors via path #3250
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
How to use the Graphite Merge QueueAdd 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. |
PR Review: Path-based Actor RoutingGreat work on implementing path-based routing for actors! The implementation is solid with excellent test coverage. Here are my findings: ✅ Strengths
🐛 Potential Issues1. Typo in Header Constant (packages/guard/src/routing/pegboard_gateway.rs:13)pub const X_RIVET_AMESPACE: HeaderName = HeaderName::from_static("x-rivet-namespace");Should be 2. Token Validation Not ImplementedBoth implementations have
3. Query Parameter Routing Removed Without Migration PathThe PR removes query parameter fallbacks (
🔒 Security Considerations
🎯 Performance Considerations
📝 Code Quality
🧪 Test CoverageExcellent coverage for
Missing integration tests:
🎨 Minor Code Style
📋 Recommendations
Overall AssessmentThis is a well-implemented feature with strong test coverage. The main concerns are the typo and clarifying the token validation behavior. The code is production-ready after addressing the typo. Approval Status: ✅ Approved with minor fixes recommended |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
ed34c5a to
89114d1
Compare
PR Review: Path-Based Actor RoutingI've reviewed the changes in this PR that add path-based routing for actors. Overall, this is a well-implemented feature with excellent test coverage. Here are my findings: ✅ Strengths
🔍 Issues Found1. Typo in Header Constant (pegboard_gateway.rs:13)pub const X_RIVET_AMESPACE: HeaderName = HeaderName::from_static("x-rivet-namespace");Should be 2. Unused Parameters (pegboard_gateway.rs:19-27)The
While the comment indicates token validation is implemented in EE, consider:
3. Fragment Handling InconsistencyIn let query_string = match (query_pos, fragment_pos) {
(Some(q), Some(f)) if q < f => &path[q..f], // Excludes fragment
...While this is generally correct for routing, it's worth documenting this behavior explicitly since:
💡 Suggestions1. Security ConsiderationsSince this PR adds a new routing mechanism, consider:
2. Logging Enhancement (mod.rs:54)tracing::debug!(?actor_path_info, "routing using path-based actor routing");Consider adding more context like the original path for debugging: tracing::debug!(?actor_path_info, original_path = %path, "routing using path-based actor routing");3. Code DuplicationThe
4. Edge Case: Very Long PathsConsider adding tests and/or validation for:
5. Performance Consideration (mod.rs:145-235)The
📝 Minor Style Notes
🔒 Security Assessment✅ No security vulnerabilities identified in the defensive security context:
📊 SummaryThis is a solid implementation with good test coverage. The main issues are:
Recommendation: ✅ Approve with minor fixes The typo should be fixed before merge. The other items are suggestions for improvement but don't block merging. 🤖 Generated with Claude Code |
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
89114d1 to
e5f9730
Compare
Pull Request Review: Path-Based Actor RoutingThis PR introduces path-based routing for actors as an alternative to header-based routing. The implementation is well-structured and thoroughly tested. Here's my comprehensive review: ✅ Strengths1. Excellent Test Coverage
2. Consistent Implementation
3. Clean Code Structure
4. Backward Compatibility
🔍 Issues Found1. Critical: Typo in Header Constant (engine/packages/guard/src/routing/pegboard_gateway.rs:13)pub const X_RIVET_AMESPACE: HeaderName = HeaderName::from_static("x-rivet-namespace");Issue: Impact: This constant is defined but never used in the code, suggesting incomplete implementation or future work. The typo would cause issues if this header is used elsewhere. Recommendation:
2. Security: Token Validation Not ImplementedMultiple locations have comments stating: Locations:
Issue: Token validation is deferred to Enterprise Edition (EE), but there's no validation or early return for invalid tokens in the open-source version. Recommendation:
|
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
e5f9730 to
a5f5632
Compare
NathanFlurry
force-pushed
the
10-23-fix_fix_biome_ignore_path
branch
from
7b4e5e7 to
323a773
Compare
Pull Request Review: Path-based Actor RoutingThis PR introduces path-based routing for actors, allowing traffic to be routed via URL paths in addition to headers. Overall, the implementation is well-structured with excellent test coverage. Here are my findings: ✅ Strengths
🔍 Code Quality Issues1. Typo in Header Name (packages/guard/src/routing/pegboard_gateway.rs:13)pub const X_RIVET_AMESPACE: HeaderName = HeaderName::from_static("x-rivet-namespace");Should be 2. Unused Token Parameter (packages/guard/src/routing/pegboard_gateway.rs:22)pub async fn route_request_path_based(
// ...
_token: Option<&str>, // Prefixed with _ but mentioned in docs
// ...
)The
3. Code Duplication in Path ParsingThe path parsing logic is implemented twice (Rust and TypeScript) with slightly different approaches. While tests ensure they match, consider:
🐛 Potential Bugs1. Fragment Handling Inconsistency (packages/guard/src/routing/mod.rs:147-149)let fragment_pos = path.find('#');
// ...
(Some(q), Some(f)) if q < f => &path[q..f],The code strips fragments from the base path but doesn't validate fragment positions. If a fragment appears before a query string (malformed URL), this could produce unexpected results. Consider adding validation. 2. Missing Path Validation in TypeScriptThe TypeScript implementation at
🔒 Security Considerations1. Token Validation PlaceholderBoth implementations have placeholders for token validation: // NOTE: Token validation implemented in EERecommendation: Ensure this is tracked and implemented before production use. Path-based tokens in URLs have different security implications than header-based auth:
2. Path Traversal ProtectionThe current implementation doesn't explicitly sanitize the
3. Actor ID ValidationThe code accepts any non-empty string as an actor ID in the path, then validates it later: let actor_id = Id::parse(actor_id_str).context("invalid actor id in path")?;This is correct, but ensure the error messages don't leak sensitive information about valid/invalid actor IDs. ⚡ Performance Considerations1. Multiple String AllocationsIn let actor_id = segments[2].to_string();
// ...
(Some(segments[4].to_string()), 6)Consider using string slices ( 2. Path Parsing on Every RequestPath parsing happens on every request. Since most paths won't match the actor routing pattern, consider:
Example: pub fn parse_actor_path(path: &str) -> Option<ActorPathInfo> {
// Early return for non-matching paths
if !path.starts_with("/gateway/actors/") {
return None;
}
// ... rest of parsing
}📝 Documentation & Style1. Missing Inline DocumentationThe
2. Import Organization (Adhering to CLAUDE.md)Per CLAUDE.md: "Always add imports at the top of the file inside of inline within the function." The imports in the test files appear correct, but verify all new files follow this pattern. 3. Logging Follows Conventions ✅Good adherence to structured logging patterns: tracing::debug!(?actor_path_info, "routing using path-based actor routing");Lowercase messages with structured fields - matches CLAUDE.md guidelines. 🧪 Testing Recommendations1. Add Integration TestsWhile unit tests are comprehensive, consider adding:
2. Security-focused TestsAdd tests for:
3. Test File Location ✅Test files are correctly placed:
🎯 Recommendations SummaryMust Fix:
Should Fix:
Nice to Have:
📊 Overall AssessmentCode Quality: ⭐⭐⭐⭐ (4/5) This is a solid implementation with excellent test coverage. The primary concerns are the typo that needs fixing and ensuring token validation is implemented before production use. The architecture is clean and maintains backward compatibility well. Great work! 🎉 |
Pull Request ReviewI've reviewed PR #3250 which adds path-based routing for actor traffic. Overall, this is a well-implemented feature with excellent test coverage. Here's my detailed feedback: ✅ Strengths
🔍 Code Quality Issues1. Typo in Header Constant (engine/packages/guard/src/routing/pegboard_gateway.rs:13)pub const X_RIVET_AMESPACE: HeaderName = HeaderName::from_static("x-rivet-namespace");Should be 2. Unused Function ParametersIn pub async fn route_request_path_based(
// ...
_token: Option<&str>, // Unused with underscore prefix
// ...
_headers: &hyper::HeaderMap, // Unused
_is_websocket: bool, // Unused
)While underscored parameters indicate intentionally unused values, consider whether these parameters will be used in the EE version for token validation. If not, they could be removed to simplify the API. 3. Query Parameter RemovalThe PR removes support for passing routing information via query parameters (e.g.,
🛡️ Security Concerns1. Token Validation Not Implemented (Critical if EE not deployed)Both Rust and TypeScript have: // NOTE: Token validation implemented in EE// NOTE: Token validation implemented in EEConcerns:
2. Actor ID ValidationIn let actor_id = Id::parse(actor_id_str).context("invalid actor id in path")?;Good - Actor IDs are validated. However, in the TypeScript version, the actor ID is passed as a string without validation before being used. Consider adding validation. 3. Path Traversal ProtectionThe parser correctly rejects double slashes (
📊 Performance Considerations
🔧 Minor Issues1. Inconsistent Error ContextIn pegboard_gateway.rs:42: let actor_id = Id::parse(actor_id_str).context("invalid actor id in path")?;But in the original header-based routing (line 86): let actor_id = Id::parse(actor_id_str).context("invalid x-rivet-actor header")?;The error messages reference different sources. Consider making error messages more specific about path-based vs header-based routing. 2. Missing DocumentationThe
3. Truncated PR DiffThe diff appears to be truncated at the end: 340→/**
341→ * Parse actor routing information from path
...
// Include qThis suggests there might be incomplete code that wasn't visible in the review. 📝 Recommendations
✨ SummaryThis is a solid implementation with excellent test coverage. The main concerns are:
The feature is well-designed and the fallback to header-based routing ensures backwards compatibility. Once the security and typo issues are addressed, this should be ready to merge. |
NathanFlurry
force-pushed
the
10-23-fix_fix_biome_ignore_path
branch
from
323a773 to
a97aa78
Compare
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
a5f5632 to
82faa7e
Compare
Pull Request Review: Actor Path-Based RoutingThis PR introduces path-based routing for actors as an alternative to header-based routing, allowing traffic to be routed via URL paths like ✅ Strengths
🐛 Bug Found - Typo in Header NameLocation: pub const X_RIVET_AMESPACE: HeaderName = HeaderName::from_static("x-rivet-namespace");Issue: Impact: This constant is defined but never used in this PR. However, it's exported and could cause confusion or errors if someone tries to use it later. The constant name doesn't match the actual header string. Fix: Rename the constant to 🔒 Security Considerations
📊 Performance Considerations
🤔 Code Quality & Best Practices
🧪 Testing Recommendations
📝 Minor Issues
✅ RecommendationApprove with minor fix required: The typo Overall, this is a well-implemented feature with excellent test coverage. The main concern is the typo in the header constant name. After fixing that, this should be good to merge. 🎯 Action Items
|
PR Review: Path-based Actor RoutingThis PR adds path-based routing for actors as an alternative to header-based routing. Overall, this is a well-implemented feature with excellent test coverage. Here's my detailed review: ✅ Strengths1. Excellent Test Coverage
2. Clean Implementation
3. Backwards Compatibility
🔍 Issues & Concerns1. Typo in Header Name
|
NathanFlurry
deleted the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
1 participant
No description provided.