Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 28, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Oct 31, 2025 1:23am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 31, 2025 1:23am
rivet-inspector Ignored Ignored Preview Oct 31, 2025 1:23am
rivet-site Ignored Ignored Preview Oct 31, 2025 1:23am

Copy link
Member Author

NathanFlurry commented Oct 28, 2025


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 Oct 28, 2025

PR Review: Add Build Metadata to API Public

Thank you for this contribution! This PR adds useful build metadata to the public API's metadata endpoint. Overall the implementation is solid, but I have some suggestions and concerns to address.

Positives

  1. Good use of workspace dependencies: Properly uses .workspace = true for dependencies
  2. Comprehensive metadata: Includes useful information (git SHA, build timestamp, rustc version, target triple, profile)
  3. Proper error handling: Build script correctly returns Result and propagates errors
  4. Follows existing patterns: Mirrors the existing rivet-env package's use of vergen

Issues and Concerns

1. Security Concern: Information Disclosure (Severity: Medium)

The metadata endpoint exposes detailed build information including Git SHA, build timestamp, rustc version and host, and build profile. This information could aid attackers in identifying specific versions with known vulnerabilities or determining if a debug build is running.

Recommendation: Consider whether all this information needs to be public or if some fields should be restricted to authenticated/admin users.

Location: engine/packages/api-public/src/metadata.rs:13-21

2. Missing OpenAPI Documentation

The /metadata endpoint is not included in the OpenAPI spec defined in router.rs. While the endpoint exists and is routed, it's not documented in the openapi paths macro.

Recommendation: Add metadata::get_metadata to the OpenAPI paths list

Location: engine/packages/api-public/src/router.rs:14-32

3. Potential Build Failure with Missing Git Repository

The build script uses vergen_gitcl::GitclBuilder::all_git() which may fail if the code is built from a tarball/zip without git history, the .git directory is missing, or git is not installed.

Recommendation: Consider using .idempotent() or handling git metadata gracefully

Location: engine/packages/api-public/build.rs:13

4. Testing: No Test Coverage

The metadata endpoint has no tests. While this is a simple endpoint, tests would verify the endpoint returns 200 OK, all expected fields are present, and the skip_auth behavior works correctly.

Recommendation: Add integration tests for the metadata endpoint.

5. Minor: Inconsistent Vergen Usage

The rivet-env package uses vergen with default configuration while rivet-api-public uses it with specific features and git metadata.

Recommendation: Consider whether rivet-env should be updated to use the same pattern for consistency.

Additional Suggestions

  1. Documentation: Add a doc comment explaining what metadata is returned and why
  2. Type Safety: Consider creating a typed response struct instead of json!({})
  3. Cargo.toml Consistency: The workspace version was updated from 25.8.1 to 25.8.2. Ensure this is intentional.

Questions

  1. Is the metadata endpoint intended to be public (no authentication)? Given the security implications, please confirm this is intentional.
  2. Are there specific use cases for exposing git SHA and build timestamps?
  3. Should the /metadata endpoint be documented in the OpenAPI spec?

Summary

The implementation is functional and follows codebase conventions well. Main concerns are security (information disclosure), robustness (potential build failures without git), and documentation (missing OpenAPI spec and tests).

Review Status: Changes Requested

cc @NathanFlurry

Base automatically changed from 10-27-chore_scripts_add_ping_actor_script to main October 31, 2025 01:19
@NathanFlurry NathanFlurry force-pushed the 10-27-chore_api-public_return_build_metadata branch from cc21086 to 727eeb1 Compare October 31, 2025 01:22
@NathanFlurry NathanFlurry merged commit d9d781e into main Oct 31, 2025
7 of 10 checks passed
@NathanFlurry NathanFlurry deleted the 10-27-chore_api-public_return_build_metadata branch October 31, 2025 01:24
@claude
Copy link

claude bot commented Oct 31, 2025

Pull Request Review

Overview

This PR adds build metadata to the public API's /metadata endpoint by integrating vergen and vergen-gitcl to expose git commit SHA, build timestamp, Rust compiler version, and build profile information.

Code Quality ✅

Positive aspects:

  • Clean integration of vergen for build-time metadata generation
  • Proper error handling in build.rs with Result<()> return type
  • Follows Rust conventions for build scripts
  • Correctly uses workspace dependencies per CLAUDE.md guidelines
  • Structured logging with skip_all already in place for the endpoint

Adherence to CLAUDE.md:

  • ✅ Uses workspace dependencies correctly (vergen.workspace = true)
  • ✅ Follows error handling pattern with anyhow::Result
  • ✅ Maintains existing code style and patterns

Potential Issues & Recommendations

1. Error Handling Consistency (Minor)

In build.rs:16-17, the existing code uses .unwrap() while the new vergen code uses ? for error propagation. Consider updating the unwrap() calls for consistency, though unwrap() is generally acceptable in build scripts for environment variables that should always be present.

2. Missing Cargo Rebuild Triggers (Medium Priority)

The build script does not tell Cargo to rebuild when git state changes. Without rebuild triggers for .git/HEAD and .git/refs, the git SHA and other metadata may become stale between commits unless a clean build is performed.

3. Potential Build Failure in Non-Git Environments (High Priority)

The vergen_gitcl::GitclBuilder::all_git()? call may fail in environments without git or when building from a tarball/archive (common in CI/CD or package distributions). Consider handling the git builder separately with fallback logic to avoid build failures.

4. Compile-time Panic Risk (High Priority)

In metadata.rs:16-21, all the env!() macro calls will cause a compile-time panic if the environment variables are not set. If the git builder fails (see issue #3), this will prevent compilation. Consider using option_env!() for git-related variables with fallback values like "unknown".

5. API Documentation Missing (Low Priority)

The get_metadata endpoint is not included in the OpenAPI documentation (router.rs:15-32). Consider adding it to the #[openapi(paths(...))] macro if this endpoint should be publicly documented.

Security Considerations ✅

  • Information Disclosure: The metadata exposed (git SHA, build info) is generally safe for public APIs and beneficial for debugging and version tracking
  • Authentication: Endpoint correctly calls ctx.skip_auth(), which is appropriate for public metadata
  • No security concerns identified

Performance Considerations ✅

  • Build-time code generation has zero runtime overhead
  • Metadata endpoint returns static strings compiled into the binary
  • No database queries or external calls
  • Performance impact: negligible

Test Coverage ⚠️

Missing:

  • No tests found for the metadata endpoint
  • No validation that the build metadata is correctly set

Recommendation:
Add a basic integration test to verify the endpoint returns expected fields and that all metadata is present.

Summary

Overall Assessment: Good with recommendations ⭐⭐⭐⭐

The implementation is clean and follows project conventions well. The main concerns are around build robustness in non-git environments and potential compilation failures. Recommend addressing issues #3 and #4 before merging to prevent build breakage in CI/CD pipelines or when building from source archives.

Suggested Action Items:

  1. 🔴 High Priority: Add fallback handling for git metadata in non-git environments
  2. 🔴 High Priority: Use option_env!() for git-related variables or ensure builds always occur in git repos
  3. 🟡 Medium Priority: Add cargo rebuild triggers for git changes
  4. 🟢 Low Priority: Add the endpoint to OpenAPI documentation
  5. 🟢 Low Priority: Add basic test coverage for the endpoint

Review generated with Claude Code

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