Skip to content

[SLOP(gpt-5)] feat(cli): add rivetkit cli#5255

Open
NathanFlurry wants to merge 1 commit into
stack/slop-claude-opus-4-8-1m-medium-docs-website-consolidate-landing-pages-deploy-quickstarts-and-docs-restructuring-psyxxqonfrom
stack/slop-gpt-5-feat-cli-add-rivetkit-cli-kprpzusk
Open

[SLOP(gpt-5)] feat(cli): add rivetkit cli#5255
NathanFlurry wants to merge 1 commit into
stack/slop-claude-opus-4-8-1m-medium-docs-website-consolidate-landing-pages-deploy-quickstarts-and-docs-restructuring-psyxxqonfrom
stack/slop-gpt-5-feat-cli-add-rivetkit-cli-kprpzusk

Conversation

@NathanFlurry

Copy link
Copy Markdown
Member

[SLOP(claude-opus-4-8-medium)] refactor(cli): extract engine-process crate, rework rivet dev providers

[SLOP(claude-opus-4-8-low)] docs(actors): drop dev install of @rivetkit/cli from cloudflare and supabase quickstarts

[SLOP(claude-opus-4-8-medium)] refactor(cli): extract engine-process crate, rework rivet dev providers

[SLOP(claude-opus-4-8-low)] docs(actors): drop dev install of @rivetkit/cli from cloudflare and supabase quickstarts
@NathanFlurry

Copy link
Copy Markdown
Member Author

Stack for rivet-dev/rivet

Get stack: forklift get 5255
Push local edits: forklift submit
Merge when ready: forklift merge 5255

@claude

claude Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Code Review

This PR introduces the rivet CLI (a new Rust binary under engine/packages/cli/), extracts the engine-process management logic into the new rivetkit-engine-process crate (previously embedded in rivetkit-core), adds a @rivetkit/cli npm package wrapper, updates the publish pipeline, and ships the Supabase icon + quickstart docs. The structure is clean overall, but I flagged several issues below.


Bugs / Correctness

CloudClient::request vs request_ok — near-identical methods
engine/packages/cli/src/cloud.rs has two public async methods (request<T> and request_ok<T>) whose bodies differ by only one line: request returns Ok(None) on HTTP 404 before reading the body, while request_ok skips that check. The only call site of request_ok is create_or_update_pool (a PUT that never 404s). These should be unified into one method with an optional treat_404_as_none: bool parameter, or the 404-special-case logic should be the only difference. As-is the duplication is a maintenance hazard — if the auth logic changes it must be updated in two places.

serverless_config move-before-use after ordering flip in upsert.rs
engine/packages/pegboard/src/ops/runner_config/upsert.rs moves the endpoint_config_changed block (cache purge + metadata refresh) before pool_created. serverless_config was previously consumed inside the old endpoint_config_changed block. Make sure the borrow checker doesn't silently shadow an earlier serverless_config extraction; the diff does show one if let Some((url, headers)) = serverless_config in the new block, so verify that serverless_config is not also consumed earlier in the function body and that the reordering is safe with respect to pool_created.

wait_for_pool silent timeout on the "error" branch
engine/packages/cli/src/cloud.rs wait_for_pool:

"error" => return Ok(()),

When throw_on_error is false, an error status silently returns Ok(()). The caller in deploy.rs then proceeds to build and push an image against a pool that failed. Consider returning a non-fatal warning log at minimum, or documenting why silent success here is correct.

CARGO_MANIFEST_DIR used in local_engine_search_roots in a production binary
rivetkit-rust/packages/engine-process/src/lib.rs:

fn local_engine_search_roots() -> Vec<PathBuf> {
    Path::new(env!("CARGO_MANIFEST_DIR"))
        .ancestors()
        ...
}

env!("CARGO_MANIFEST_DIR") resolves to the source directory of the rivetkit-engine-process crate at compile time. This is fine in development (both binaries live in the same workspace), but once the CLI is distributed as an npm package, CARGO_MANIFEST_DIR will point to a non-existent path inside the user's npm cache. The function always returns a vec of stale paths that find_local_engine_binary_in_roots will never find a binary in. This is benign but confusing — the comment calling these "local roots" implies they exist at runtime. At minimum add a comment that these paths are only valid in the source tree, or filter out roots that don't exist before iterating.


Style / Convention Violations (CLAUDE.md)

println! in production Rust code
engine/packages/cli/src/commands/deploy.rs:

// The dashboard URL is the command's result; print it to stdout so it
// can be captured by scripts.
println!("{dashboard_url}");

CLAUDE.md: "Never use eprintln! or println! for logging in Rust code." The comment acknowledges this is intentional (script capture), so this is a documented exception, but the intent should be clearer. If the value is machine-readable output (not a log), a dedicated output path (e.g. writing directly to stdout with std::io::Write) is better than println! which is fine here functionally but may conflict with the tracing subscriber's stderr output model. At minimum, keep the comment.

Wildcard _ => arm in build_handler_url
engine/packages/cli/src/commands/dev.rs:

fn build_handler_url(provider: Option<Provider>, fn_name: &str, port: u16) -> String {
    match provider {
        Some(Provider::Supabase) => { ... }
        _ => format!("http://127.0.0.1:{port}/api/rivet"),
    }
}

CLAUDE.md: "Never use a _ => fall-through arm when matching on a Rust enum." If a new Provider variant is added, this silently uses the default URL instead of producing a compile error. Enumerate all variants explicitly:

Some(Provider::Cloudflare) | Some(Provider::Serverless) | None => ...,
Some(Provider::None) => unreachable!(...),

wait_for_pool uses a _ => arm on the status string
engine/packages/cli/src/cloud.rs:

_ => sleep(Duration::from_secs(2)).await,

This is matching on a &str returned from an API, which is an open value space, so _ => is appropriate here per CLAUDE.md. No action needed.

tracing::info! with format! inside a structured field
engine/packages/cli/src/util.rs:

tracing::info!(command = %format!("{} {}", program, args.join(" ")), "running command");

CLAUDE.md: "Do not format parameters into the main message. Use structured fields." The field command is a structured field (good), but it uses format! to produce a single string instead of separate program and args fields. Prefer:

tracing::info!(program, args = ?args, "running command");

Em dashes in new documentation files
website/src/content/docs/deploy/cli.mdx and website/src/content/docs/deploy/rivet-compute.mdx contain em dashes () in bullet points:

- The GitHub Actions workflow completed without errors — check the run logs
- A runtime crash on startup — test the image locally with `docker run`

CLAUDE.md: "Never use em dashes (—) in any plain-English writing." Replace with periods or restructure the sentences.

Duplicate pnpm build -F rivetkit in publish.yaml

run: |
  pnpm build -F rivetkit
  pnpm build -F rivetkit -F '@rivetkit/*' ...

rivetkit is built twice. The first standalone invocation appears to be intended to ensure the meta-package builds before the rest, but the second invocation already includes -F rivetkit. Verify this is intentional and add a comment explaining the order dependency; otherwise remove the first line if the second is sufficient.


Security

Hardcoded "dev" bearer token in register_runner_config
engine/packages/cli/src/commands/dev.rs:

.bearer_auth("dev")

The local engine's dev auth token is hardcoded. This is acceptable for local development, but:

  1. The value should be a named constant (e.g. DEV_AUTH_TOKEN) so it is obvious it is intentional and easy to change.
  2. Add a comment noting this is the engine's dev-mode token, not a user credential.

rivetkit-typescript/packages/cli/npm/*/package.json packages ship an engine binary
The CI step that places the rivet-engine binary next to the rivet CLI binary is a good design. Ensure the binary paths are validated at runtime before execution (which verify_binary_path does) and that the engine binary download checksum validation in download_engine_binary is always enforced — it is, via the SHA256SUMS manifest check.


Missing Pieces

No AGENTS.md symlinks for new crates
CLAUDE.md: "Every directory that has a CLAUDE.md must also have an AGENTS.md symlink pointing to it." The new engine/packages/cli/ and rivetkit-rust/packages/engine-process/ directories do not have CLAUDE.md files, so no symlinks are required there. However, frontend/packages/icons/CLAUDE.md is modified (adding new content) — verify its AGENTS.md symlink already exists.

Missing error event handler on child in scripts/publish/src/lib/npm.ts
Both runNpmDistTagAdd and npmViewLatestTag spawn child processes and listen for close, but neither listens for the error event. If npm is not found or fails to spawn, the Promise never resolves or rejects and the publish pipeline hangs indefinitely. Add:

child.on("error", (err) => reject(err));

wait_for_pool uses a bounded for _ in 0..180 loop with a sleep inside
engine/packages/cli/src/cloud.rs polls via for _ in 0..180 { ... sleep(2s) } (6-minute maximum). CLAUDE.md discourages loop { check; sleep } patterns in favor of event-driven approaches, but for an HTTP polling loop against an external API (cloud deploy status) there is no event source available, and the CLAUDE.md caveat covers sleep_until(deadline) arms in an event-select loop. This is acceptable — but the total timeout of 360 seconds should be named as a constant for clarity.


Minor Notes

  • rivetkit-engine-process is correctly placed in rivetkit-rust/packages/ and included in rivetkit-core behind the native-runtime feature flag. The extraction is clean.
  • The pegboard-outbound change (serialize_with_embedded_versionserialize + manual prefix bytes) looks like a deliberate protocol serialization fix; no issues found.
  • frontend/packages/shared-data/src/deploy.ts correctly removes the hand-rolled faSupabase definition now that the real icon is in the kit. Good cleanup.
  • credentials.rs correctly sets 0o600 permissions on the credentials file on Unix. The double permission set (at open time via OpenOptionsExt::mode and again via set_permissions) is redundant but harmless.

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.

1 participant