fix(core): fix getters on ActorConn proxy

[NathanFlurry]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Sep 26, 2025 7:06am

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: fix(core): fix getters on ActorConn proxy

Todo Checklist:

• Read and analyze the changed file
• Understand the specific fix being made
• Check for potential bugs or issues
• Evaluate code quality and best practices
• Assess performance and security implications
• Check if test coverage is needed
• Provide comprehensive review feedback

---

Review Analysis

Change Summary

The fix changes line 494 in packages/rivetkit/src/client/client.ts:

- const value = Reflect.get(target, prop, receiver);
+ const value = Reflect.get(target, prop, target);

This is within the createActorProxy function's proxy handler, specifically in the getter trap for handling built-in properties and methods.

Code Quality & Best Practices ✅

• Correct Fix: The change properly fixes the proxy receiver issue. Using receiver as the third argument was incorrect because it refers to the proxy itself, potentially causing infinite recursion
• Good Practice: Using target as the receiver ensures the property access is performed on the actual target object (ActorHandleRaw/ActorConnRaw)
• Minimal Change: The fix is surgical and only changes what's necessary

Bug Analysis ✅ FIXED

• Root Issue: The original code Reflect.get(target, prop, receiver) could cause issues when accessing getters/setters because receiver points to the proxy, potentially triggering the proxy handler recursively
• Fix Effectiveness: Using target as the receiver ensures property access happens directly on the underlying object, avoiding proxy recursion
• Impact: This likely fixes scenarios where accessing properties on ActorConn instances was failing or behaving unexpectedly

Performance Considerations ✅

• Minimal Impact: The change has no performance implications - still a single Reflect.get call
• Prevents Recursion: Actually improves performance by preventing potential infinite recursion scenarios

Security Concerns ✅

• No Security Issues: The change maintains the same security boundary
• Proper Encapsulation: Still preserves the proxy's ability to intercept and control property access appropriately

Test Coverage Considerations ⚠️

• Missing Context: The PR lacks description and test changes
• Recommendation: Should include tests that verify:
  • Property getters work correctly on ActorConn instances
  • No infinite recursion occurs when accessing built-in properties
  • Method binding is preserved (which the fix maintains with .bind(target))

Technical Details

• Proxy Pattern: This is part of a sophisticated proxy system that allows calling actor actions directly as methods (e.g., actor.myAction() instead of actor.action({name: 'myAction'}))
• Method Binding: The subsequent .bind(target) ensures methods maintain proper this context
• Architecture: The fix preserves the proxy's dual purpose of handling both real properties and dynamic action methods

Overall Assessment ✅

This is a solid bug fix that addresses a real issue in the proxy implementation. The change is minimal, targeted, and technically correct. While the PR could benefit from more descriptive commit messages and test coverage, the code change itself is excellent.

Recommendation: Approve - this fixes a legitimate proxy receiver issue without introducing new problems.

---

[NathanFlurry]

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

• chore(core): reimpl reconnect logic #1310
• fix(core): fix getters on ActorConn proxy #1311 👈 (View in Graphite)
• chore(core): simplify connection handling & fix graceful reconnect #1309
• chore(core): clean up token configs #1298
• fix(core): clean up websocket protocol handling #1297
• chore(core): add token & optional headers to client #1296
• feat: serverless #1288
• 09-16-fix_next-js_fix_example_for_next-js #1250
• fix(core): fix missing ManagerDriver impls #1308
• main

---

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.

[graphite-app]

Merge activity

• Sep 26, 7:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 26, 7:06 AM UTC: CI is running for this pull request on a draft pull request (#1312) due to your merge queue CI optimization settings.
• Sep 26, 7:07 AM UTC: Merged by the Graphite merge queue via draft PR: #1312.