Skip to content

feat(vbare): add conversion boilerplate generator, fix envoy protocol conversion#5000

Open
MasterPtato wants to merge 1 commit into05-07-chore_clean_up_typesfrom
05-07-feat_vbare_add_conversion_boilerplate_generator_fix_envoy_protocol_conversion
Open

feat(vbare): add conversion boilerplate generator, fix envoy protocol conversion#5000
MasterPtato wants to merge 1 commit into05-07-chore_clean_up_typesfrom
05-07-feat_vbare_add_conversion_boilerplate_generator_fix_envoy_protocol_conversion

Conversation

@MasterPtato
Copy link
Copy Markdown
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 8, 2026

🚅 Deployed to the rivet-pr-5000 environment in rivet-frontend

Service Status Web Updated (UTC)
website ✅ Success (View Logs) Web May 9, 2026 at 5:00 am
frontend-cloud ❌ Build Failed (View Logs) Web May 8, 2026 at 12:30 am
ladle ❌ Build Failed (View Logs) Web May 8, 2026 at 12:30 am
kitchen-sink ❌ Build Failed (View Logs) Web May 8, 2026 at 12:30 am
frontend-inspector ❌ Build Failed (View Logs) Web May 8, 2026 at 12:28 am
mcp-hub ✅ Success (View Logs) Web May 8, 2026 at 12:27 am

Copy link
Copy Markdown
Contributor Author

MasterPtato commented May 8, 2026

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

This stack of pull requests is managed by Graphite. Learn more about stacking.

@MasterPtato MasterPtato mentioned this pull request May 8, 2026
11 tasks
@MasterPtato MasterPtato force-pushed the 05-07-feat_vbare_add_conversion_boilerplate_generator_fix_envoy_protocol_conversion branch from 05224ba to 999dbf2 Compare May 8, 2026 00:29
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 8, 2026

PR #5000 Review: feat(vbare): add conversion boilerplate generator, fix envoy protocol conversion

Overview

This PR has two parts:

  1. New code generator (scripts/vbare-gen-converters/index.ts) - a TypeScript CLI that parses two adjacent BARE schema files and emits bidirectional Rust converter boilerplate. Fields and variants that are structurally identical on both sides get auto-filled; anything that diverges gets a todo!() so the implementer is forced to make an explicit decision.
  2. envoy-protocol versioned conversion refactor - the old monolithic versioned.rs (1846 lines, hand-written macro-based) and versioned_conversions.in (532 lines) are replaced by versioned/mod.rs (wrapper enums + OwnedVersionedData impls) and individual vN_to_vM.rs / vM_to_vN.rs files generated by the new tool.

This is a meaningful maintainability improvement. Adding a v6 schema in the future will be a matter of running one command and filling in the deltas, rather than hand-authoring 300+ lines of conversion machinery.


Concerns

1. todo!() in SQLite downgrade paths should be unreachable!() (high concern)

v2_to_v1.rs and v3_to_v2.rs contain todo!() in per-type SQLite functions like convert_sqlite_get_pages_request_v3_to_v2 and convert_sqlite_commit_request_v3_to_v2. The top-level union converters do correctly guard these paths by bailing early before calling the per-type functions. So these functions are unreachable in practice. But they are declared pub, carry no marker, and todo!() panics at runtime. The migration guide says these dead todo!()s "can stay" - that is pragmatic - but they should be replaced with unreachable!("guarded by top-level bail in convert_to_rivet_v3_to_v2") to communicate intent and produce a clearer backtrace if the invariant ever breaks.

2. SQLite page-IO compat errors are plain bail! strings, not ProtocolCompatibilityError (medium concern)

In the old code, all feature-gated downgrade errors went through incompatible(), giving callers a typed ProtocolCompatibilityError they could downcast. The new convert_to_rivet_v3_to_v2 and convert_to_envoy_v3_to_v2 use raw bail!("stateless sqlite requests require envoy-protocol v3") instead of incompatible(ProtocolCompatibilityFeature::SqlitePageIo, ...). This means SqlitePageIo and SqlitePageRange compatibility errors are no longer distinguishable via err.downcast::(). The incompatible() helper exists precisely for this pattern and should be used here.

3. @generated marker left on completed files

The migration guide says to drop the @generated marker after filling in every todo!(). Both v4_to_v5.rs and v5_to_v4.rs appear fully implemented (no todo!() remaining) but still carry the // @generated initial scaffold comment. If the convention is that this marker signals "not yet reviewed by a human", these need cleanup.

4. Test change for target-version is correct but the comment could be more specific

The fix changing assert_compatibility_error(err, ..., version) to assert_compatibility_error(err, ..., 3) is correct: remote SQL drops at the v4->v3 boundary, so target_version is always 3 regardless of how far down the chain was requested. The added comment is helpful. Minor suggestion: name the source file (v4_to_v3.rs) in the comment so the error origin is traceable without grepping.


Smaller issues

Generator: fragile ?-stripping in convertExpr - The code strips a trailing ? from fallible inner expressions before wrapping them in .map(). This works for current output (converter calls always look like fn_name(v)?) but would produce incorrect Rust for any future case where the expression is more complex. Tracking fallibility separately and wrapping at call sites would be safer.

Generator: order.includes(name) is O(n^2) in emitFile - The deduplication loop uses Array.includes. A Set for seen names would be O(n). Not a problem for current schema sizes but trivially fixable.

Generator: renderStruct gives no context on type-changed fields - When a field exists on both sides but its type changed, the emitted todo!() has no annotation explaining what changed. A comment like "// was: v3::Foo, now: v4::Bar" would help the implementer understand why the scaffold did not auto-fill it.


What is done well

  • Per-file structure (vN_to_vM.rs) is dramatically easier to read and review than the old macro-based approach. Reviewing a single migration step is now one file instead of grepping through 500-line macros.
  • The generator correctly handles transitive type reachability, trivial alias filtering, bidirectional generation, and Rust keyword escaping - all non-trivial edge cases.
  • incompatible() visibility change to pub(crate) is correct - it is now needed across module boundaries.
  • The OwnedVersionedData impl structure (enum variant per version + step functions) is clean and naturally enforces that each step applies only to the right source variant.
  • .claude/reference/vbare-migrations.md is a clear, actionable procedural guide for the next person adding a schema version.

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.

1 participant