Skip to content

feat(rivetkit): enable configuring manager host#3969

Closed
NathanFlurry wants to merge 1 commit intomainfrom
01-17-feat_rivetkit_enable_configuring_manager_host
Closed

feat(rivetkit): enable configuring manager host#3969
NathanFlurry wants to merge 1 commit intomainfrom
01-17-feat_rivetkit_enable_configuring_manager_host

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member Author


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.

@railway-app
Copy link
Copy Markdown

railway-app bot commented Jan 18, 2026

🚅 Deployed to the rivet-pr-3969 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jan 18, 2026 at 2:42 am
frontend-cloud 😴 Sleeping (View Logs) Web Jan 18, 2026 at 2:40 am
frontend-inspector 😴 Sleeping (View Logs) Web Jan 18, 2026 at 2:39 am

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3969 January 18, 2026 02:30 Destroyed
@claude
Copy link
Copy Markdown

claude bot commented Jan 18, 2026

Pull Request Review

Summary

This PR adds the ability to configure the host/hostname that the manager server binds to via a new managerHost configuration option. The implementation is clean and consistent across all three runtime platforms (Node.js, Deno, and Bun).

Code Quality: ✅ Good

Strengths:

  • Consistent implementation across all three runtimes (Node.js, Deno, Bun)
  • Properly marked as @experimental to signal this is a new feature
  • Good use of structured logging by including hostname in the log output
  • Clean, minimal changes with no unnecessary refactoring

Suggestions:

  1. Documentation schema is incomplete (rivetkit-typescript/packages/rivetkit/src/registry/config/index.ts:336)

    • The DocRegistryConfigSchema is missing the new managerHost field
    • Add this after managerPort:
    managerHost: z.string().optional().describe("Host to bind the manager server to. Default: undefined (binds to all interfaces)"),
  2. Consider adding a default value (rivetkit-typescript/packages/rivetkit/src/registry/config/index.ts:134)

    • Currently managerHost has no default, which means it will be undefined
    • This is acceptable behavior (servers typically bind to all interfaces when hostname is undefined)
    • However, for consistency with other config options and clarity, you might consider:
    managerHost: z.string().optional().describe("Host to bind the manager server to. When undefined, binds to all available network interfaces."),
    • Update the description to explicitly document the undefined behavior
  3. Hostname logging may expose sensitive info

    • The hostname is now logged at info level in all three serve functions
    • If users configure a hostname that contains sensitive information, this will be logged
    • This is probably fine since it's info level and controlled by the user, but worth noting

Potential Bugs: ✅ None identified

The implementation correctly passes the hostname parameter to all three runtime serve functions. The optional nature of the field is handled properly.

Performance: ✅ No concerns

No performance impact. The change only affects server initialization.

Security: ⚠️ Minor consideration

Binding behavior:

  • When managerHost is undefined, servers will bind to all network interfaces (0.0.0.0)
  • When specified, servers will only bind to that specific interface
  • This is expected behavior, but users should be aware:
    • Not setting managerHost = accessible from all interfaces (less secure)
    • Setting to 127.0.0.1 or localhost = only accessible locally (more secure)

Recommendation: Add a security note to the JSDoc comment explaining the security implications of different hostname values.

/**
 * @experimental
 *
 * What host to bind the manager server to.
 * 
 * Security: When undefined, the server binds to all network interfaces,
 * making it accessible from external connections. Set to '127.0.0.1' or
 * 'localhost' to restrict access to local connections only.
 */
managerHost: z.string().optional(),

Test Coverage: ⚠️ No tests included

This PR doesn't include tests for the new configuration option. Consider adding:

  • Unit test verifying the config schema accepts managerHost
  • Integration test verifying the server binds to the specified host (may be complex to test reliably)

At minimum, the schema validation should be tested.

Overall Assessment: ✅ Approved with suggestions

This is a solid, straightforward implementation. The main issues are:

  1. Missing documentation schema entry (should be fixed before merge)
  2. Security documentation could be clearer
  3. Consider adding basic tests

The code follows the project's patterns and style guidelines from CLAUDE.md. The feature is appropriately marked as experimental.

Recommendation: Fix the documentation schema issue, then merge. The other suggestions are nice-to-haves but not blockers.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Jan 18, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3969

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: e4194b6

@NathanFlurry NathanFlurry marked this pull request as ready for review January 22, 2026 01:44
@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app bot commented Jan 22, 2026

Merge activity

  • Jan 22, 1:44 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 22, 1:44 AM UTC: CI is running for this pull request on a draft pull request (#3996) due to your merge queue CI optimization settings.
  • Jan 22, 1:45 AM UTC: Merged by the Graphite merge queue via draft PR: #3996.

@graphite-app graphite-app bot closed this Jan 22, 2026
@graphite-app graphite-app bot deleted the 01-17-feat_rivetkit_enable_configuring_manager_host branch January 22, 2026 01:45
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.

1 participant