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 10, 2025 9:20pm

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 10, 2025 9:20pm
rivet-inspector	Ignored	Preview		Nov 10, 2025 9:20pm
rivet-site	Ignored	Preview		Nov 10, 2025 9:20pm

[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

• feat(site): add typedoc docs #3446
• chore(rivetkit): remove deprecated packages #3447
• chore(rivetkit): add support for conn params on onRequest and onWebSocket #3445
• feat(rivetkit): add asyncapi spec #3442
• chore(rivetkit): add actor router to the openapi spec #3441
• fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440
• fix(rivetkit): fix race condition with websocket open events #3439
• fix(rivetkit): standardize error metadata return type #3438
• fix(rivetkit): remove incorrect getEndpoint call in metadata handler #3433 👈 (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: 46a76b7

[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]

Code Review

Summary

This PR fixes a type mismatch bug where getEndpoint(runConfig) was being called with a RunnerConfig parameter, but getEndpoint expects a ClientConfig parameter. The fix simplifies the logic to directly use runConfig.overrideServerAddress, which is the correct approach.

Positive Aspects ✅

1. Correct Bug Fix: The removal of the incorrect getEndpoint() call fixes a type mismatch:

   • getEndpoint() in api-utils.ts:19 expects ClientConfig (which has an endpoint field)
   • runConfig is of type RunnerConfig (which does NOT have an endpoint field)
   • RunnerConfig only has overrideServerAddress, which is what should be used

2. Improved Code Clarity: The new implementation is clearer:

   • Directly returns runConfig.overrideServerAddress
   • Updated comments accurately describe the behavior
   • Removes unnecessary conditional logic around disableDefaultServer

3. Better Semantics: The new approach makes more sense:

   • Only override the client endpoint when explicitly configured via overrideServerAddress
   • Returning undefined signals to the client to keep using its current endpoint
   • This is the correct behavior as documented in the MetadataResponse interface (lines 114-123)

4. Clean Imports: Properly removes the unused import of getEndpoint

Recommendations 📝

1. Consider Adding Tests

There are no tests covering the handleMetadataRequest function. Consider adding unit tests to verify:

• When overrideServerAddress is set, it's returned as clientEndpoint
• When overrideServerAddress is undefined, clientEndpoint is undefined
• Different runnerKind values are correctly reflected in the response

Example test structure:

describe('handleMetadataRequest', () => {
  it('returns overrideServerAddress when set', () => {
    const runConfig = { overrideServerAddress: 'http://custom:8080', ... };
    const response = handleMetadataRequest(mockContext, mockRegistry, runConfig);
    expect(response.clientEndpoint).toBe('http://custom:8080');
  });

  it('returns undefined clientEndpoint when overrideServerAddress is not set', () => {
    const runConfig = { overrideServerAddress: undefined, ... };
    const response = handleMetadataRequest(mockContext, mockRegistry, runConfig);
    expect(response.clientEndpoint).toBeUndefined();
  });
});

2. TypeScript Strictness

This bug highlights that TypeScript didn't catch the type mismatch. Consider:

• Reviewing TypeScript compiler settings in tsconfig.json
• Ensuring strict: true is enabled
• This would have caught the error at compile time

3. Documentation

The updated comments are good, but could be enhanced:

// If server address is explicitly overridden, return it so the client
// can redirect to the new endpoint. Otherwise, return undefined to
// indicate the client should continue using its current endpoint.
clientEndpoint: runConfig.overrideServerAddress,

Security & Performance ✅

• No security concerns: The change doesn't introduce any security vulnerabilities
• No performance impact: Actually improves performance slightly by removing unnecessary function call and conditional logic

Conclusion

This is a clean and correct bug fix. The main improvement would be adding test coverage to prevent similar issues in the future.

Approval: ✅ Approved with recommendation to add tests

---

Review generated with assistance from Claude Code