You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 is a clean deletion PR removing the entire KV channel subsystem (~2,800 lines). The changes are well-scoped and consistent. A few observations:
Summary Assessment
This is a straightforward removal of a dedicated WebSocket transport (/kv/connect) used by @rivetkit/sqlite-native for VFS KV operations. The deletion is thorough and covers all layers: schema, Rust protocol crate, TypeScript SDK, guard routing, and the runtime router.
Positive Notes
All related artifacts are removed together (schema, generated types, Rust crate, TypeScript package, workspace entries, routing) with no dangling references left behind.
CLAUDE.md is updated to remove the now-irrelevant sync instructions, which keeps the guidance accurate.
The JSDoc comment added to hardCrashActor marking it as test-only is a useful clarification.
Questions / Concerns
What replaces the KV channel?
The PR description doesn't explain what @rivetkit/sqlite-native will use instead of /kv/connect for its VFS KV operations. If the native SQLite VFS is still in use, there should be a follow-up PR (or existing code) routing KV operations through the runner protocol or another channel. If sqlite-native is also being removed, that should be noted. Worth clarifying in the PR description.
Leftover references to @rivetkit/engine-kv-channel-protocol?
The package is removed from rivetkit-typescript/packages/rivetkit/package.json and the pnpm workspace, but it's worth confirming no other packages in the monorepo still depend on @rivetkit/engine-kv-channel-protocol (e.g., other examples or internal packages).
actor-gateway/gateway.ts removal of the /kv/connect skip:
The removed early-return guard that bypassed the gateway for /kv/connect paths is safe to delete since the route no longer exists. Just confirming there's no scenario where external traffic could still hit that path expecting the old behavior.
Minor
The PR is currently in DRAFT state. Once the replacement transport (or confirmation that sqlite-native is also dropped) is clarified, it looks ready for final review.
Overall this is a clean removal with no obvious issues. The main open question is what replaces the KV transport for native SQLite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: