Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 13, 2025 2:49am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 13, 2025 2:49am
rivet-inspector Ignored Ignored Preview Nov 13, 2025 2:49am
rivet-site Ignored Ignored Preview Nov 13, 2025 2:49am

Copy link
Member Author

NathanFlurry commented Nov 10, 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 10, 2025

Code Review: Add Required Zod Validation for JSON Encoding

Summary

This PR adds Zod schema validation to the JSON encoding path in RivetKit's serialization/deserialization flow. The changes ensure type safety when using JSON encoding for client-server communication, while maintaining backward compatibility with CBOR and bare encodings.

Positive Aspects

Strong Type Safety: The new Zod schemas provide runtime validation for JSON-encoded messages, catching malformed data early
Comprehensive Coverage: All protocol message types are covered (ToClient, ToServer, HTTP actions, errors, etc.)
Consistent Pattern: The validation is applied uniformly across all serialization/deserialization call sites
Backward Compatible: CBOR and bare encodings remain unchanged, only JSON gets validation

Issues & Concerns

🔴 Critical: ArrayBuffer Schema Validation Issue

Location: rivetkit-typescript/packages/rivetkit/src/actor/client-protocol-schema-json/mod.ts:4

const ArrayBufferSchema = z.instanceof(ArrayBuffer);

Problem: This schema will fail validation for JSON-encoded data because:

  1. When data is JSON-encoded, ArrayBuffers are converted to base64 strings via jsonStringifyCompat (see src/actor/protocol/serde.ts:154-176)
  2. When JSON-decoded, these base64 strings are converted back to ArrayBuffers via jsonParseCompat (see src/actor/protocol/serde.ts:178-210)
  3. However, Zod validation happens after JSON.parse but before the custom reviver in jsonParseCompat processes the special types

Expected flow:

Serialize: ArrayBuffer -> ["", base64] -> JSON string
Deserialize: JSON string -> parse -> validate with Zod -> revive special types

But validation happens on the raw parsed JSON (where ArrayBuffers are still ["", base64] arrays), not on the final revived objects.

Impact: All JSON-encoded messages with ArrayBuffer fields will fail validation, breaking JSON encoding completely.

Recommended Fix: The schemas should match the intermediate JSON representation:

const ArrayBufferSchema = z.tuple([z.literal(""), z.string()]);
const UintSchema = z.tuple([z.literal(""), z.string()]);

Or alternatively, validate after custom parsing, not before.

⚠️ Major: Type Safety Escape Hatch

Location: rivetkit-typescript/packages/rivetkit/src/remote-manager-driver/api-utils.ts:55-56

requestZodSchema: z.any() as z.ZodType<TInput>,
responseZodSchema: z.any() as z.ZodType<TOutput>,

Problem: Using z.any() defeats the entire purpose of this PR. The apiCall function bypasses all validation.

Impact: API calls won't benefit from the validation layer, creating inconsistent behavior between different code paths.

Recommended Fix: Either:

  1. Require callers to pass explicit schemas
  2. Create a registry of schemas for known API endpoints
  3. At minimum, use z.unknown() instead of z.any() to preserve some type checking

⚠️ Medium: Validation Order Issue

Location: rivetkit-typescript/packages/rivetkit/src/serde.ts:62-64 and 85-94

// Serialize
const validated = zodSchema.parse(value);
return jsonStringifyCompat(validated);

// Deserialize  
parsed = jsonParseCompat(jsonString);
return zodSchema.parse(parsed);

Problem: For deserialization, you're validating the data structure after JSON.parse but before jsonParseCompat applies the custom reviver that converts special types. This means:

  • BigInts are validated as ["", string] arrays, not as bigint
  • ArrayBuffers are validated as ["", string] arrays, not as ArrayBuffer

Current flow:

JSON.parse -> Zod validation -> jsonParseCompat reviver

Expected flow (likely):

JSON.parse + jsonParseCompat reviver -> Zod validation

Impact: Schema definitions don't match the actual runtime types they're meant to validate.

ℹ️ Minor: Type Inconsistency

Location: rivetkit-typescript/packages/rivetkit/src/actor/protocol/serde.ts:25-26

export class CachedSerializer<T, TJson = T> {
	#data: T | TJson;

Observation: The #data field accepts either T or TJson, but there's no runtime tracking of which type it actually contains. This could lead to incorrect serialization if a TJson value is passed with bare encoding (line 73 casts to T unconditionally).

Recommendation: Consider narrowing the type or adding runtime guards.

Testing Recommendations

Given the critical issue with ArrayBuffer validation, I strongly recommend:

  1. Add tests that verify JSON encoding/decoding with:
    • Messages containing ArrayBuffer fields
    • Messages containing BigInt fields
    • Nested structures with these types
  2. Test the validation actually rejects malformed data (wrong types, missing fields, etc.)
  3. Test all three encodings (json, cbor, bare) to ensure no regressions
  4. Add integration tests covering the full serialization pipeline

Performance Considerations

The Zod validation adds overhead to every JSON-encoded message. Consider:

  • Benchmarking the performance impact on high-throughput scenarios
  • Whether validation should be optional in production (with a config flag)
  • Caching parsed schemas if not already done by Zod

Conclusion

While this PR adds valuable type safety, the ArrayBuffer schema validation issue is a blocker that will break JSON encoding. This needs to be fixed before merging. Once the validation logic is corrected to match the actual serialization format, this will be a solid improvement to the codebase.

Recommendation: ❌ Request changes - fix critical validation issue before merge

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 10, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3431

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3431

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3431

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3431

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3431

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3431

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3431

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3431

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3431

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3431

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3431

commit: 1784d8f

@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: Add Required Zod Validation for JSON Encoding

Overview

This PR adds Zod schema validation for JSON encoding in RivetKit's client-server protocol, ensuring type safety and runtime validation when using JSON as the encoding format. The implementation is generally solid but there are a few concerns to address.


Strengths

  1. Type Safety Enhancement: Adding Zod validation for JSON encoding significantly improves runtime type safety and catches serialization/deserialization errors early.

  2. Comprehensive Schema Coverage: The new client-protocol-schema-json/mod.ts file covers all protocol messages with proper use of discriminated unions for tagged message types.

  3. Consistent Application: The schemas are consistently applied across all serialization/deserialization call sites (WebSocket connections, server-side message processing, HTTP action handling, event manager).

  4. Backward Compatibility: The changes maintain backward compatibility by only validating JSON encoding (not CBOR or bare) and keeping existing VersionedDataHandler for non-JSON formats.

  5. Clean Architecture: The generic type parameters allow for flexible typing where JSON schemas can differ from internal representations.


Issues and Concerns

1. Security: Use of z.any() in API Utils (Priority: High)

Location: src/remote-manager-driver/api-utils.ts:55-56

Using z.any() completely bypasses validation, defeating the purpose of this PR. This creates a security and data integrity risk.

Recommendation: Replace with z.unknown() at minimum, or better yet, define proper Zod schemas for the API endpoints that use this utility.

2. ArrayBuffer Schema Validation (Priority: Medium)

Location: src/actor/client-protocol-schema-json/mod.ts:4

z.instanceof(ArrayBuffer) may not work correctly across different execution contexts or after JSON serialization where ArrayBuffers are converted to base64 strings. Consider using a custom Zod schema that validates the array format that jsonStringifyCompat produces.

3. Missing Test Coverage (Priority: Medium)

No new tests were added for the Zod validation logic. Tests should verify that validation catches malformed JSON messages, invalid discriminated union tags, missing required fields, and wrong types for bigint/ArrayBuffer.

4. Error Handling (Priority: Low)

Using .parse() throws errors synchronously. Consider using .safeParse() and handling validation errors more gracefully with better context about what was being validated.

5. Type Complexity (Priority: Low)

The dual generic types add complexity. Add JSDoc comments explaining when T !== TJson and why, documenting that TJson represents the JSON-serializable form while T represents the internal representation.


Additional Observations

  • Positive: The use of discriminated unions is excellent for type narrowing and validation
  • Positive: Proper handling of nullable types with OptionalArrayBufferSchema and OptionalUintSchema
  • Positive: The schemas align well with the existing jsonStringifyCompat/jsonParseCompat logic

Checklist for Author

  • Replace z.any() usage in api-utils.ts with proper schemas or at minimum z.unknown()
  • Add validation tests for the new Zod schemas
  • Consider improving ArrayBuffer schema validation to handle JSON representation
  • Use .safeParse() instead of .parse() for better error handling
  • Add JSDoc comments explaining the T vs TJson generic types

Verdict

Approve with changes requested. The core implementation is solid and adds valuable type safety. However, the use of z.any() in API utils undermines the security benefits and should be addressed before merging. The missing tests are also concerning for such a critical change to the serialization layer.

Once the z.any() issue is resolved and basic tests are added, this will be a strong improvement to the codebase.

@NathanFlurry NathanFlurry force-pushed the 11-09-chore_rivetkit_add_required_zod_validation_for_json_encoding branch from d27a9f0 to be3dc7d Compare November 13, 2025 02:47
@claude
Copy link

claude bot commented Nov 13, 2025

Code Review - PR #3431

This PR adds Zod schema validation for JSON-encoded protocol messages in RivetKit.

Positive Aspects

  1. Security & Data Integrity: Adding Zod validation provides runtime type checking for JSON encoding
  2. Good Type Safety: Proper use of TypeScript generics (TJson parameter)
  3. Comprehensive Coverage: All protocol message types covered with discriminated unions
  4. Backward Compatible: Only affects JSON encoding

Critical Issues

  1. Ineffective Validation for CBOR/Bare - Validation only applies to JSON encoding (src/serde.ts:56-77). CBOR and bare encodings bypass validation entirely.

  2. z.any() in api-utils.ts:55-56 - Using z.any() defeats the purpose of validation for remote manager API calls.

  3. Unsafe Type Cast at src/serde.ts:73 - The 'as T' cast is unsafe when value might be TJson type.

  4. Missing Test Coverage - No tests found for the new validation logic.

  5. Performance Concern - CachedSerializer validates on every serialize() call, not cached.

  6. Schema Mismatch Risk - Zod schemas manually maintained separately from protocol definitions.

Recommendations Before Merge

Critical:

  • Address z.any() usage or add explanatory comment
  • Add comprehensive test coverage
  • Document JSON-only validation behavior

Important:

  • Consider validating all encodings or document why excluded
  • Add schema maintenance documentation

See full details in src/serde.ts, src/actor/protocol/serde.ts, and src/remote-manager-driver/api-utils.ts

@NathanFlurry NathanFlurry force-pushed the 11-09-chore_rivetkit_add_listing_actors_by_only_name_to_manager_api branch from 51507e2 to a8f3a9c Compare November 13, 2025 20:53
@NathanFlurry NathanFlurry force-pushed the 11-09-chore_rivetkit_add_required_zod_validation_for_json_encoding branch from be3dc7d to 1784d8f Compare November 13, 2025 20:53
@vercel
Copy link

vercel bot commented Nov 13, 2025

Deployment failed with the following error:

Too many requests - try again in 1 minute (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 13, 2025

Code Review - PR 3431: Add Required Zod Validation for JSON Encoding

Summary

This PR adds Zod schema validation for JSON encoding/decoding in the RivetKit TypeScript client protocol. The changes introduce runtime type validation when using JSON encoding, while leaving CBOR and bare encodings unvalidated.


Strengths

  1. Type Safety Improvement: Adding Zod validation for JSON encoding provides runtime type checking that can catch serialization/deserialization errors early.

  2. Comprehensive Schema Coverage: The new client-protocol-schema-json/mod.ts file provides thorough schema definitions for all protocol messages.

  3. Backward Compatible Design: The validation is only applied to JSON encoding, leaving existing CBOR and bare encoding paths unchanged.

  4. Consistent Application: The schemas are correctly threaded through all the serialization points.


Issues and Concerns

1. Performance Impact Not Assessed (Severity: Medium)

The PR adds Zod validation to every JSON serialization/deserialization call, which introduces runtime overhead. For high-frequency message passing:

  • Location: serde.ts:63 and serde.ts:94
  • Issue: zodSchema.parse(value) runs on every JSON encode/decode operation
  • Impact: This could significantly impact performance for actor-to-actor communication

Recommendation: Add benchmarks, consider development-only validation mode, and document performance implications.

2. Inconsistent Validation Across Encodings (Severity: Medium)

Only JSON encoding gets validation, while CBOR and bare encodings remain unvalidated. Either document why JSON needs validation but CBOR doesn't, or add validation for all encodings.

3. z.any() Defeats the Purpose (Severity: High)

In remote-manager-driver/api-utils.ts:55-56, using z.any() completely bypasses validation. Consider defining proper schemas or making the schema parameter optional.

4. ArrayBuffer Validation May Fail (Severity: High)

The schema uses z.instanceof(ArrayBuffer) but validation may run before jsonParseCompat converts base64 strings back to ArrayBuffers. Need integration tests to verify the order of operations in the deserialization pipeline.

5. Missing Test Coverage (Severity: Medium)

No tests verify validation errors, malformed data rejection, or integration with the full pipeline.


Testing Recommendations

Before merging:

  1. Add tests for validation errors
  2. Add integration tests for the full pipeline with ArrayBuffers/BigInts
  3. Performance benchmark comparing JSON encoding before/after
  4. Verify all three encoding types work correctly

Summary Recommendation

Request Changes - The core idea is sound, but needs:

  1. Resolution of the ArrayBuffer validation timing issue
  2. Either remove z.any() or document why it is acceptable
  3. Add test coverage for validation errors
  4. Document or benchmark performance impact

The PR improves type safety which is valuable, but the issues above need addressing to ensure correctness and avoid performance regressions.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 13, 2025

Merge activity

  • Nov 13, 9:15 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 13, 9:16 PM UTC: CI is running for this pull request on a draft pull request (#3465) due to your merge queue CI optimization settings.
  • Nov 13, 9:16 PM UTC: Merged by the Graphite merge queue via draft PR: #3465.

@graphite-app graphite-app bot closed this Nov 13, 2025
@graphite-app graphite-app bot deleted the 11-09-chore_rivetkit_add_required_zod_validation_for_json_encoding branch November 13, 2025 21:16
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.

3 participants