Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Dec 1, 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 Dec 1, 2025 9:30pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 1, 2025 9:30pm
rivet-inspector Ignored Ignored Preview Dec 1, 2025 9:30pm
rivet-site Ignored Ignored Preview Dec 1, 2025 9:30pm

Copy link
Contributor Author

MasterPtato commented Dec 1, 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 Dec 1, 2025

PR Review: Formalize /metadata Endpoint

Summary

This PR formalizes the /metadata endpoint by adding proper OpenAPI documentation, type-safe response structures, and consistent error handling. It also includes cleanup of unused protobuf dependencies and formatting fixes.

Code Quality

Strengths:

  • Proper use of strongly-typed response struct instead of json!() macro
  • Follows repository patterns for API handlers (inner function + error handling wrapper)
  • Good use of utoipa for OpenAPI documentation
  • Consistent with other endpoint implementations in the codebase

Issues Found:

CRITICAL BUG - Wrong HTTP Method in OpenAPI Doc

In engine/packages/api-public/src/metadata.rs line 26, the utoipa annotation specifies delete but the actual route is GET /metadata (line 57 in router.rs). This will generate incorrect OpenAPI documentation.

Fix: Change line 26 from delete to get

Performance Considerations

  • Minimal allocations (only string conversions)
  • No database queries or external calls
  • Very lightweight endpoint - good performance profile

Security Concerns

  • Correctly uses ctx.skip_auth() since this is public metadata
  • No sensitive information exposed (build metadata is safe to share)
  • Follows auth handling pattern properly

Best Practices

Minor Issues:

  1. Unnecessary String Allocations: The response converts all static str constants to String. Since build metadata are compile-time constants, consider using static str in the response struct to avoid allocations.

  2. OpenAPI Schema Naming: The schema alias MetadataGetResponse follows good naming conventions.

Dependency Cleanup

  • Removal of unused protobuf dependency is good housekeeping
  • Properly removed from both workspace and package manifests
  • Removed from all Docker build configurations

Formatting Changes

The tab indentation changes in various files align with the repository's rustfmt.toml configuration (hard tabs). These are good cleanup changes.

Test Coverage

No tests were added for the formalized endpoint. Consider adding:

  • Integration test to verify response structure
  • Test to ensure all build metadata fields are populated
  • OpenAPI schema validation test

Recommendations

Must Fix:

  • Change delete to get in utoipa::path annotation (metadata.rs:26)

Consider:

  • Use static str instead of String in GetResponse for better performance
  • Add integration tests for the endpoint
  • Verify the generated OpenAPI spec includes the metadata endpoint correctly after fixing the HTTP method

Overall Assessment

Good refactoring that improves type safety and API documentation. One critical bug needs fixing (wrong HTTP method in OpenAPI doc), but otherwise the implementation follows best practices and repository conventions well.

Status: Needs changes before merge

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 1, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 70bb42d

@MasterPtato MasterPtato marked this pull request as ready for review December 1, 2025 23:31
@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 2, 2025

Merge activity

  • Dec 2, 7:45 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Dec 2, 7:45 AM UTC: CI is running for this pull request on a draft pull request (#3568) due to your merge queue CI optimization settings.
  • Dec 2, 7:46 AM UTC: Merged by the Graphite merge queue via draft PR: #3568.

graphite-app bot pushed a commit that referenced this pull request Dec 2, 2025
@graphite-app graphite-app bot closed this Dec 2, 2025
@graphite-app graphite-app bot deleted the 12-01-chore_api_formalize__metadata branch December 2, 2025 07:46
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