Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 31, 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 7, 2025 4:48am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 7, 2025 4:48am
rivet-inspector Ignored Ignored Preview Nov 7, 2025 4:48am
rivet-site Ignored Ignored Preview Nov 7, 2025 4:48am

Copy link
Member Author

NathanFlurry commented Oct 31, 2025


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 Oct 31, 2025

PR Review: Add Create Actor Script

Summary

This PR adds a useful TypeScript script for creating actors via the Rivet API. The script provides an interactive CLI interface for gathering input parameters and making API calls.

Code Quality & Best Practices

Positive aspects:

  • Clean, straightforward implementation
  • Proper error handling for required fields
  • Consistent with the shebang and structure used in other scripts

Issues identified:

  1. Missing helper functions - The script doesn't follow the established pattern in other scripts like runner-configs/upsert.ts (scripts/api/runner-configs/upsert.ts:10-26), which defines reusable helper functions like ask(), askNumber(), and parseJson(). This leads to:

    • Repetitive code for prompting with defaults
    • Inconsistent prompt formatting
    • No validation for JSON input parsing
  2. Crash policy validation - The script accepts any string for crash_policy, but according to the type definitions (engine/sdks/rust/api-full/rust/src/models/crash_policy.rs:16-23), only three values are valid: restart, sleep, or destroy. The script should validate this input or at least provide these options to the user.

  3. Input JSON parsing - When the user provides input JSON (scripts/api/actors/create.ts:41), there's no validation or error handling. If the JSON is malformed, the API will reject it with a potentially cryptic error.

  4. Inconsistent default behavior - The default for crash_policy in the script is restart, which matches one SDK default, but the API type definition shows destroy as the default (engine/packages/types/src/actors.rs:31-32). This inconsistency could cause confusion.

  5. Missing readline cleanup - If an error occurs before line 42, the readline interface won't be properly closed, which could leave the terminal in an inconsistent state.

Recommendations

High Priority:

  1. Add validation for crash_policy to only accept valid values (restart, sleep, destroy)
  2. Add JSON parsing validation for the input field with proper error messages
  3. Consider extracting common helper functions to a shared module (or at minimum, follow the pattern in upsert.ts)

Medium Priority:
4. Clarify the default crash policy (consider making it destroy to match the API default)
5. Add a try-catch around readline operations to ensure proper cleanup
6. Consider adding validation or better prompts for the name field (should be a valid DNS subdomain per CLAUDE.md naming conventions)

Low Priority:
7. Add a success message after creation (like the ✅ pattern in other scripts)
8. Consider printing the created actor details in a more user-friendly format

Example Fix for Crash Policy Validation

const validCrashPolicies = ["restart", "sleep", "destroy"] as const;
let crashPolicy = (await rl.question("Crash Policy (restart/sleep/destroy, default: restart): ")).trim() || "restart";

if (\!validCrashPolicies.includes(crashPolicy as any)) {
	console.error("Error: Crash Policy must be one of: restart, sleep, destroy");
	rl.close();
	process.exit(1);
}

Security & Performance

  • No security concerns identified
  • Performance is appropriate for a CLI script

Test Coverage

  • Consider adding the script to any integration test suite if one exists for these utility scripts
  • Manual testing should verify all input combinations, especially edge cases with optional fields

Overall Assessment

This is a functional addition that provides useful utility for developers. With the recommended improvements, particularly around input validation and consistency with other scripts, it would be production-ready. The changes are straightforward and low-risk.

@NathanFlurry NathanFlurry force-pushed the 10-31-chore_scripts_add_create_actor_script branch from 553b794 to 6498dbc Compare November 7, 2025 04:46
@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 7, 2025

Merge activity

  • Nov 7, 4:47 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 7, 4:48 AM UTC: CI is running for this pull request on a draft pull request (#3406) due to your merge queue CI optimization settings.
  • Nov 7, 4:49 AM UTC: Merged by the Graphite merge queue via draft PR: #3406.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 7, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 6498dbc

@graphite-app graphite-app bot closed this Nov 7, 2025
@graphite-app graphite-app bot deleted the 10-31-chore_scripts_add_create_actor_script branch November 7, 2025 04:49
@claude
Copy link

claude bot commented Nov 7, 2025

PR Review: Add Create Actor Script

Thanks for adding this utility script! Here is my review:

Code Quality

Positive Observations

  • Follows the executable script pattern with shebang
  • Correctly uses readline/promises for input
  • Proper error handling for HTTP responses
  • Environment variable support for common configuration
  • API structure matches the backend types

Issues and Recommendations

1. Missing JSON Input Validation (High Priority - Potential Bug)
The input field at line 41 accepts arbitrary JSON but does not validate or parse it. If the user enters invalid JSON, it will be sent as a string to the API, which may cause errors. Recommend adding JSON parsing validation similar to scripts/api/runner-configs/upsert.ts:44-52.

2. Crash Policy Validation (Medium Priority)
The script accepts any string for crash_policy, but the backend only accepts restart, sleep, or destroy (see engine/packages/types/src/actors.rs:37-42). Recommend adding validation to prevent API errors.

3. Inconsistent Code Style (Medium Priority)
The script has inconsistent input handling patterns compared to other scripts in the repo. Consider using helper functions like ask() as seen in scripts/api/runner-configs/upsert.ts:10-26 for consistent error messages and better code reusability.

4. Inconsistent Namespace Input (Low Priority)
Lines 19-22 do not call trim() on the user input, unlike other inputs in the same file.

5. Missing Success Message (Low Priority - UX)
Unlike other scripts, this script does not print a friendly success message. Consider adding a success indicator before the JSON output.

Security

No security concerns identified. The script properly uses environment variables for sensitive data.

Performance

No performance concerns for a CLI utility script.

Test Coverage

No tests exist for this script, but that is consistent with other scripts in scripts/api/.

Summary

This is a useful addition! The main issues are:

  1. High Priority: Add JSON input validation to prevent runtime errors
  2. Medium Priority: Add crash policy validation
  3. Low Priority: Improve consistency with existing script patterns

The script is functional as-is, but these improvements would make it more robust and maintainable.

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