Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

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

Copy link
Member Author

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

PR Review: Add db nuke subcommand

Overview

This PR adds a new db nuke subcommand to the engine CLI for deleting database contents, with initial support for FileSystem databases. Overall, this is a useful development tool, but there are several issues that should be addressed.


Critical Issues

1. Security: Hardcoded Path in Shell Script ⚠️

File: scripts/run/nuke-rocksdb.sh:9

rm -rf "~/Library/Application Support/rivet-engine/"

Issues:

  • The tilde (~) won't expand inside double quotes - this command will literally try to delete a directory named ~ instead of the user's home directory
  • The path is hardcoded to macOS-specific location (Library/Application Support)
  • This script won't work on Linux or Windows
  • The script doesn't use the actual database path from the config

Recommendation:
Either remove this script entirely (since the db nuke command handles this properly) or fix it to:

rm -rf ~/Library/Application\ Support/rivet-engine/
# Or better, call the actual command:
cargo run --bin rivet-engine -- db nuke --yes

Code Quality Issues

2. Logging: Use Structured Logging Instead of println!

File: engine/packages/engine/src/commands/db/mod.rs:71-73

According to CLAUDE.md logging patterns, the codebase uses tracing for structured logging. The current implementation uses println\!:

println\!("Removing {}", file_system.path.display());
tokio::fs::remove_dir_all(&file_system.path).await?;
println\!("Complete");

Recommendation:

tracing::info\!(path = %file_system.path.display(), "removing database directory");
tokio::fs::remove_dir_all(&file_system.path).await?;
tracing::info\!("database nuked successfully");

This provides:

  • Consistent logging across the codebase
  • Structured data for observability
  • Proper log levels following the pattern seen in other engine commands

3. Error Handling: Match is Not Exhaustive

File: engine/packages/engine/src/commands/db/mod.rs:66-76

The match statement on config.database() only handles two database types. While this appears to currently cover all variants, the lack of a catch-all arm means:

  • If new database types are added in the future, this will silently break
  • The compiler won't warn about missing variants

Current:

match config.database() {
    rivet_config::config::Database::Postgres(_) => {
        bail\!("nuke postgres not implemented");
    }
    rivet_config::config::Database::FileSystem(file_system) => {
        // ... implementation
    }
}

Recommendation:
Verify that the Database enum is properly marked with #[non_exhaustive] or add a catch-all arm if new variants could be added.


Style & Convention Issues

4. Message Capitalization

Per CLAUDE.md: "Log messages should be lowercase unless mentioning specific code symbols."

  • Line 63: "Please pass '--yes' flag..." - should start with lowercase
  • The error message should be: "please pass '--yes' flag to confirm you want to nuke the database"

Positive Aspects ✅

  1. Good safety mechanism: Requiring --yes flag prevents accidental data deletion
  2. Clear error message: The Postgres unimplemented error is clear and helpful
  3. Consistent with repo patterns: The command structure follows existing CLI patterns
  4. Simple and focused: The implementation is straightforward and does one thing well

Additional Considerations

5. Missing Warning/Confirmation Output

For a destructive operation, consider adding a more visible warning:

tracing::warn\!("⚠️  this will permanently delete all data in the database");
tracing::info\!(path = %file_system.path.display(), "removing database directory");

6. Error Context

When remove_dir_all fails, the error context could be more helpful:

tokio::fs::remove_dir_all(&file_system.path)
    .await
    .with_context(|| format\!("failed to remove database directory: {}", file_system.path.display()))?;

Test Coverage

No tests were added for this functionality. Consider adding:

  • Unit test for the --yes flag validation
  • Integration test that creates a test database and nukes it

Summary

Must Fix:

  • 🔴 Fix the shell script path expansion issue or remove the script
  • 🟡 Replace println\! with tracing macros
  • 🟡 Lowercase error messages

Nice to Have:

  • Add error context for filesystem operations
  • Add warning logs for destructive operations
  • Add test coverage

Overall, this is a useful feature for development workflows, but needs some refinement to match the codebase standards and fix the shell script bug.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 3, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 620ef31

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: Add db nuke subcommand

Thank you for adding this useful development utility! Here's my review:

✅ Positive Aspects

  1. Good safety mechanism: The --yes flag requirement prevents accidental database deletion
  2. Clear user feedback: The command prints the path being removed and completion status
  3. Proper error handling: Uses bail! for unimplemented Postgres support and other error cases
  4. Follows repository conventions:
    • Uses anyhow::* for error handling
    • Lowercase log messages (println! in this case)
    • Proper use of tokio::fs for async operations

🐛 Critical Bug

scripts/run/nuke-rocksdb.sh:9 - The tilde (~) is quoted, so it won't expand to the home directory. This script will literally try to delete a directory named ~/Library/Application Support/rivet-engine/ in the current directory, which will fail.

Fix:

rm -rf ~/Library/Application\ Support/rivet-engine/
# OR
rm -rf "$HOME/Library/Application Support/rivet-engine/"

🤔 Issues & Suggestions

  1. Hardcoded macOS path: The script only works on macOS and hardcodes the path. Consider:

    • Making it cross-platform (checking uname for Linux/macOS)
    • Or better yet, using the Rust command: cargo run --bin rivet-engine -- db nuke --yes
    • The script name suggests RocksDB, but the code uses FileSystem database backend
  2. Missing confirmation in shell script: Unlike the Rust command which requires --yes, the shell script has no safety check. Consider adding:

    read -p "Are you sure you want to nuke the database? (yes/no): " confirmation
    if [ "$confirmation" != "yes" ]; then
        echo "Aborted"
        exit 1
    fi
  3. Inconsistent naming: The script is named nuke-rocksdb.sh but the Rust code handles FileSystem database backends. The naming suggests RocksDB specifically, which might be confusing.

  4. Logging convention: Consider using tracing::info! instead of println! for consistency with the project's logging patterns mentioned in CLAUDE.md:

    tracing::info!(path = %file_system.path.display(), "removing database");
    // ... operation ...
    tracing::info!("database removal complete");
  5. Minor: RUST_LOG improvement in engine-rocksdb.sh: Good addition to make it configurable, but this is unrelated to the main PR purpose (consider mentioning in PR description or separating).

🔒 Security

🧪 Test Coverage

Consider adding:

  • A test that verifies the --yes flag is required
  • A test that simulates the nuke operation (perhaps with a temporary directory)

📝 Documentation

Consider adding to the PR description:

  • Use cases for when developers should use this command
  • Whether this is intended for local development only
  • The relationship between the Rust command and the shell script

Summary

The Rust implementation looks solid with good safety mechanisms. The main issue is the critical bug in the shell script where the tilde won't expand. I'd recommend either fixing the shell script or removing it in favor of using the Rust command directly.

Recommended action: Fix the shell script bug before merging, or consider removing the shell script entirely and documenting the Rust command usage instead.

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Overview

This PR adds a db nuke subcommand to the engine for clearing database state during development. The implementation adds the command to the Rust CLI and creates a helper shell script.


Issues Found

🔴 Critical: Shell Script Bug

File: scripts/run/nuke-rocksdb.sh:9

The path in the shell script is quoted, which prevents tilde expansion:

rm -rf "~/Library/Application Support/rivet-engine/"

This will fail because the shell won't expand ~ when it's inside quotes. The command will try to delete a literal directory named ~ in the current directory.

Fix:

rm -rf ~/Library/Application\ Support/rivet-engine/
# or
rm -rf "$HOME/Library/Application Support/rivet-engine/"

🟡 Issue: Hardcoded macOS Path

File: scripts/run/nuke-rocksdb.sh:9

The script hardcodes a macOS-specific path (~/Library/Application Support/) but doesn't match the Rust implementation which uses dirs::data_local_dir() for cross-platform compatibility.

Concerns:

  1. Won't work on Linux/Windows
  2. Path doesn't match what the Rust code uses (see packages/config/src/config/db.rs:46-48)
  3. Script name suggests it's RocksDB-specific but the path is generic

Recommendation: Either:

  • Remove this script and use cargo run --bin rivet-engine -- db nuke --yes directly
  • Or make it cross-platform by calling the Rust command instead of hardcoding paths

Code Quality

✅ Good Practices

  1. Safety flag: The --yes flag prevents accidental database deletion
  2. Clear error messages: Good user feedback with bail! messages
  3. Structured logging: Uses println! for user-facing output (appropriate for CLI)
  4. Consistent style: Follows existing command patterns in the module

🟢 Minor: Consider Using Structured Logging

File: engine/packages/engine/src/commands/db/mod.rs:71-73

According to CLAUDE.md, structured logging with tracing is preferred. Consider:

tracing::info!(path = %file_system.path.display(), "removing database directory");
// ... operation ...
tracing::info!("database directory removed");

However, for CLI commands directly interacting with users, println! is acceptable.

🟢 Minor: Error Message Capitalization

File: engine/packages/engine/src/commands/db/mod.rs:68

Per CLAUDE.md logging guidelines (lowercase unless mentioning code symbols), consider:

bail!("nuke command not implemented for postgres");

Security Considerations

✅ Good Safety Measures

  1. Requires explicit --yes flag to prevent accidents
  2. Uses tokio::fs::remove_dir_all which is safer than shell commands
  3. Prints the path before deletion for user verification

🟡 Consider: Additional Safeguards

For a destructive operation, consider:

  • Checking if the directory exists before attempting deletion
  • Adding a warning about data loss
  • Logging what was deleted (file count, size, etc.)

Testing

⚠️ No Test Coverage

No tests were added for this functionality. Consider adding:

  1. Unit test for the --yes flag validation
  2. Integration test that verifies the command structure
  3. Test that verifies error handling for Postgres case

Performance

No performance concerns - this is a simple file system operation.


Summary

Must Fix:

  • 🔴 Fix tilde expansion bug in nuke-rocksdb.sh

Should Fix:

  • 🟡 Make shell script cross-platform or remove it
  • 🟡 Consider adding basic test coverage

Nice to Have:

  • 🟢 Use lowercase in error messages
  • 🟢 Add safeguards (existence check, warnings)

The Rust implementation itself is solid and follows good practices. The main issue is the shell script which has a critical bug and platform compatibility issues.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
  • Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
  • Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
  • Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@NathanFlurry NathanFlurry force-pushed the 11-02-fix_rivetkit_fix_method_get_arraybuffer.prototype.bytelength_called_on_incompatible_receiver_arraybuffer_ branch from 9d52114 to 84e9dc6 Compare November 4, 2025 20:25
@NathanFlurry NathanFlurry force-pushed the 11-02-chore_engine_add_db_nuke_subcommand branch from cf3b3e3 to 620ef31 Compare November 4, 2025 20:25
@claude
Copy link

claude bot commented Nov 4, 2025

PR Review

Thank you for adding the db nuke subcommand! This is a useful developer tool. I've identified several issues that should be addressed:

Critical Issues

1. Bug in nuke-rocksdb.sh - Tilde expansion won't work (line 9)

The path is quoted, which prevents shell tilde expansion:

rm -rf "~/Library/Application Support/rivet-engine/"

This will literally try to remove a directory named ~ instead of expanding to the home directory. Fix:

rm -rf ~/Library/Application\ Support/rivet-engine/
# or
rm -rf "$HOME/Library/Application Support/rivet-engine/"

2. Platform-specific hardcoded path

The script hardcodes a macOS-specific path (~/Library/Application Support), but the repository appears to support multiple platforms. This won't work on Linux or Windows. Consider either:

  • Using the engine's own db nuke command instead: cargo run --bin rivet-engine -- db nuke --yes
  • Or detecting the platform and using the appropriate path

Code Quality Issues

3. Logging pattern violation (engine/packages/engine/src/commands/db/mod.rs:71, 73)

Per CLAUDE.md, use tracing instead of println! for logging:

// Current
println!("Removing {}", file_system.path.display());
println!("Complete");

// Should be
tracing::info!(path = %file_system.path.display(), "removing database");
tracing::info!("database removed");

4. Missing error context (line 72)

Add context to the error for better debugging:

tokio::fs::remove_dir_all(&file_system.path)
    .await
    .context("failed to remove database directory")?;

Positive Aspects

  • Good safety mechanism with the --yes flag requirement
  • Clear error message for Postgres (not implemented)
  • Consistent with existing command structure
  • RUST_LOG environment variable override in engine-rocksdb.sh is a nice improvement

Recommendations

  1. Fix the critical tilde expansion bug in nuke-rocksdb.sh
  2. Either make the script cross-platform or just use the Rust command directly
  3. Replace println! with tracing::info!
  4. Add error context with .context()
  5. Consider adding a test case for the nuke command if there's test infrastructure for CLI commands

Test Plan Suggestion

After fixing:

  • Test on macOS that the path is correctly resolved and removed
  • Test the --yes flag requirement (should error without it)
  • Test with FileSystem database backend
  • Verify error message for Postgres backend

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: Add db nuke subcommand

Thank you for this PR! The addition of a db nuke subcommand is useful for development workflows. Here is my feedback:


Critical Issues

1. Shell Script Bug in scripts/run/nuke-rocksdb.sh (scripts/run/nuke-rocksdb.sh:9)

The tilde (~) is quoted, which prevents shell expansion. This will attempt to remove a literal directory named ~/Library/Application Support/rivet-engine/ instead of expanding to the user home directory.

Current:

rm -rf "~/Library/Application Support/rivet-engine/"

Should be:

rm -rf ~/Library/Application\ Support/rivet-engine/
# or
rm -rf "$HOME/Library/Application Support/rivet-engine/"

However, this hardcoded macOS path is problematic. See suggestion below.

2. Shell Script Should Use Rust Command (scripts/run/nuke-rocksdb.sh)

The shell script hardcodes a macOS-specific path and does not leverage the Rust db nuke command you just created. This creates inconsistency between the two approaches. The shell script should call the Rust command instead, which already handles the correct path via rivet_config and works cross-platform (similar to how engine-rocksdb.sh works).


Code Quality Issues

3. Consider Using Structured Logging (engine/packages/engine/src/commands/db/mod.rs:71-73)

The CLAUDE.md guidelines recommend using tracing for structured logging rather than println!. While println! is acceptable for CLI output, consider whether this should be logged instead. However, for direct user feedback in CLI commands, println! is probably fine.


Security & Safety Considerations

4. Good: Confirmation Flag Pattern

The --yes flag requirement is a good safety pattern for destructive operations. This prevents accidental data loss.

5. Consider Adding Additional Safety Checks (engine/packages/engine/src/commands/db/mod.rs:70-74)

Before removing the directory, you might want to check if the directory exists and inform the user if it does not, and warn about the path being deleted.


Test Coverage

6. Missing Tests

There are no tests for the new db nuke command. Consider adding integration tests to verify the command correctly requires the --yes flag, fails appropriately for Postgres (unimplemented), and successfully removes FileSystem database directories.


Documentation

7. Missing PR Description

The PR body is empty. Please add a description explaining why this command is needed, how it should be used, and any safety considerations.


Minor Improvements

8. Consistent Messaging (engine/packages/engine/src/commands/db/mod.rs:73)

Consider changing "Complete" to "Database nuked successfully" to provide clearer feedback.

9. RUST_LOG Enhancement is Good (scripts/run/engine-rocksdb.sh:9)

The change to RUST_LOG="${RUST_LOG:-debug}" is a nice improvement that allows users to override the default log level.


Summary

Must Fix:

  1. Fix the shell script tilde quoting bug or replace with Rust command call
  2. Consider cross-platform compatibility

Should Consider:
3. Add tests for the new functionality
4. Add PR description
5. Enhance safety checks for directory deletion

Nice to Have:
6. Improve user messaging consistency

Overall, this is a useful addition! The main concern is the shell script bug and cross-platform considerations. The Rust code looks solid and follows good safety patterns with the confirmation flag.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 11-02-chore_engine_add_db_nuke_subcommand branch November 4, 2025 20:52
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