chore(core): update api schema to match new engine schema #1292

NathanFlurry · 2025-09-24T01:43:09Z

No description provided.

vercel · 2025-09-24T01:43:12Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Sep 24, 2025 3:14am

claude · 2025-09-24T01:43:23Z

Claude finished @NathanFlurry's task —— View job

Code Review for API Schema Update

This PR consolidates the manager API schema and refactors routing logic. Overall, the changes look good and represent a positive consolidation, but there are several issues that need attention.

Todo List:

Read and analyze all changed files
Review API schema consolidation changes
Check router.ts refactoring for issues
Analyze remote manager driver updates
Evaluate security and performance implications
Provide comprehensive feedback

🔍 Findings

✅ Positive Changes

Good Consolidation: Moving scattered route files into a single actors.ts schema file improves maintainability
Cleaner Imports: Reduced import complexity in router.ts
Consistent Schema Structure: All actor-related schemas are now co-located
Proper Type Safety: Good use of Zod schemas with TypeScript inference

⚠️ Issues Found

1. API Inconsistency in GET /actors (High Priority)

packages/rivetkit/src/manager/router.ts:183-189

const actorOutput = await managerDriver.getWithKey({
    c,
    name,
    key: [key], // Convert string to ActorKey array
});

The API expects query params name and key but only returns a single actor. However, the response schema ActorsListResponse suggests it should return multiple actors. The endpoint should either:

Accept multiple actor IDs/keys and return multiple actors, OR
Use a singular response schema

2. Logic Error in PUT /actors (High Priority)

packages/rivetkit/src/manager/router.ts:220-231

const existingActor = await managerDriver.getWithKey({...});

if (existingActor) {
    return c.json<ActorsGetOrCreateResponse>({
        actor: createApiActor(existingActor),
        created: false,
    });
}

// Create new actor - This code will never run!
const newActor = await managerDriver.getOrCreateWithKey({...});

Bug: The existing actor check makes the "create" part unreachable. If an actor exists, it returns early, but if it doesn't exist, the code should call getOrCreateWithKey instead of getWithKey. This is a semantic error.

3. Missing Validation in API Endpoints

packages/rivetkit/src/remote-manager-driver/api-endpoints.ts:22

`/actors?actor_ids=${encodeURIComponent(actorId)}`

The query parameter is named actor_ids (plural) but the router expects name and key. This mismatch will cause API calls to fail.

4. Hard-coded Values in createApiActor (Medium Priority)

packages/rivetkit/src/manager/router.ts:716-717

namespace_id: "default", // Assert default namespace
runner_name_selector: "rivetkit", // Assert rivetkit runner

These hard-coded values should come from the actual actor data or configuration, not be asserted.

5. Deprecated TODO Comment

packages/rivetkit/src/manager/router.ts:290-312
There's a large commented-out DELETE endpoint that should either be implemented or removed entirely.

6. Missing Error Handling

The CBOR decoding in several places lacks error handling:

cbor.decode(Buffer.from(body.input, "base64")) // Could throw

🔒 Security Assessment

Base64 Input Handling: CBOR decoding from base64 input appears safe but should have size limits
No SQL Injection: All inputs are properly parameterized
Header Validation: WebSocket proxy headers are properly validated

⚡ Performance Considerations

Reduced Bundle Size: Consolidating route files slightly improves bundle size
No Major Performance Issues: The refactoring doesn't introduce performance regressions

🧪 Test Coverage

Missing: No visible test updates for the API schema changes
Recommendation: Add integration tests for the new consolidated API endpoints

📋 Recommendations

Fix the PUT /actors logic - The most critical issue
Reconcile API parameter mismatches between client and server
Add proper error handling for CBOR operations
Replace hard-coded values with dynamic configuration
Add integration tests for the consolidated API
Remove or implement the commented DELETE endpoint

💡 Suggestions for Future

Consider adding OpenAPI request/response validation middleware to catch schema mismatches early.

NathanFlurry · 2025-09-24T01:43:30Z

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 either label to this PR to merge it via the merge queue:

merge queue - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

pkg-pr-new · 2025-09-24T01:44:58Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: ecc0c9d

claude · 2025-09-24T03:13:18Z

Claude finished @NathanFlurry's task —— View job

Code Review for API Schema Update

This PR consolidates the manager API schema and refactors routing logic. I've completed a thorough review focusing on code quality, potential bugs, performance, security, and test coverage.

Todo List:

Read and analyze all changed files
Review API schema consolidation changes
Check router.ts refactoring for issues
Analyze remote manager driver updates
Evaluate security and performance implications
Provide comprehensive feedback

🔍 Analysis Results

✅ Positive Changes

Excellent Consolidation: Moving 5 scattered route schema files into a single actors.ts file greatly improves maintainability
Cleaner Architecture: Reduced import complexity and better separation of concerns
Consistent Type Safety: Proper use of Zod schemas with TypeScript inference
Improved API Structure: Co-located schemas make the API easier to understand and modify

⚠️ Issues Found

1. Hard-coded Values in createApiActor (Medium Priority)

packages/rivetkit/src/manager/router.ts:758-759

namespace_id: "default", // Assert default namespace
runner_name_selector: "rivetkit", // Assert rivetkit runner

Issue: These values should be derived from the actual actor data or configuration rather than hard-coded assertions.

2. Missing Error Handling for CBOR Decoding (Medium Priority)

packages/rivetkit/src/manager/router.ts:281 and 320

cbor.decode(Buffer.from(body.input, "base64")) // Could throw if malformed

Issue: While global error handling exists via handleRouteError, CBOR decoding could fail with malformed base64 or invalid CBOR data. Consider wrapping in try-catch for better error messages.

3. Inconsistent Query Parameter Handling (Low Priority)

The GET /actors endpoint accepts both actor_ids (comma-separated) and key parameters, but the logic could be clearer about their mutual exclusivity.

4. Potential Performance Issue with Actor ID Iteration (Low Priority)

packages/rivetkit/src/manager/router.ts:211-222
Sequential database calls for multiple actor IDs could be optimized with batch operations.

🔒 Security Assessment

Base64/CBOR Handling: Generally safe, but should validate input size limits
Query Parameter Validation: Proper use of Zod validation prevents injection
Actor ID Limits: Good 32-actor limit prevents abuse
No Sensitive Data Exposure: Schema doesn't leak internal details

⚡ Performance Considerations

Bundle Size: Consolidation provides marginal bundle size improvement
API Efficiency: No performance regressions introduced
Database Queries: Sequential actor fetching could be optimized but acceptable for current limits

🧪 Test Coverage

Missing: No visible test updates for the consolidated API endpoints
Risk: Schema consolidation changes may not be covered by existing tests
Recommendation: Add integration tests for the new consolidated endpoints

📋 Recommendations

High Priority:

Replace hard-coded values in createApiActor with dynamic data from ActorOutput

Medium Priority:
2. Add CBOR decode error handling with descriptive error messages
3. Add integration tests for the consolidated API endpoints

Low Priority:
4. Consider batch operations for multiple actor ID fetches
5. Add input size validation for base64 CBOR data

✨ Overall Assessment

This is a well-executed refactoring that significantly improves code organization. The consolidation from 5 files to 1 schema file is a clear improvement. The identified issues are relatively minor and don't affect the core functionality.

Previous Claude Review Note: I notice there was a previous review that incorrectly identified the PUT /actors logic as buggy. Upon careful analysis, the current implementation is actually correct - it properly implements get-or-create semantics by first checking existence, then creating if needed.

Verdict: ✅ Ready to merge after addressing the hard-coded values issue.

graphite-app · 2025-09-24T06:06:52Z

Merge activity

Sep 24, 6:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Sep 24, 6:07 AM UTC: CI is running for this pull request on a draft pull request (#1294) due to your merge queue CI optimization settings.
Sep 24, 6:08 AM UTC: Merged by the Graphite merge queue via draft PR: #1294.

NathanFlurry mentioned this pull request Sep 24, 2025

fix(core): fix inline client driver for cloudflare-workers #1289

Closed

NathanFlurry mentioned this pull request Sep 24, 2025

chore(core): fix conn/disconn tests for sse #1290

Closed

vercel bot had a problem deploying to Preview September 24, 2025 01:44 Failure

chore(core): update api schema to match new engine schema

ecc0c9d

NathanFlurry force-pushed the 09-23-chore_core_update_api_schema_to_match_new_engine_schema branch from c611394 to ecc0c9d Compare September 24, 2025 03:13

vercel bot had a problem deploying to Preview September 24, 2025 03:14 Failure

NathanFlurry mentioned this pull request Sep 24, 2025

chore(core): migrate gateway to use websocket protocols #1293

Closed

graphite-app bot pushed a commit that referenced this pull request Sep 24, 2025

chore(core): update api schema to match new engine schema (#1292)

e78e331

graphite-app bot closed this Sep 24, 2025

graphite-app bot deleted the 09-23-chore_core_update_api_schema_to_match_new_engine_schema branch September 24, 2025 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(core): update api schema to match new engine schema #1292

chore(core): update api schema to match new engine schema #1292

Uh oh!

NathanFlurry commented Sep 24, 2025

Uh oh!

vercel bot commented Sep 24, 2025 •

edited

Loading

Uh oh!

claude bot commented Sep 24, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Sep 24, 2025 •

edited

Loading

Uh oh!

pkg-pr-new bot commented Sep 24, 2025 •

edited

Loading

Uh oh!

claude bot commented Sep 24, 2025 •

edited

Loading

Uh oh!

graphite-app bot commented Sep 24, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chore(core): update api schema to match new engine schema #1292

chore(core): update api schema to match new engine schema #1292

Uh oh!

Conversation

NathanFlurry commented Sep 24, 2025

Uh oh!

vercel bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for API Schema Update

Todo List:

🔍 Findings

✅ Positive Changes

⚠️ Issues Found

1. API Inconsistency in GET /actors (High Priority)

2. Logic Error in PUT /actors (High Priority)

3. Missing Validation in API Endpoints

4. Hard-coded Values in createApiActor (Medium Priority)

5. Deprecated TODO Comment

6. Missing Error Handling

🔒 Security Assessment

⚡ Performance Considerations

🧪 Test Coverage

📋 Recommendations

💡 Suggestions for Future

Uh oh!

NathanFlurry commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

pkg-pr-new bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for API Schema Update

Todo List:

🔍 Analysis Results

✅ Positive Changes

⚠️ Issues Found

1. Hard-coded Values in createApiActor (Medium Priority)

2. Missing Error Handling for CBOR Decoding (Medium Priority)

3. Inconsistent Query Parameter Handling (Low Priority)

4. Potential Performance Issue with Actor ID Iteration (Low Priority)

🔒 Security Assessment

⚡ Performance Considerations

🧪 Test Coverage

📋 Recommendations

✨ Overall Assessment

Uh oh!

graphite-app bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel bot commented Sep 24, 2025 •

edited

Loading

claude bot commented Sep 24, 2025 •

edited

Loading

NathanFlurry commented Sep 24, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 24, 2025 •

edited

Loading

claude bot commented Sep 24, 2025 •

edited

Loading

graphite-app bot commented Sep 24, 2025 •

edited

Loading