chore: fix diff between ee, fix plural api query params #3524

MasterPtato · 2025-11-21T22:46:12Z

No description provided.

vercel · 2025-11-21T22:46:16Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Dec 2, 2025 7:11pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Dec 2, 2025 7:11pm
rivet-inspector	Ignored	Preview	Dec 2, 2025 7:11pm
rivet-site	Ignored	Preview	Dec 2, 2025 7:11pm

MasterPtato · 2025-11-21T22:46:30Z

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 · 2025-11-21T22:48:57Z

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:

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.
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

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 · 2025-11-21T22:50:16Z

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: d4c7511

claude · 2025-11-22T02:48:36Z

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:

Backward compatibility: The deprecated comma-separated parameters are still supported alongside the new array-based parameters, allowing gradual migration.
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.
Proper deprecation markers: Doc comments with "Deprecated." are added to the old parameters, which will appear in OpenAPI docs.

Potential Issues ⚠️

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.
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 ✅

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.
Caching preserved: The namespace resolution and runner finding operations maintain their existing cache patterns.

Security Concerns ✅

Input validation: The TooManyActorIds validation (max 32) prevents abuse via excessive ID lists.
Namespace filtering: Actors are properly filtered by namespace ID to ensure users can only see actors in their namespace.

Test Coverage ⚠️

Test files updated: engine/packages/engine/tests/common/actors.rs was updated to use the new parameters, which is good.
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

OpenAPI version updated: Version bump to 2.0.25-rc.1 is reflected in both Cargo.lock and openapi.json ✅
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.

claude · 2025-11-25T03:01:36Z

PR Review: Fix diff between EE, fix plural API query params

Overview

This PR addresses synchronizing differences between editions and migrating from comma-separated to array-based query parameters.

Code Quality

Strengths

Proper API deprecation with backward compatibility
Consistent pattern across endpoints (actor_ids->actor_id, namespace_ids->namespace_id, etc)
OpenAPI spec updated
Efficient merging logic for old and new parameters

Issues Found

Must Fix

engine/packages/api-public/src/actors/utils.rs:16 - Internal function still uses deprecated parameter

Current:

actor_ids: Some(actor_id.to_string()),

Should be:

actor_id: vec![actor_id],

Should Consider

No tests added for backward compatibility
Dependency changes not explained in description
Verify docker configs were generated correctly from templates

Performance

Good: Efficient batching by datacenter, parallel execution

Security

Good: valid_hosts whitelisting added
Minor: Verify localhost entries only for dev environments

Verdict

Solid refactoring with good backward compatibility. Main issue is internal function using deprecated API.

Recommendation: Update utils.rs:16 and add backward compatibility tests.

claude · 2025-11-26T02:21:29Z

Pull Request Review

This PR modernizes API query parameters by migrating from comma-separated string values to proper array-based query parameters, along with configuration updates for the development environment. Overall, the changes are well-structured and maintain backward compatibility.

✅ Strengths

Backward Compatibility: The implementation properly deprecates old parameters while maintaining support for them. This prevents breaking existing API clients.
Consistent Pattern: The same approach is applied consistently across all affected endpoints.
OpenAPI Documentation: The OpenAPI spec is properly updated with deprecation notices and correct type definitions for array parameters.

🔍 Issues & Concerns

1. Critical: Inconsistent Validation in api-public/actors/list.rs

Location: engine/packages/api-public/src/actors/list.rs:82-86

The code still only parses the old actor_ids parameter but doesn't include the new actor_id field. This means the public API won't accept the new actor_id array parameter, only the deprecated actor_ids string. This is inconsistent with the peer API implementation.

The code should be updated to concatenate both parameters like in the peer API.

2. Docker Config Duplication

All docker/dev* configs have duplicate 127.0.0.1 in valid_hosts. Remove one of the duplicate entries in the template at docker/template/src/services/edge/rivet-engine.ts:23.

3. Missing Test Coverage

The PR doesn't appear to add new tests for the dual parameter support. Consider adding tests that verify both old and new parameters work correctly.

🎯 Recommendations

Fix the public API actors list endpoint to handle the new actor_id parameter (critical)
Remove duplicate 127.0.0.1 entries in valid_hosts
Add tests to verify that both old and new query parameters work correctly
Consider documenting when the deprecated parameters will be removed

Summary

This is a good cleanup that modernizes the API. The main blocker is issue #1 - the public API actors endpoint needs to be updated to match the peer API implementation. After addressing these issues, this PR will be ready to merge.

Verdict: Needs changes before merge ⚠️

claude · 2025-11-27T00:57:29Z

Code Review - PR #3524

This PR fixes differences between enterprise edition (EE) and improves plural API query parameters by introducing proper array-based query parameters while maintaining backward compatibility with the deprecated string-based format.

Summary

Changes: Migrates API query parameters from comma-separated strings to proper arrays
Files Changed: 43 files
Additions: +437 lines
Deletions: -135 lines

Positive Aspects ✅

Backward Compatibility: Excellent approach to deprecation - the PR properly maintains backward compatibility by keeping the old *_ids and *_names parameters while marking them as deprecated in the OpenAPI spec.
Consistent Pattern: The migration pattern is consistent across all endpoints:
- /actors: actor_ids → actor_id (array)
- /namespaces: namespace_ids → namespace_id (array)
- /runner-configs: runner_names → runner_name (array)
- /runners: runner_ids → runner_id (array)

Clean Array Merging: The concatenation pattern to merge deprecated and new params is clean:

let actor_ids = [
    query.actor_id,
    query.actor_ids.map(|x| x.split(',')...).unwrap_or_default(),
].concat();

Config Synchronization: Added valid_hosts to all Docker configs ensures proper host validation across all environments.
Dependency Cleanup: Good cleanup in Cargo.toml files removing unused dependencies.

Issues & Concerns 🔴

1. Unused Import in api-public/src/actors/list.rs (Line 3)

use gas::prelude::*;

This import was added but doesn't appear to be used. The Id type is already imported via rivet_api_types. This should be removed unless there's a specific reason for it.

Location: engine/packages/api-public/src/actors/list.rs:3

2. Inconsistent Error Handling Pattern

In api-public/src/actors/list.rs:98-103, the code uses a custom errors::Validation::InvalidInput struct:

return Err(errors::Validation::InvalidInput {
    message: "Cannot provide both actor_id and (name + key)...".to_string(),
}.build());

However, in api-peer/src/actors/list.rs:71, similar validation uses bail!:

bail!("name is required when not using actor_ids")

Recommendation: Standardize on one approach. The structured error approach in api-public is better for API consistency and provides better error metadata.

3. Potential Issue with Array Concatenation

The array merging logic concatenates query.actor_id (a Vec<Id>) with parsed values from query.actor_ids:

let actor_ids = [
    query.actor_id,
    query.actor_ids.map(|x| /* parse */).unwrap_or_default(),
].concat();

Concern: If a user provides both actor_id[]=1&actor_id[]=2 AND the deprecated actor_ids=3,4, they'll get all four IDs [1,2,3,4]. This might not be the intended behavior - typically, you'd want to reject requests that use both formats.

Recommendation: Add validation to ensure only one format is used:

if !query.actor_id.is_empty() && query.actor_ids.is_some() {
    return Err(/* error about using both formats */);
}

4. Missing Validation in Peer API

The api-peer endpoints don't have the same level of validation as api-public. For example:

api-peer/src/actors/list.rs doesn't validate the 32 actor limit
api-peer/src/actors/list.rs doesn't validate mutually exclusive parameters

Recommendation: Since peer API is called by public API, consider whether these validations should exist in both layers or only in public API. Document the decision.

5. Cargo.toml Changes Seem Unrelated

The changes to engine/packages/bootstrap/Cargo.toml and engine/packages/guard/Cargo.toml remove some dependencies but the PR title/description doesn't mention this cleanup.

Questions:

Were rivet-api-types and uuid truly unused in bootstrap?
Is the urlencoding addition to engine related to the query parameter changes?

Recommendation: Ensure these dependency changes are intentional and tested.

Performance Considerations ⚡

String Parsing Overhead: The deprecated path still parses comma-separated strings:
```
x.split(',').filter_map(|s| s.trim().parse::<rivet_util::Id>().ok())
```
This is fine for backward compatibility but document that the array-based approach should be preferred.
Array Concatenation: The .concat() operation creates a new vector. For the expected scale (max 32 actors), this is negligible.

Security Considerations 🔒

Input Validation: The 32 actor limit in api-public/src/actors/list.rs:115-121 is good security practice to prevent abuse.
Silent Parsing Failures: The filter_map with .ok() silently ignores invalid IDs:
```
.filter_map(|s| s.trim().parse::<rivet_util::Id>().ok())
```
Question: Should invalid IDs return an error instead of being silently dropped? Current behavior could hide mistakes.

Testing 🧪

Missing: The PR doesn't include test updates. Given the API contract changes:

Should add tests for:
- New array-based query parameters
- Backward compatibility with deprecated string format
- Validation when both formats are provided
- Edge cases (empty arrays, invalid IDs, etc.)

Locations to add tests:

engine/packages/engine/tests/common/actors.rs (which was modified)
New integration tests for the API endpoints

Documentation 📝

OpenAPI Updates: Good that engine/artifacts/openapi.json was updated with deprecation notices.
Missing Migration Guide: Consider adding inline code comments or documentation about:
- When the old parameters will be removed
- Migration examples for API consumers
- Performance differences (if any)

Recommendations Summary

Must Fix 🔴

Remove unused gas::prelude::* import from api-public/src/actors/list.rs:3
Add validation to prevent using both old and new parameter formats simultaneously
Add tests for the new query parameter behavior

Should Consider 🟡

Standardize error handling approach across api-public and api-peer
Document validation strategy differences between API layers
Add validation to peer API endpoints or document why it's not needed
Add inline comments about deprecation timeline
Consider returning errors for invalid IDs instead of silently dropping them

Nice to Have 🟢

Add migration guide for API consumers
Performance benchmarks if this changes hot paths
Document the dependency changes in Cargo.toml files

Conclusion

This is a well-structured refactoring that improves API consistency and follows RESTful best practices for array query parameters. The backward compatibility approach is solid. Main concerns are around validation completeness and potential edge cases with simultaneous use of old and new parameter formats.

Recommendation: Request changes for the must-fix items, particularly around validation and testing.

graphite-app · 2025-12-03T00:33:00Z

Merge activity

Dec 3, 12:33 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Dec 3, 12:33 AM UTC: CI is running for this pull request on a draft pull request (#3577) due to your merge queue CI optimization settings.
Dec 3, 12:34 AM UTC: Merged by the Graphite merge queue via draft PR: #3577.

MasterPtato requested a review from NathanFlurry November 21, 2025 22:46

vercel bot deployed to Preview – rivetkit-serverless November 21, 2025 22:47 View deployment

MasterPtato force-pushed the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch from 107bfe2 to 3acf83e Compare November 22, 2025 02:45

vercel bot deployed to Preview – rivetkit-serverless November 22, 2025 02:46 View deployment

MasterPtato force-pushed the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch from 3acf83e to a931dae Compare November 25, 2025 02:58

MasterPtato mentioned this pull request Nov 25, 2025

fix(pb): implement events and commands for actor wfs #3543

Closed

vercel bot deployed to Preview – rivetkit-serverless November 25, 2025 02:59 View deployment

MasterPtato force-pushed the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch from a931dae to 2e19120 Compare November 26, 2025 02:16

vercel bot deployed to Preview – rivetkit-serverless November 26, 2025 02:18 View deployment

This was referenced Nov 27, 2025

fix(runtime): evict runners on SIGTERM, add periodic shutdown progress messages #3550

Closed

chore(gh): make claude edit existing discussion comment for subsequent reviews #3551

Closed

MasterPtato force-pushed the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch from 2e19120 to 441926f Compare November 27, 2025 00:52

vercel bot deployed to Preview – rivetkit-serverless November 27, 2025 00:53 View deployment

MasterPtato force-pushed the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch from 441926f to 29d2a5d Compare November 27, 2025 00:58

vercel bot deployed to Preview – rivetkit-serverless November 27, 2025 00:59 View deployment

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

d4c7511

MasterPtato force-pushed the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch from 29d2a5d to d4c7511 Compare December 2, 2025 19:10

This was referenced Dec 2, 2025

feat(pegboard): move pegboard serverless to wf #3416

Closed

fix(serverless): general cleanup #3450

Closed

vercel bot deployed to Preview – rivetkit-serverless December 2, 2025 19:11 View deployment

MasterPtato mentioned this pull request Dec 2, 2025

fix(serverless): drain runners after url/headers change #3571

Closed

NathanFlurry approved these changes Dec 2, 2025

View reviewed changes

This was referenced Dec 2, 2025

fix(serverless): fix connection wf lifecycle #3573

Closed

fix(serverless): misc pr fixes #3576

Closed

graphite-app bot pushed a commit that referenced this pull request Dec 3, 2025

chore: fix diff between ee, fix plural api query params (#3524)

e2cb44c

graphite-app bot closed this Dec 3, 2025

graphite-app bot deleted the 11-21-chore_fix_diff_between_ee_fix_plural_api_query_params branch December 3, 2025 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: fix diff between ee, fix plural api query params #3524

chore: fix diff between ee, fix plural api query params #3524

Uh oh!

MasterPtato commented Nov 21, 2025

Uh oh!

vercel bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

MasterPtato commented Nov 21, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 21, 2025

Uh oh!

pkg-pr-new bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 22, 2025

Uh oh!

claude bot commented Nov 25, 2025

Uh oh!

claude bot commented Nov 26, 2025

Uh oh!

claude bot commented Nov 27, 2025 •

edited

Loading

Uh oh!

graphite-app bot commented Dec 3, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore: fix diff between ee, fix plural api query params #3524

chore: fix diff between ee, fix plural api query params #3524

Uh oh!

Conversation

MasterPtato commented Nov 21, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

claude bot commented Nov 21, 2025

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

Summary

Code Quality

Potential Bugs

Performance

Security

Test Coverage

Minor Notes

Recommendation

Uh oh!

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

Uh oh!

Uh oh!

claude bot commented Nov 22, 2025

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

Summary

Code Quality and Best Practices ✅

Potential Issues ⚠️

Performance Considerations ✅

Security Concerns ✅

Test Coverage ⚠️

Configuration Changes ✅

Minor Suggestions

Overall Assessment

Uh oh!

claude bot commented Nov 25, 2025

PR Review: Fix diff between EE, fix plural API query params

Overview

Code Quality

Strengths

Issues Found

Must Fix

Should Consider

Performance

Security

Verdict

Uh oh!

claude bot commented Nov 26, 2025

Pull Request Review

✅ Strengths

🔍 Issues & Concerns

1. Critical: Inconsistent Validation in api-public/actors/list.rs

2. Docker Config Duplication

3. Missing Test Coverage

🎯 Recommendations

Summary

Uh oh!

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

Uh oh!

Code Review - PR #3524

Summary

Positive Aspects ✅

Issues & Concerns 🔴

1. Unused Import in api-public/src/actors/list.rs (Line 3)

2. Inconsistent Error Handling Pattern

3. Potential Issue with Array Concatenation

4. Missing Validation in Peer API

5. Cargo.toml Changes Seem Unrelated

Performance Considerations ⚡

Security Considerations 🔒

Testing 🧪

Documentation 📝

Recommendations Summary

Must Fix 🔴

Should Consider 🟡

Nice to Have 🟢

vercel bot commented Nov 21, 2025 •

edited

Loading

MasterPtato commented Nov 21, 2025 •

edited

Loading

pkg-pr-new bot commented Nov 21, 2025 •

edited

Loading

claude bot commented Nov 27, 2025 •

edited

Loading

graphite-app bot commented Dec 3, 2025 •

edited

Loading