refactor(rivetkit): add typed event and queue schemas for actors#4209
Conversation
|
🚅 Deployed to the rivet-pr-4209 environment in rivet-frontend
|
|
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.
How to use the Graphite Merge QueueAdd 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. |
PR Review: refactor(rivetkit): add typed event and queue schemas for actorsThis PR adds typed event and queue schemas for actors using the Standard Schema interface. The overall design is clean and well-structured. Here is my analysis: Bugs1. Unvalidated data fallback in In After passing validation, the validated/transformed output should always be used, not the raw input. The same pattern appears in the Both fallbacks bypass schema transformation for the multi-arg case. Probably unreachable for well-typed usage today, but could silently break with schema libraries that transform/coerce data. Breaking Changes Not Documented2. The PR removes:
This is an API-breaking change not flagged in the PR description or checklist. 3. The database cleanup block was removed entirely with no replacement. Database connections are no longer cleaned up when an actor is destroyed. Intentional? 4. The increment/decrement around action execution was removed. This could affect sleep scheduling if actions were previously preventing premature sleep. Worth confirming Design Concerns5. Multi-queue When calling 6. Potentially unused imports in
7. This change appears unrelated to the PR scope. Intentional? Minor Notes8. Missing behavioral tests Only type-level tests were updated. Consider adding integration tests covering:
9. The old 10. Since What Is Good
|
8a8c7d9 to
64bb333
Compare
| "@rivetkit/sqlite-vfs": "workspace:*", | ||
| "@rivetkit/traces": "workspace:*", | ||
| "@rivetkit/virtual-websocket": "workspace:*", | ||
| "@standard-schema/spec": "^1.0.0", |
There was a problem hiding this comment.
This line adds a new dependency '@standard-schema/spec' but the pnpm-lock.yaml file wasn't updated. Run 'pnpm install' locally to update the lockfile and commit both the package.json and the updated pnpm-lock.yaml file together.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| import { z } from "zod"; | ||
| import { getRunMetadata } from "@/actor/config"; | ||
| import type { ActorDefinition, AnyActorDefinition } from "@/actor/definition"; |
There was a problem hiding this comment.
The imports are not properly sorted. The import for ActorDefinition should come before the getRunMetadata import to maintain alphabetical order.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
64bb333 to
5511321
Compare
| import type { StandardSchemaV1 } from "@standard-schema/spec"; | ||
| import { Unsupported } from "./errors"; |
There was a problem hiding this comment.
Imports should be sorted alphabetically. Move the StandardSchemaV1 import after the Unsupported import.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| import type { OtlpExportTraceServiceRequestJson } from "@rivetkit/traces"; | ||
| import { | ||
| createTraces, | ||
| type SpanHandle, | ||
| type SpanStatusInput, | ||
| type Traces, | ||
| } from "@rivetkit/traces"; | ||
| import type { OtlpExportTraceServiceRequestJson } from "@rivetkit/traces"; | ||
| import invariant from "invariant"; |
There was a problem hiding this comment.
Imports should be sorted alphabetically. The OtlpExportTraceServiceRequestJson import should come after the invariant import, and the ActorTracesDriver import should be moved up with the other imports.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
5511321 to
87740eb
Compare
87740eb to
fb17656
Compare
| const client = await this.#config.db.createClient({ | ||
| getDatabase: () => this.driver.getDatabase(this.#actorId), | ||
| }); | ||
| this.#rLog.info({ msg: "database migration starting" }); | ||
| await this.#config.db.onMigrate?.(client); | ||
| this.#rLog.info({ msg: "database migration complete" }); | ||
| this.#db = client; |
There was a problem hiding this comment.
This code is causing the CI to fail because it's calling this.driver.getDatabase(this.#actorId), but the ActorDriver interface doesn't have a getDatabase method defined. The database provider interfaces need to be updated to include this method, or this code needs to be adjusted to use the existing interface methods.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
fb17656 to
8da1c60
Compare
8da1c60 to
a1f946a
Compare
49bf396 to
35e2bc0
Compare
Merge activity
|
# 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

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: