Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 4, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 4, 2025 9:00pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 4, 2025 9:00pm
rivet-inspector Ignored Ignored Preview Nov 4, 2025 9:00pm
rivet-site Ignored Ignored Preview Nov 4, 2025 9:00pm

This was referenced Nov 4, 2025
Copy link
Member Author

NathanFlurry commented Nov 4, 2025

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.

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR updates the default engine version from a hardcoded value (25.8.2) to use the package version from package.json (VERSION constant). This ensures the auto-start engine version stays synchronized with the RivetKit package version.

✅ Positive Observations

  1. Better Maintainability: Using VERSION instead of hardcoded 25.8.2 ensures the engine version automatically updates with package releases, reducing manual maintenance burden.

  2. Consistent Pattern: The VERSION constant is already properly exported from src/utils.ts (line 9) and imported from package.json, which is a clean approach.

  3. Backward Compatibility: The change preserves the existing behavior - the environment variable RIVET_RUN_ENGINE_VERSION still takes precedence if set, falling back to the version constant only when not specified.

  4. Minimal Change: The PR is appropriately scoped - just 2 lines changed in a single file.

🔍 Considerations & Recommendations

1. Version Synchronization Assumption

The change assumes that the RivetKit package version (currently 2.0.22-rc.1) should match the engine version. This works well if:

  • The engine and RivetKit are released together with matching versions
  • The engine is compatible with RivetKit at the same version number

Question: Is the RivetKit version always compatible with the same-versioned engine? If they have different versioning schemes or release cycles, this could cause issues.

2. Test Coverage

No tests were added or modified for this change. Consider adding a test to verify:

  • The default runEngineVersion uses VERSION when RIVET_RUN_ENGINE_VERSION is not set
  • The environment variable still takes precedence when set

Example test location: tests/engine-process.test.ts

3. Documentation

Consider documenting this behavior in:

  • The RunnerConfigSchema comments (src/registry/run-config.ts:44)
  • Any user-facing documentation about engine versioning

Current comment just says @experimental - could add:

/** 
 * @experimental 
 * Version of the engine to auto-start. 
 * Defaults to the RivetKit package version unless RIVET_RUN_ENGINE_VERSION is set.
 */

4. Pre-release Version Handling

The current package version is 2.0.22-rc.1 (a release candidate). Ensure the engine build/download logic can handle:

  • Pre-release version tags (-rc.1, -beta, etc.)
  • Development versions

The ensureEngineProcess function (src/registry/mod.ts:112-113) should be verified to handle these version formats.

🎯 Overall Assessment

Recommendation: Approve with minor suggestions

The change is well-scoped and improves maintainability. The main considerations are:

  1. Verifying version compatibility assumptions
  2. Adding test coverage
  3. Handling pre-release versions appropriately

The code change itself is clean and follows existing patterns in the codebase.


Note: This is an automated review. Please validate the version compatibility assumptions and consider the testing recommendations.

@NathanFlurry NathanFlurry force-pushed the 11-04-chore_guard_handle_cors_on_the_gateway branch from 70ec891 to e7f7c04 Compare November 4, 2025 20:25
@NathanFlurry NathanFlurry force-pushed the 11-04-chore_rivetkit_auto-update_auto-start_engine_version branch from 700a49a to 34d8908 Compare November 4, 2025 20:25
@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

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

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

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review - chore(rivetkit): auto-update auto-start engine version

Overview

This PR replaces a hardcoded engine version string (25.8.2) with a dynamic reference to the package's VERSION constant, which is derived from package.json. This is a good improvement for maintainability.

✅ Positive Aspects

  1. Maintainability: Using VERSION from package.json eliminates the need to manually update the default engine version in multiple places when releasing new versions.

  2. Consistency: The VERSION constant is already used throughout the codebase (e.g., in httpUserAgent() at utils.ts:20, EXTRA_ERROR_LOG at utils.ts:271, and welcome messages at mod.ts:187,197), so this change aligns with existing patterns.

  3. Code Quality: The change is minimal, focused, and follows the single responsibility principle.

  4. Import Pattern: The import statement correctly adds VERSION to the existing import from @/utils, following the project's pattern of consolidating imports from the same module.

🤔 Considerations

  1. Semantic Versioning Implications: The current package version is 2.0.22-rc.1 (a release candidate). When this becomes the default runEngineVersion, users who don't explicitly set RIVET_RUN_ENGINE_VERSION will automatically use this RC version.

    • Question: Is the engine version intended to always match the RivetKit package version? Or should engine versions follow an independent versioning scheme?
    • Consideration: If engine and RivetKit versions diverge in the future, this tight coupling might need revisiting.
  2. Environment Variable Override: The change maintains the existing behavior where RIVET_RUN_ENGINE_VERSION env var takes precedence, which is good. However, there's no validation that the version string format is valid.

  3. Testing: I couldn't find any tests that verify the default runEngineVersion behavior. Consider adding a test to ensure:

    • The default value matches the package version when no env var is set
    • The env var override works correctly
    • The version string is used correctly when starting the engine (see mod.ts:112-114)
  4. Documentation: The runEngineVersion field is marked as @experimental. Consider documenting in the code or changelog that the default value now dynamically matches the RivetKit version.

🔒 Security & Performance

  • No security concerns identified
  • No performance impact (this is a config-time constant resolution)

📝 Suggested Improvements (Optional)

  1. Add a test case for the default version behavior:
describe('RunnerConfigSchema', () => {
  it('should default runEngineVersion to VERSION when env var not set', () => {
    delete process.env.RIVET_RUN_ENGINE_VERSION;
    const config = RunnerConfigSchema.parse({});
    expect(config.runEngineVersion).toBe(VERSION);
  });
});
  1. If engine versioning should be independent from RivetKit versioning in the future, consider introducing a separate ENGINE_VERSION constant or configuration.

✅ Verdict

This is a solid improvement that reduces maintenance burden and follows existing patterns in the codebase. The change is safe to merge, though you may want to consider the semantic versioning implications and test coverage suggestions above.

Approval: ✅ Approved with minor suggestions for consideration.

@graphite-app graphite-app bot force-pushed the 11-04-chore_guard_handle_cors_on_the_gateway branch 2 times, most recently from 63d1984 to c9797c4 Compare November 4, 2025 20:58
@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: Auto-update auto-start engine version

Summary

This PR replaces the hardcoded engine version (25.8.2) with a dynamic reference to the package version (VERSION from package.json). This ensures the engine version stays in sync with the RivetKit package version automatically.

Code Quality ✅

Positive aspects:

  • Clean, minimal change that follows the DRY principle
  • Properly imports VERSION alongside existing import from @/utils
  • The VERSION constant is already exported and used elsewhere in the codebase (e.g., httpUserAgent(), EXTRA_ERROR_LOG)
  • Maintains backward compatibility with the RIVET_RUN_ENGINE_VERSION environment variable override

Observations:

  • Current package version is 2.0.22-rc.1, which will replace the previous hardcoded 25.8.2
  • This appears to be an intentional version change as part of a larger versioning strategy update

Functionality ✅

The change maintains the same logic flow:

  1. First checks for RIVET_RUN_ENGINE_VERSION environment variable
  2. Falls back to VERSION (previously hardcoded 25.8.2)

The VERSION is sourced from package.json via:

import pkgJson from "../package.json" with { type: "json" };
export const VERSION = pkgJson.version;

This value is used by ensureEngineProcess() to:

  • Download the correct engine binary from https://releases.rivet.gg/engine/{version}/
  • Cache the binary with version-specific naming
  • Log version information on startup

Potential Concerns ⚠️

  1. Version Mismatch Risk: If the package version doesn't match an available engine release, the download will fail. Consider:

    • Does every RivetKit version have a corresponding engine release?
    • What happens during pre-release versions (e.g., 2.0.22-rc.1)? Does the engine release URL support rc versions?
  2. Testing: The change would benefit from:

    • Verification that engine binary exists at https://releases.rivet.gg/engine/2.0.22-rc.1/
    • Test case validating the default version fallback works correctly
    • Documentation of the version sync strategy
  3. Breaking Changes: If this changes the default engine version significantly (from 25.x to 2.x), users who don't specify RIVET_RUN_ENGINE_VERSION will get a different engine version. Consider:

    • Is this a breaking change?
    • Should there be migration documentation?

Security ✅

No security concerns. The change doesn't introduce new attack vectors and maintains the same data flow.

Performance ✅

No performance impact. The VERSION constant is loaded once at module initialization.

Test Coverage 📝

Recommendation: Add a test case to verify the default engine version behavior:

describe('RunnerConfigSchema', () => {
  it('defaults runEngineVersion to package VERSION', () => {
    const config = RunnerConfigSchema.parse({});
    expect(config.runEngineVersion).toBe(VERSION);
  });
  
  it('allows RIVET_RUN_ENGINE_VERSION env override', () => {
    // Test with mocked env variable
  });
});

Recommendations

  1. Verify engine release availability: Ensure that https://releases.rivet.gg/engine/2.0.22-rc.1/rivet-engine-{target} exists for all supported platforms
  2. Add test coverage: Create tests for the default version behavior
  3. Update documentation: If this changes the versioning strategy, document it in CHANGELOG or migration guide
  4. Consider version validation: Add a startup warning if the engine version doesn't follow expected semver format

Verdict: ✅ Approve with minor suggestions

The change itself is well-implemented and improves maintainability by eliminating a hardcoded version number. The main concern is ensuring the engine release infrastructure supports this automatic versioning strategy, particularly for pre-release versions.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 5, 2025

Merge activity

  • Nov 5, 12:08 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 5, 12:08 AM UTC: CI is running for this pull request on a draft pull request (#3364) due to your merge queue CI optimization settings.
  • Nov 5, 12:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3364.

@graphite-app graphite-app bot closed this Nov 5, 2025
@graphite-app graphite-app bot deleted the 11-04-chore_rivetkit_auto-update_auto-start_engine_version branch November 5, 2025 00:11
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.

3 participants