Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

@jog1t
Copy link
Collaborator

@jog1t jog1t commented Oct 20, 2025

No description provided.

@vercel
Copy link

vercel bot commented Oct 20, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-next-js-max Ready Ready Preview Comment Oct 22, 2025 3:36am
rivetkit-serverless Ready Ready Preview Comment Oct 22, 2025 3:36am

@jog1t jog1t marked this pull request as ready for review October 20, 2025 15:46
Copy link
Collaborator Author

jog1t commented Oct 20, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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 20, 2025

Pull Request Review

Summary

This PR fixes a bug in the getActor function in the remote manager driver by removing the unused name parameter from the API query string. The change correctly aligns the API call with the backend implementation.

Analysis

The Bug

The previous implementation was passing both name and actor_ids query parameters:

`/actors?name=${name}&actor_ids=${encodeURIComponent(actorId)}`

However, looking at the backend router (packages/rivetkit/src/manager/router.ts:264-268), the /actors GET endpoint expects:

  • name: required string
  • actor_ids: optional string
  • key: optional string

The Problem

The backend logic (router.ts:301-311) shows that when actor_ids are provided, the name parameter is used to validate the actor name matches. However, the remote manager driver was inconsistently using this endpoint:

  1. getActor (line 135 in mod.ts) - Fetches by actor ID and validates name matches afterward (lines 140-148)
  2. getActorByKey (line 29 in api-endpoints.ts) - Correctly uses name + key parameters

The fix appropriately reflects that when fetching by actor ID directly, the name validation happens after the API call in the driver, not as a query filter on the backend.

Code Quality: ✅ Good

Pros:

  • Removes unused parameter, improving code clarity
  • Uses underscore _ convention to indicate intentionally unused parameter
  • Maintains backward compatibility (the backend doesn't strictly require name when actor_ids is provided)
  • Consistent with the pattern used in getActorByKey which properly uses name

Minor Consideration:
The backend router query schema (line 265) marks name as required, but the implementation (lines 302-311) only uses it when filtering by actor_ids. This creates a slight inconsistency between the schema and the implementation. However, this PR correctly adapts to the actual backend behavior.

Potential Issues: ⚠️ Minor

  1. Backend Schema Mismatch: The OpenAPI schema defines name as required, but the implementation treats it as optional when actor_ids is provided. This could cause issues with API validation or documentation.

    Recommendation: Consider updating the backend schema to make name optional, or enforce its usage in the implementation.

  2. Missing Test Coverage: No tests were found that specifically test the getActor function's API call parameters.

    Recommendation: Add integration tests to verify the correct parameters are passed to the /actors endpoint.

Security: ✅ No Concerns

  • Proper URL encoding is maintained via encodeURIComponent
  • No new attack vectors introduced

Performance: ✅ No Impact

  • Slightly reduces query string size (minimal impact)
  • No additional network calls or processing

Best Practices: ✅ Follows Guidelines

  • Adheres to the project's style (using _ for unused params)
  • Maintains existing error handling
  • Clear, descriptive commit message

Verdict: ✅ Approved with Recommendations

This is a valid bug fix that correctly aligns the remote driver with the backend implementation. The change is minimal, low-risk, and improves code correctness.

Action Items (optional, for future work):

  1. Consider aligning the backend OpenAPI schema with the actual implementation regarding the name parameter requirement
  2. Add test coverage for the getActor API endpoint parameters

Great catch on this inconsistency! 🎯

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 20, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: b8d85f3

@NathanFlurry NathanFlurry merged commit 433d74c into main Oct 22, 2025
5 of 8 checks passed
@NathanFlurry NathanFlurry deleted the 10-20-fix_get_actor_in_remote_driver branch October 22, 2025 03:30
@claude
Copy link

claude bot commented Oct 22, 2025

Code Review

This PR fixes the remote driver's getActor functionality and adds action execution capability to the inspector. Overall, the changes look good and follow the codebase conventions. Here's my detailed feedback:

✅ Positive Points

  1. Remote Driver Fix (api-endpoints.ts:15-26): Removing the unused name parameter from the query string is correct. Since the API call at line 139 in mod.ts validates the name after receiving the response, including it in the query could have been causing issues or was simply redundant.

  2. Inspector Action Execution: The new executeAction capability is well-implemented and follows existing patterns for other inspector accessors.

  3. Connection Lifecycle Management: The implementation properly creates and cleans up the temporary connection in the finally block (instance.ts:252-253), which is good practice.

🔍 Code Quality & Best Practices

Good:

  • Import changes follow the project style (moving from type to named import for ActionContext and ConnDriverKind)
  • Structured logging would apply here but you're creating a synthetic connection, so no logging is needed
  • Error handling is delegated to the existing executeAction method
  • The API endpoint validation uses zod schema properly

Minor Observations:

  • The name parameter in getActor is now unused (renamed to _), which is fine for indicating it's intentionally ignored
  • The connection created for inspector action execution uses ConnDriverKind.HTTP which is semantically correct

🐛 Potential Issues

Medium Priority:

  1. Connection Cleanup (instance.ts:253): The __connDisconnected call passes true for the second parameter. I'd recommend verifying this is the correct flag - ensure it doesn't inadvertently trigger side effects like persisting connection state that shouldn't be persisted for these synthetic connections.

  2. Missing Error Handling (inspector/actor.ts:260-267): The new /action endpoint doesn't have try-catch error handling, unlike the /db/query endpoint above it (lines 242-251). If executeAction throws, it could result in an unhandled exception. Consider adding:

    try {
        const result = await c.var.inspector.accessors.executeAction(name, params);
        return c.json({ result }, 200);
    } catch (error) {
        return c.json({ error: (error as Error).message }, 500);
    }

Low Priority:

  1. Type Safety (inspector/actor.ts:258): Using z.array(z.any()) for params reduces type safety. While this may be unavoidable for a generic action executor, consider if there's a way to validate the structure better or document this limitation.

🔒 Security Considerations

  • Inspector Access Control: Ensure the inspector endpoints have proper authentication/authorization. These endpoints can now execute arbitrary actions, which could be sensitive.
  • The synthetic connection created doesn't validate connection parameters, which is fine for inspector use but worth noting.

⚡ Performance Considerations

  • Creating a connection per action execution has overhead (UUID generation, connection setup/teardown). This is acceptable for an inspector/debugging tool but would be problematic for high-frequency operations.
  • The connection is properly cleaned up in the finally block, which prevents memory leaks.

🧪 Test Coverage

Missing Tests:

  • No tests found for the inspector endpoints or the remote manager driver
  • Recommend adding tests for:
    • The new /action endpoint
    • The executeAction inspector accessor
    • The remote driver getActor behavior (especially the name validation logic in mod.ts:138-147)

📝 Documentation

  • Consider adding JSDoc comments to the executeAction accessor explaining that it creates a synthetic connection
  • The inspector API endpoint could benefit from documentation about what actions can be executed and any limitations

🎯 Summary

Required Changes:

Recommended Changes:

  • Verify the __connDisconnected parameters are correct
  • Add test coverage for new functionality
  • Consider adding JSDoc comments

Overall Assessment: ✅ Approve with minor changes

The core fix is solid and the new functionality is well-implemented. The main concern is the missing error handling in the API endpoint, which should be addressed before merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants