Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 21, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 21, 2025 5:21am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 21, 2025 5:21am
rivet-inspector Ignored Ignored Preview Nov 21, 2025 5:21am
rivet-site Ignored Ignored Preview Nov 21, 2025 5:21am

Copy link
Member Author

NathanFlurry commented Nov 21, 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 21, 2025

Code Review

Summary

This PR centralizes build metadata generation by moving vergen build script from api-public to util package and exposes it as a shared build_meta module. It also adds a long version flag to the engine CLI with detailed build information.

✅ Strengths

  1. Good refactoring: Consolidating build metadata into a shared utility module (rivet_util::build_meta) eliminates code duplication and makes it easier to use across multiple packages.

  2. Clean architecture: Moving the build script from api-public to util makes sense since util is a common dependency. This follows the monorepo's dependency pattern correctly.

  3. Proper workspace dependency usage: Correctly uses once_cell.workspace = true for the new dependency, following the project's dependency management conventions.

  4. Improved UX: Adding --version --long support to the CLI provides more detailed version information for debugging and support.

  5. Maintains API compatibility: The metadata endpoint continues to return the same JSON structure, just sourcing from a different location.

🔍 Code Quality Observations

engine/packages/util/src/build_meta.rs:28-32

The cargo_profile() function compares strings at runtime. Consider using a const evaluation instead:

pub const fn cargo_profile() -> &'static str {
    match CARGO_DEBUG {
        "true" => "debug",
        _ => "release"
    }
}

However, the current implementation is fine since it's simple and clear.

engine/packages/engine/src/main.rs:9-20

The LONG_VERSION uses Lazy<String> which allocates at runtime. Since this is only used once during CLI parsing, this is acceptable. Alternative approaches (like a const format macro) would be more complex without significant benefit.

📝 Minor Suggestions

  1. Documentation: Consider adding a module-level doc comment to build_meta.rs explaining its purpose:

    //! Build metadata constants generated at compile time via vergen.
    //!
    //! This module provides centralized access to build information including
    //! version, git SHA, and compilation details.
    
  2. Consistency: The RUNTIME constant is hardcoded as "engine". If other packages might use build_meta, consider:

    • Making it configurable via environment variable
    • Or documenting that it's specifically for the engine package
  3. Test coverage: Consider adding a simple integration test to verify the metadata endpoint returns expected fields (though this is minor since it's a straightforward refactor).

🔒 Security

No security concerns identified. The build metadata is public information intended to be exposed.

⚡ Performance

No performance concerns. The changes are compile-time or one-time initialization costs.

🧪 Testing

  • No tests were added, but the refactor is straightforward
  • The existing /metadata endpoint provides implicit integration testing
  • Manual testing with cargo run --bin engine -- --version and cargo run --bin engine -- --version --long would verify the CLI changes

✅ Overall Assessment

This is a clean, well-executed refactor that improves code organization and adds useful functionality. The changes follow the project's conventions (workspace dependencies, error handling, code style) and maintain backward compatibility.

Recommendation: Approve

The code is ready to merge. The minor suggestions above are optional improvements, not blockers.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

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

graphite-app bot pushed a commit that referenced this pull request Nov 21, 2025
@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-feat_engine_add_long_--version_details branch November 21, 2025 05:26
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