Skip to content

Add callRpcMethod to python entrypoint helper#6317

Open
ryanking13 wants to merge 2 commits intomainfrom
gyeongjae/rpc-caller
Open

Add callRpcMethod to python entrypoint helper#6317
ryanking13 wants to merge 2 commits intomainfrom
gyeongjae/rpc-caller

Conversation

@ryanking13
Copy link
Contributor

Splitted off from #6296.

Other parts of #6296 will go to workers-py.

@ryanking13 ryanking13 requested review from dom96 and hoodmane March 13, 2026 05:08
@ryanking13 ryanking13 requested review from a team as code owners March 13, 2026 05:08
Co-authored-by: ryanking13 <ryanking13@users.noreply.github.com>
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 13, 2026

Only one file changed, no tests added. Let me analyze the change:

Functional analysis:

  • callRpcMethod(binding, method, ...args) is simply binding[method](...args) -- a straightforward dynamic dispatch.
  • The comment explains the motivation clearly: Pyodide's JsProxy adds send/throw methods that shadow RPC methods with the same name.
  • This is a new addition to the entrypoint helper, purely additive, no behavioral changes to existing code.
  • No backward compatibility concern -- it's an addition.
  • No security concern -- this is calling methods on bindings the Python worker already has access to.

Potential issues to consider:

  1. No validation that method exists on binding -- if binding[method] is undefined, calling it will throw a TypeError: binding[method] is not a function. This is reasonable since the error message would be clear enough.
  2. The function is typed as returning any, which is fine given the rest of the file uses any extensively.
  3. No test is included, but the PR description says the consumer code goes to workers-py, and this function is trivial enough that integration testing from the Python side makes more sense.

This change is clean, minimal, well-commented, purely additive, and the implementation is correct for its stated purpose. There are no actionable issues.

LGTM

github run

@ask-bonk ask-bonk bot requested a review from a team as a code owner March 13, 2026 05:09
Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the SDK dependency on python-entrypoint-helper is concerning. I wonder if we have a path towards getting rid of it. At the very least we need to make sure we add protections so that we don't accidentally break it.

{
"dependencies": {
"@opencode-ai/plugin": "1.2.24"
"@opencode-ai/plugin": "1.2.25"
Copy link
Contributor

@dom96 dom96 Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be reverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, bonk is keep pushing changes to PRs. Will revert before merge.

@ryanking13
Copy link
Contributor Author

@dom96

Hmm, the SDK dependency on python-entrypoint-helper is concerning. I wonder if we have a path towards getting rid of it.

We already have a SDK dependency on python-entrypoint-helper a lot and I am not sure if we can do this without defining a helper method in JS.

At the very least we need to make sure we add protections so that we don't accidentally break it.

Yes, considering our backward compatibility policy, it would be impossible to make any breaking changes to python-entrypoint-helper at all from now on. This is applied not just to this method but to all the code in python-entrypoint-helper that we are all aware of. What kind of protection do you think we need?

@ryanking13 ryanking13 requested a review from dom96 March 16, 2026 03:28
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