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.

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

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

PR Review: Add noop converters for v1 -> v3

Summary

This PR adds missing deserialize_converters() and serialize_converters() methods for three protocol enum types (ToRunner, ToGateway, and ToServerlessServer) that only have a V3 variant. The converters are implemented as no-op functions since these types had no changes between v1 and v3.

Code Quality ✅

Strengths:

  • The implementation is consistent with the pattern established for similar enum types in the file
  • The use of Ok as a no-op converter is idiomatic and clean
  • Comments clearly explain the rationale
  • The fix aligns with the trait requirements for OwnedVersionedData

Observations:

  • Each converter returns vec![Ok, Ok] - this creates a vector with two converter functions that simply pass through the value unchanged
  • This matches the versioning model: v1→v2 (first Ok) and v2→v3 (second Ok) for deserialization, reverse for serialization

Potential Issues ⚠️

Minor Concerns:

  1. Return type inference: The impl Fn(Self) -> Result<Self> return type relies on type inference to understand that Ok is being used as a function pointer. While this compiles fine, it might be slightly less explicit than other implementations in the file.

  2. Consistency with enum structure: The three types only have a V3 variant but claim to support versions 1, 2, and 3 in deserialize_version(). This is correct behavior (all versions map to V3), but the no-op converters might not actually be exercised if there is only one enum variant.

  3. Potential confusion: The comment "No changes between v1 and v3" could be interpreted as "the wire format is identical" vs "we don't have separate enum variants". Both are true, but the latter is more relevant here.

Performance Considerations ✅

  • No performance concerns
  • The no-op converters add minimal overhead
  • In practice, these converters likely will not be called since there is only one enum variant

Security Concerns ✅

  • No security issues identified
  • The changes are purely structural trait implementations with no data transformation

Test Coverage ⚠️

Concern:

  • No test files found in the runner-protocol package
  • Without tests, it is unclear if the converters are actually used by the vbare library or if the no-op behavior is correct

Recommendation:
Consider adding integration tests that verify:

  1. Deserializing v1/v2/v3 payloads produces the correct V3 variant
  2. The converter functions are correctly invoked by the versioning system
  3. Round-trip serialization/deserialization works across versions

Verdict: ✅ Approve with Minor Suggestions

This is a solid fix that completes the trait implementation. The code is clean and follows established patterns. The main concern is the lack of test coverage to validate that this implementation is correct and necessary.

Suggested follow-up:

  • Add tests for the versioned protocol types
  • Consider adding a clarifying comment about why converters are needed even with a single enum variant

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

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

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-fix_runner-protocol_add_noop_converters_for_v1_-_v3 branch November 21, 2025 02:43
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