Skip to content

chroe: rename rivetkit-sqlite to depot-client#4883

Merged
NathanFlurry merged 1 commit intomainfrom
05-02-chroe_rename_rivetkit-sqlite_to_depot-client
May 2, 2026
Merged

chroe: rename rivetkit-sqlite to depot-client#4883
NathanFlurry merged 1 commit intomainfrom
05-02-chroe_rename_rivetkit-sqlite_to_depot-client

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 2, 2026

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

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 2, 2026

Code Review: chore rename rivetkit-sqlite to depot-client

Overview:
This PR renames the rivetkit-sqlite / rivetkit-sqlite-types crates to depot-client / depot-client-types and relocates them from rivetkit-rust/packages/ to engine/packages/. The change is largely mechanical — Cargo manifests, workspace references, import paths, the wasm-gate script, and internal docs all updated consistently.


Issues

1. Test deletion without replacement (high concern)

rivetkit-rust/packages/rivetkit-sqlite/tests/remote_execution_parity.rs is deleted outright (261 lines, 3 test cases) and is not present anywhere in the new engine/packages/depot-client/tests/ tree:

  • remote_migration_order_persists_across_reopen
  • remote_execute_write_forces_writer_for_readonly_sql
  • remote_manual_transactions_stay_on_writer_until_commit_or_rollback

These cover important correctness invariants for migration persistence, writer enforcement for read-only SQL, and manual transaction routing. They used open_database_from_engine, which appears to have been renamed to open_database_from_envoy, but the tests themselves should be updated and carried over, not dropped. If they are deliberately removed because they are now covered elsewhere, that should be called out explicitly in the PR description.

2. Undocumented API change mixed into a rename PR

Every VFS registration and context-build call in the test files gains a new None argument in multiple sites (vfs.rs, scenario.rs, etc.). Based on the CLAUDE.md update (CompactionSignaler) this appears to be an optional compaction signaler wired into the VFS layer. That is a behavioural change beyond a rename and should be documented in the PR description, with the new parameter type and purpose made explicit. Passing None everywhere in tests also means the new code path is never exercised under test.


Minor

  • Commit message typo: Title reads "chroe:" instead of "chore:".
  • AGENTS.md symlink scope: A symlink was correctly added at engine/packages/depot-client/tests/inline/fault/AGENTS.md. If a top-level engine/packages/depot-client/CLAUDE.md exists, it also needs its own AGENTS.md sibling per the CLAUDE.md convention.
  • PR description is template boilerplate with unchecked checklist. It would help reviewers to know: (a) why the package moves to engine/packages/, (b) what the new None parameter represents, (c) whether the deleted tests are intentionally dropped or need to be ported.

What looks good

  • All workspace dependency references, feature flags (sqlite-local, wasm ban list), and internal doc links updated consistently with no dangling rivetkit-sqlite references left behind.
  • docs-internal/engine/sqlite-vfs.md correctly collapses the now-irrelevant native vs WASM parity section.
  • Formatting fixes (split concatenated lines in VFS tests) are clean.
  • depot/CLAUDE.md test-location bullet updated correctly.

Summary

The mechanical rename is clean and complete. Two items to resolve before merging: (1) the deleted remote execution parity tests need to be ported to engine/packages/depot-client/tests/ with the updated open_database_from_envoy API, and (2) the new optional CompactionSignaler parameter should be documented and at least one test site should exercise it with a real signaler.

@NathanFlurry NathanFlurry force-pushed the 05-02-fix_rivetkit-wasm_fix_mem_leaks branch from f914045 to 8bac2e2 Compare May 2, 2026 22:44
@NathanFlurry NathanFlurry force-pushed the 05-02-chroe_rename_rivetkit-sqlite_to_depot-client branch from e678e81 to 133b3e1 Compare May 2, 2026 22:44
@railway-app railway-app Bot temporarily deployed to rivet-frontend / production May 2, 2026 22:44 Inactive
Base automatically changed from 05-02-fix_rivetkit-wasm_fix_mem_leaks to main May 2, 2026 22:44
@NathanFlurry NathanFlurry merged commit 133b3e1 into main May 2, 2026
7 of 17 checks passed
@NathanFlurry NathanFlurry deleted the 05-02-chroe_rename_rivetkit-sqlite_to_depot-client branch May 2, 2026 22:44
This was referenced May 4, 2026
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