Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

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

Copy link
Contributor Author

MasterPtato commented Nov 21, 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 Nov 21, 2025

PR Review: chore: fix diff between ee, fix plural api query params

Summary

This PR introduces proper plural query parameter handling for list APIs (actors, namespaces, runners, runner_configs) by adding singular-form array parameters alongside the existing deprecated comma-separated string parameters. It also adds valid_hosts configuration to docker dev configs and fixes a cyclical dependency issue.

Code Quality

Positive:

  • Good backward compatibility approach - old actor_ids, namespace_ids, etc. parameters are deprecated but still functional
  • Clean concatenation pattern using [old, new].concat() to merge both parameter styles
  • Proper use of #[serde(default)] for optional Vec fields
  • Test code updated to use the new URL query builder pattern with proper encoding

Suggestions:

  1. Inconsistent deprecation handling in api-public/src/actors/list.rs:82-86
    The public API handler only parses actor_ids (deprecated) but doesn't merge with actor_id:

    let actor_ids = query.actor_ids.as_ref().map(|x| {
        x.split(',')
            .filter_map(|s| s.trim().parse::<rivet_util::Id>().ok())
            .collect::<Vec<_>>()
    });

    This should follow the same pattern as api-peer handlers to merge both sources. Currently, if a user passes actor_id array params to the public API, they might be ignored in the validation check at line 90 which only checks actor_ids.

  2. fetch_actor_by_id in utils.rs still uses deprecated format
    At line 16-17, fetch_actor_by_id still uses:

    actor_ids: Some(actor_id.to_string()),
    ..Default::default()

    Should be updated to use the new actor_id field for consistency.

Potential Bugs

  1. Public API validation mismatch (line 82-95 in api-public/src/actors/list.rs)
    The validation checks actor_ids.is_some() but the struct has both actor_ids: Option<String> and actor_id: Vec<Id>. A request with only actor_id params would pass the validation check (since actor_ids would be None) but then fail the query.name.is_none() check at line 197 since neither code path handles query.actor_id.

Performance

  • The batching approach in fetch_actors_by_ids is efficient - grouping by datacenter and executing in parallel
  • Using [vec1, vec2].concat() is fine for the expected small sizes of these query params

Security

  • No security concerns identified - proper input parsing with filter_map handles invalid UUIDs gracefully

Test Coverage

  • Test code properly updated to use URL builder pattern
  • The list_actors test helper now correctly appends multiple actor_id query params

Minor Notes

  • Typo fix in epoxy comment ("aftoer" -> "after") - nice catch
  • Cyclical dependency workaround in namespace ops (inline ListResponse struct) is pragmatic

Recommendation

The core changes for plural query params in api-peer look correct. However, the api-public handler needs to be updated to also merge query.actor_id with the parsed actor_ids to maintain consistency and avoid the validation/logic mismatch described above.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 21, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 3acf83e

@claude
Copy link

claude bot commented Nov 22, 2025

PR Review: chore: fix diff between ee, fix plural api query params

Summary

This PR introduces API query parameter improvements by adding proper plural parameter support (actor_id, namespace_id, runner_id, runner_name) while deprecating the comma-separated string versions (actor_ids, namespace_ids, runner_ids, runner_names). It also adds valid_hosts configuration to Docker dev configs and bumps the version to 2.0.25-rc.1.

Code Quality and Best Practices ✅

Good patterns observed:

  1. Backward compatibility: The deprecated comma-separated parameters are still supported alongside the new array-based parameters, allowing gradual migration.

  2. Concatenation pattern: The code consistently uses [query.actor_id, query.actor_ids.map(...).unwrap_or_default()].concat() to merge both parameter styles - this is clean and idiomatic.

  3. Proper deprecation markers: Doc comments with "Deprecated." are added to the old parameters, which will appear in OpenAPI docs.

Potential Issues ⚠️

  1. fetch_actor_by_id still uses deprecated parameter (engine/packages/api-public/src/actors/utils.rs:14-18):

    let list_query = rivet_api_types::actors::list::ListQuery {
        namespace,
        actor_ids: Some(actor_id.to_string()),  // Uses deprecated field
        ..Default::default()
    };

    This function should be updated to use the new actor_id: vec![actor_id] field instead for consistency, since it's setting a single actor ID.

  2. Empty array handling inconsistency: In engine/packages/api-peer/src/actors/list.rs:35-67, when actor_ids is not empty, the code fetches actors and filters by namespace. However, if a user passes an empty actor_id=[] parameter (explicitly empty array), it will concat to empty and fall through to requiring name, which is the expected behavior. Just noting this edge case is handled correctly.

Performance Considerations ✅

  1. Efficient batching: The fetch_actors_by_ids function in utils.rs properly batches requests by datacenter label, executing them in parallel with join_all - this is efficient.

  2. Caching preserved: The namespace resolution and runner finding operations maintain their existing cache patterns.

Security Concerns ✅

  1. Input validation: The TooManyActorIds validation (max 32) prevents abuse via excessive ID lists.

  2. Namespace filtering: Actors are properly filtered by namespace ID to ensure users can only see actors in their namespace.

Test Coverage ⚠️

  1. Test files updated: engine/packages/engine/tests/common/actors.rs was updated to use the new parameters, which is good.

  2. No new tests added: Consider adding integration tests that specifically verify:

    • Both the new array-based and deprecated comma-separated parameters work
    • Empty array handling
    • Mixed usage (both parameters provided - should this be an error or merge?)

Configuration Changes ✅

The valid_hosts additions to Docker configs appear to be standard development configuration updates for host validation.

Minor Suggestions

  1. OpenAPI version updated: Version bump to 2.0.25-rc.1 is reflected in both Cargo.lock and openapi.json

  2. Removed unused dependency: rivet-api-types was removed from namespace/Cargo.toml - ensure this doesn't break any imports (the code appears to define ListResponse locally in the ops files).

Overall Assessment

This is a well-structured change that improves API ergonomics by supporting proper array query parameters. The backward compatibility approach is appropriate. The main actionable item is updating fetch_actor_by_id to use the new parameter style for consistency.

Recommendation: Approve with the minor suggestion to update the deprecated parameter usage in fetch_actor_by_id.

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.

2 participants