Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Jan 1, 2026

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

Project Deployment Review Updated (UTC)
rivetkit-serverless Error Error Jan 1, 2026 1:45am
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 1, 2026 1:45am
rivet-inspector Ignored Ignored Preview Jan 1, 2026 1:45am
rivet-site Ignored Ignored Preview Jan 1, 2026 1:45am

Copy link
Contributor Author

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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 1, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: a64352e

@claude
Copy link

claude bot commented Jan 1, 2026

PR Review: Fix gateway token reading, rivetkit path token, vergen git sha

This PR addresses three important fixes related to token handling, build configuration, and observability. Overall the changes are well-structured and address real issues, but there are a few areas that need attention.

Code Quality & Best Practices

✅ Strengths

  1. Improved token extraction logic - The refactoring in pegboard_gateway.rs properly handles both path-based and header-based token reading with fallback logic
  2. Build consolidation - Moving vergen configuration from rivet-env to rivet-util is a good consolidation that reduces duplication
  3. Git SHA override - The OVERRIDE_GIT_SHA environment variable support in Docker builds is a practical solution for CI/CD scenarios

⚠️ Issues & Concerns

1. Unused parameter in route_request_inner (packages/guard/src/routing/pegboard_gateway.rs:157)

The _token parameter is prefixed with underscore but the comment says "NOTE: Token validation implemented in EE". This suggests:

  • The parameter is intentionally unused in the open-source version
  • However, it's being passed through the call chain but never validated

Recommendation: Consider documenting this more clearly or adding a compile-time feature flag check to make the intent explicit.

2. Error filtering inconsistency (grafana dashboards)

The Prometheus queries for 4xx errors still exclude API_CANCELLED errors:

error!="API_CANCELLED"

But the 5xx error queries now include all errors. The last panel also removes the error!="API_CANCELLED" filter. This inconsistency could lead to:

  • Confusing metrics where cancelled requests are counted differently across panels
  • Potential noise in error rates during normal operation

Recommendation: Be consistent about how API_CANCELLED errors are handled across all queries, or document why they're treated differently.

3. Serde rename comment without fix (packages/pegboard/src/workflows/runner_pool.rs:143)

// Should have `#[serde(rename_all = "snake_case")]` but doesn't

This comment acknowledges a serialization issue but doesn't fix it. This suggests:

  • There may be existing data serialized in the old format
  • Changing it now could break compatibility
  • But leaving it unfixed creates technical debt

Recommendation: Either:

  • Fix the issue and add migration logic if needed
  • Document WHY it can't be fixed (e.g., "Breaking change for existing workflows")
  • Create a follow-up issue to address this properly

4. Script argument handling (scripts/run/docker/engine-rocksdb.sh)

The new FILTERED_ARGS array doesn't actually filter anything - it just copies all arguments:

FILTERED_ARGS=()
for arg in "$@"; do
	FILTERED_ARGS+=("$arg")
done

This is identical to just using "$@" directly. Either this is incomplete or unnecessary.

Recommendation: Either implement actual filtering logic or remove this code and use "$@" directly.

Security Concerns

Token handling in WebSocket protocols

The changes improve token handling, but there are security considerations:

  1. Path-based tokens (/gateway/${actorId}@${token}${path}) expose tokens in URLs, which:

    • May be logged in proxy/gateway access logs
    • Could appear in browser history
    • Are visible in network inspection tools
  2. WebSocket protocol header tokens are better but still transmitted as plain text

Recommendation:

  • Document that path-based tokens should only be used over HTTPS
  • Consider adding a warning or deprecation notice for path-based token auth
  • Ensure tokens are sanitized from logs

Performance Considerations

The changes don't introduce performance regressions. The token extraction logic is straightforward with minimal overhead.

Testing

Missing test coverage for:

  1. Token extraction from sec-websocket-protocol header with multiple protocols
  2. Fallback from path token to header token
  3. URL encoding in actor IDs within WebSocket protocols
  4. Git SHA override in build process

Recommendation: Add integration tests for the token extraction logic, especially the WebSocket protocol parsing.

Minor Issues

  1. Debug mode enabled (Cargo.toml): The change from debug = false to debug = true increases binary size and may impact performance. Is this intentional for development only?

  2. Grafana dashboard regeneration: Multiple identical dashboard files were updated. Per CLAUDE.md, these should be regenerated from the template. Confirm the template was updated first.

  3. TypeScript change (actor-websocket-client.ts:29): The path token format changed from inline to conditional. This looks correct but ensure all callers are updated.

Recommendations Summary

Must fix:

  • Clarify or remove the incomplete FILTERED_ARGS logic in the script
  • Document the _token parameter usage pattern for OSS vs EE

Should fix:

  • Make API_CANCELLED error filtering consistent across Grafana queries
  • Either fix or properly document the serde rename_all issue
  • Add tests for token extraction logic

Consider:

  • Document security implications of path-based token auth
  • Review if debug = true should remain in the main branch

Overall this is a solid bugfix PR that addresses real issues. The main concerns are around incomplete changes (script args, serde comment) and security documentation for token handling.

@MasterPtato MasterPtato force-pushed the 12-31-fix_fix_gateway_token_reading_rivetkit_path_token_vergen_git_sha branch from 7c1841f to a64352e Compare January 1, 2026 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants