fix(rivetkit): remove incorrect getEndpoint call in metadata handler

[NathanFlurry]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 13, 2025 2:48am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 13, 2025 2:48am
rivet-inspector	Ignored	Preview		Nov 13, 2025 2:48am
rivet-site	Ignored	Preview		Nov 13, 2025 2:48am

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(rivetkit): remove incorrect getEndpoint call in metadata handler #3433 : 2 dependent PRs (#3438 , #3451 ) 👈 (View in Graphite)
• feat(rivetkit): add json protocol support #3432
• chore(rivetkit): add required zod validation for json encoding #3431
• chore(rivetkit): add listing actors by only name to manager api #3430
• refactor(rivetkit): extract WebSocket protocol parsing to shared utility #3429
• refactor(rivetkit): restructure connection lifecycle contexts #3428
• chore(rivetkit): add WebSocketContext and RequestContext #3427
• chore(rivetkit): add conn type #3426
• chore(rivetkit): add connection for raw http #3425
• chore(rivetkit): rename onFetch to onRequest #3424
• refactor(rivetkit): add state manager for conn with symbol-based access #3423
• fix(rivetkit): use loggerWithoutContext for websocket error when actor undefined #3412
• feat(rivetkit): add GET /actors/names #3411
• feat(api-public): expose actor kv api #3410
• chore(rivetkit): auto-generate inspector token for engine driver #3409
• chore(rivetkit): close websockets on out of sequence messages #3404
• fix(rivetkit): properly apply clientEndpoint from metadata lookup #3402
• fix(rivetkit): properly handle msgIndex for hibernatable websocket reconnection #3401
• chore(rivetkit): rename onstart/onstop to onwake/onsleep #3422
• chore(rivetkit): split ActorInstance logic in to multiple classes #3421
• chore(rivetkit): move conns to separate persisted kv keys #3420
• chore(rivetkit): remove deprecated conn tokens & sse support #3419
• main

---

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]

PR Review

Summary

This PR fixes an incorrect usage of getEndpoint in the metadata handler. The change simplifies the logic by removing the fallback to getEndpoint(runConfig) and only returning overrideServerAddress when explicitly set.

Code Quality ✅

Positive aspects:

• Cleaner logic: The simplified approach is more straightforward and easier to understand
• Better separation of concerns: getEndpoint is designed for ClientConfig, not RunnerConfig, so removing it fixes a type mismatch
• Improved documentation: The new comment clearly explains the intent - client should use its current endpoint unless explicitly overridden
• Removes unused import: Properly cleaned up the unused getEndpoint import

Code style: Follows TypeScript conventions and is consistent with the surrounding code.

Correctness Analysis ✅

Before this PR:

• Called getEndpoint(runConfig) with a RunnerConfig parameter
• getEndpoint expects a ClientConfig (see api-utils.ts:19)
• This is a type mismatch that could cause runtime issues since RunnerConfig has different fields than ClientConfig

After this PR:

• Only returns overrideServerAddress when explicitly set
• Returns undefined otherwise, signaling client to use its current configuration
• This aligns with the documented behavior in MetadataResponse interface (line 115-122)

Potential Issues ⚠️

Behavior change consideration:

The old code had a fallback path:

runConfig.overrideServerAddress ?? 
  (runConfig.disableDefaultServer ? undefined : getEndpoint(runConfig))

This would return a default endpoint (http://127.0.0.1:6420) when:

1. No override was set AND
2. Default server was NOT disabled

Question: Is there any client code that relies on receiving a default endpoint from the metadata response? If so, this could be a breaking change.

However, based on the interface documentation, returning undefined is the correct behavior to indicate "use your current endpoint." The old behavior was likely unintentional.

Testing Recommendations 🧪

Consider adding tests for:

1. Metadata response when overrideServerAddress is set
2. Metadata response when overrideServerAddress is undefined
3. Verify client behavior when receiving undefined vs explicit endpoint

Performance ✅

No performance concerns. Removing the function call slightly reduces overhead.

Security ✅

No security concerns identified.

Recommendation: APPROVE ✅

This is a solid fix that corrects a type mismatch and simplifies the logic. The new behavior better matches the documented intent of the clientEndpoint field.

Minor suggestion: Consider adding a unit test to prevent regression of this logic, especially around the overrideServerAddress behavior.

[pkg-pr-new]

More templates

• example-ai-agent
• example-background-jobs
• example-better-auth-external-db
• example-bots
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-counter
• example-counter-serverless
• example-crdt
• example-cursors
• example-cursors-raw-websocket
• example-database
• example-deno
• example-sqlite
• example-elysia
• example-express
• example-freestyle
• example-game
• example-hono
• example-hono-bun
• example-hono-react
• example-kitchen-sink
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc
• example-workflows

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 0bc4b29

[claude]

Code Review - PR #3433

Summary

This PR makes two key improvements to error handling and metadata endpoint logic in RivetKit:

1. Error metadata handling: Changes metadata from nullable to optional (.nullable() → .optional()) in error schemas
2. clientEndpoint logic: Removes incorrect getEndpoint() call from metadata handler

✅ Positive Findings

1. Correct semantic fix for metadata handling
The change from null to undefined for missing metadata is semantically correct:

• Before: Missing metadata was coerced to null (metadata ?? null)
• After: Missing metadata remains undefined

This is better because:

• The JSON serialization now properly omits the field when undefined (cleaner JSON payloads)
• The BARE/CBOR protocol correctly uses null for the wire format (as per protocol requirements)
• Zod schema correctly reflects this with .optional() instead of .nullable()

The implementation correctly handles both serialization formats:

• JSON (rivetkit-typescript/packages/rivetkit/src/actor/protocol/old.ts:302-322): Only includes metadata field if present
• BARE/CBOR (rivetkit-typescript/packages/rivetkit/src/actor/protocol/old.ts:324-343): Converts undefined to null for wire protocol

2. Correct fix for clientEndpoint logic
The removal of getEndpoint(runConfig) call is correct because:

• getEndpoint() expects a ClientConfig but was receiving RunnerConfig (type mismatch)
• The function was from @/remote-manager-driver/api-utils which is meant for client-side endpoint resolution
• The new logic correctly returns overrideServerAddress only when explicitly set
• The comment clearly explains the intent: return undefined so clients use their existing configuration

3. Consistent changes across all affected files

• All 4 files updated consistently (schemas, serializers, deserializers)
• Both error types updated: Error and HttpResponseError

🔍 Observations

1. Type safety consideration
In rivetkit-typescript/packages/rivetkit/src/actor/protocol/old.ts:304, using const val: any = {...} bypasses type checking. Consider defining a proper type:

type ErrorValueJson = {
    group: string;
    code: string;
    message: string;
    actionId: number | null;
    metadata?: unknown;
};

const val: ErrorValueJson = {
    group: value.group,
    code: value.code,
    message: value.message,
    actionId: value.actionId !== undefined ? value.actionId : null,
};

2. Conditional field addition pattern
The pattern of conditionally adding metadata field is correct:

if (value.metadata !== undefined) {
    val.metadata = value.metadata;
}

This ensures the field is completely omitted from JSON when not present, rather than being null or undefined.

✅ Security, Performance, and Quality

• Security: No security concerns identified
• Performance: The changes are performance-neutral or slightly positive (smaller JSON payloads when metadata is absent)
• Best practices: Follows TypeScript/Zod conventions for optional fields
• Test coverage: No tests appear to be modified, but this is a bug fix that corrects existing behavior

📝 Suggestions

1. Type safety: Replace any type with proper interface (minor, non-blocking)
2. Test coverage: Consider adding a test case specifically for errors without metadata to ensure undefined handling is correct
3. Documentation: The inline comments are excellent - they clearly explain the JSON vs BARE/CBOR distinction

Overall Assessment

LGTM ✅

This is a well-executed bug fix that:

• Corrects a type mismatch in the metadata endpoint handler
• Improves semantic correctness of optional field handling
• Maintains consistency across serialization formats
• Includes clear explanatory comments

The changes are low-risk and improve code correctness.

[claude]

PR Review: Remove incorrect getEndpoint call in metadata handler

Summary

This PR removes the getEndpoint call from the metadata handler and simplifies the logic to only return overrideServerAddress when set. This is a good fix that addresses a type mismatch and improves clarity.

Code Quality ✅

Positive aspects:

1. Correct type usage: The old code was calling getEndpoint(runConfig), but getEndpoint expects a ClientConfig (from api-utils.ts:19), not a RunnerConfig. This was a type mismatch that could cause issues.
2. Simplified logic: The new code is clearer - it only returns overrideServerAddress if explicitly set, otherwise undefined/null, which aligns with the documented behavior in the MetadataResponse interface (router.ts:115-123).
3. Improved comments: The updated comment better explains the intent - clients should use their current endpoint unless explicitly overridden.
4. Removed unused import: Properly cleaned up the getEndpoint import.

Correctness ✅

The change is correct based on the codebase patterns:

• In registry/mod.ts:123, overrideServerAddress is set from config.endpoint for serverless runners
• The metadata endpoint should return this value so clients know where to reconnect
• The old logic with disableDefaultServer was confusing and unnecessary - if the server is disabled, clients already know their endpoint

Potential Concerns

1. Behavior change for disableDefaultServer case ⚠️
The old code had special handling when disableDefaultServer was true - it would return undefined. The new code doesn't check this flag.

However, after reviewing the code, this appears to be correct because:

• When disableDefaultServer is true, there's no HTTP server running
• Clients would already need to be configured with the correct endpoint
• Returning null tells them "no change needed, use your current config"

2. Missing test coverage ⚠️
There are no test files for the router or metadata endpoint. Consider adding tests to verify:

• Metadata returns correct clientEndpoint when overrideServerAddress is set
• Metadata returns undefined when overrideServerAddress is not set
• Different runner kinds (serverless vs normal) return appropriate values

Performance Considerations ✅

No performance impact - the change removes a function call and simplifies logic.

Security Considerations ✅

No security concerns. The metadata endpoint is already public and this change doesn't expose any new information.

Recommendations

1. Consider adding tests: The lack of test coverage for this endpoint makes it risky to change behavior. A simple test suite would help catch regressions.

2. Documentation: The JSDoc comment improvement is good, but consider documenting the serverless use case explicitly since that's when this value is typically set (see registry/mod.ts:123).

Verdict

LGTM ✅ - This is a good fix that corrects a type mismatch and simplifies the logic. The behavior change appears intentional and correct based on the documented interface and usage patterns in the codebase.

[vercel]

Deployment failed with the following error:

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

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

[claude]

PR Review: fix(rivetkit): remove incorrect getEndpoint call in metadata handler

Summary

This PR removes the getEndpoint call from the metadata handler and simplifies the clientEndpoint logic. The change correctly addresses an issue where the metadata endpoint was returning the wrong endpoint information.

Code Quality & Best Practices ✅

Positive aspects:

• Clear intent: The updated comment explains the behavior better - returning null to indicate the client should use its current configured endpoint
• Removes unnecessary import (getEndpoint from api-utils)
• Simplifies conditional logic by removing the disableDefaultServer check
• Properly typed - no type safety issues introduced

Observations:
The old code had two issues:

1. Type mismatch: getEndpoint() expects a ClientConfig but was being called with RunnerConfig (these are different types - see src/client/config.ts vs src/registry/run-config.ts)
2. Wrong abstraction: getEndpoint() is designed for client-side use to determine where to connect, not for server-side use to tell clients where to connect

Logic Analysis ✅

Before:

clientEndpoint: runConfig.overrideServerAddress ?? 
  (runConfig.disableDefaultServer ? undefined : getEndpoint(runConfig))

After:

clientEndpoint: runConfig.overrideServerAddress

Why this is correct:

1. Looking at src/remote-manager-driver/mod.ts:82-88, the client only overrides its endpoint when metadataData.clientEndpoint is truthy
2. The clientEndpoint field is meant to tell clients to redirect to a different endpoint (see the JSDoc on line 115-122 of router.ts)
3. If overrideServerAddress is not set, returning undefined correctly signals "no redirect needed"
4. The disableDefaultServer check was likely incorrect - it's about whether to start the default server, not about what endpoint to return
5. Calling getEndpoint(runConfig) was definitely wrong since:
   • It doesn't have an endpoint property (that's on ClientConfig)
   • It would return http://127.0.0.1:6420 which is the default client connection address, not necessarily this server's public address

Security Considerations ✅

No security concerns. The change reduces complexity and potential for confusion about endpoint configuration.

Performance Considerations ✅

Minor improvement: removes an unnecessary function call on every metadata request.

Test Coverage ⚠️

Recommendation: While the fix is correct, consider adding tests for:

1. Metadata response when overrideServerAddress is set
2. Metadata response when overrideServerAddress is not set
3. Ensure client correctly handles undefined vs defined clientEndpoint

To verify current behavior:

# Check for existing metadata endpoint tests
find rivetkit-typescript/packages/rivetkit/tests -name "*.test.ts" -exec grep -l "metadata" {} \;

Documentation ✅

The updated comment is clearer and more accurate than the previous one.

Verdict: ✅ LGTM

This is a good fix that corrects both a type error and a logic error. The simplified code is more maintainable and correctly implements the intended behavior.

Suggested follow-up (optional):

• Add test coverage for the metadata endpoint behavior with different configurations
• Consider adding TypeScript type checking that would have caught the ClientConfig vs RunnerConfig mismatch earlier

[claude]

Code Review

Summary

This PR correctly fixes a type mismatch bug in the metadata handler. The previous code was incorrectly calling getEndpoint(runConfig) which expected a ClientConfig but was receiving a RunnerConfig - these are two different types with different purposes.

What Changed

Before:

clientEndpoint:
    runConfig.overrideServerAddress ??
    (runConfig.disableDefaultServer
        ? undefined
        : getEndpoint(runConfig)),  // ❌ Type error: RunnerConfig passed to function expecting ClientConfig

After:

clientEndpoint: runConfig.overrideServerAddress,  // ✅ Correctly uses RunnerConfig property

Analysis

✅ Correctness

The fix is correct for the following reasons:

1. Type Safety: getEndpoint() in remote-manager-driver/api-utils.ts:19 expects ClientConfig with an endpoint property. RunnerConfig doesn't have this property, causing a type mismatch.

2. Semantic Correctness: Looking at the MetadataResponse interface documentation (router.ts:114-122), clientEndpoint should indicate when the client needs to connect to a different endpoint. The overrideServerAddress field in RunnerConfig (run-config.ts:52) serves exactly this purpose.

3. Logic Simplification: The old logic with disableDefaultServer check was unnecessary complexity. If overrideServerAddress is set, return it; otherwise, return undefined to let clients use their configured endpoint.

✅ Code Quality

• Removes unused import (getEndpoint)
• Simplifies conditional logic
• Improved code comments explain the behavior clearly

✅ Behavior Preservation

The behavior is preserved for the intended use case:

• When overrideServerAddress is set → returns it (same as before when set)
• When overrideServerAddress is not set → returns undefined (clients continue using their current endpoint)

The removal of the disableDefaultServer check is actually a bug fix - that flag controls whether the runner starts its own HTTP server, which is orthogonal to what clientEndpoint should return.

Potential Concerns

None identified. The change is straightforward and correct.

Recommendations

✅ Approve and merge - This is a clean bug fix that:

• Fixes a type error
• Simplifies the logic
• Maintains correct behavior
• Improves code documentation

Testing Considerations

Consider verifying:

1. Serverless runners correctly report their override endpoint when overrideServerAddress is set
2. Normal runners return undefined when no override is configured
3. Clients respect the clientEndpoint field from the metadata response

Great catch on this type mismatch! 🎯

[graphite-app]

Merge activity

• Nov 13, 9:15 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 13, 9:16 PM UTC: CI is running for this pull request on a draft pull request (#3465) due to your merge queue CI optimization settings.
• Nov 13, 9:16 PM UTC: Merged by the Graphite merge queue via draft PR: #3465.