fix: fix ReadableStream being incorrectly wrapped by BindingWrapper

[ryanking13]

This fixes a bug that the ReadableStream object returned from rpc call is incorrectly wrapped with _BindingWrapper.

In #112, I wrapped all JsProxies returned from rpc call with _BindingWrapper, so that nested rpc objects can be handled properly. However, this was too aggressive that it ended up wrapping objects such as ReadableStream with the _BindingWrapper as well.

This should not be not a problem normally because _BindingWrapper basically pass-through all the attributes and parameters. However, when we are accessing the constructor attribute, which is both an object and callable, we wrap it with a Python function and loose the access to its inner attributes, leading to the following error.

    File "introspection.py", line 88, in wrapper_func
      return python_to_rpc(await result)
                           ^^^^^^^^^^^^
    File "/session/metadata/entry.py", line 29, in fetch
      return Response(obj.body, headers={"etag": obj.httpEtag})
    File "/session/metadata/python_modules/workers/_workers.py", line 418, in __init__
      if body.constructor.name not in RESPONSE_ACCEPTED_TYPES:
         ^^^^^^^^^^^^^^^^^^^^^
  AttributeError: 'function' object has no attribute 'name'

This PR is a workaround, avoid wrapping JS objects that can directly passed to the Response object. In the long term, I think the proper fix is to create a Python wrapper for ReadableStream and handle it inside the python_from_rpc function.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR prevents _BindingWrapper from wrapping JS objects like ReadableStream that should be passed through directly (e.g. to Response).

Issues

1. (Medium) Coupling RESPONSE_ACCEPTED_TYPES to binding-wrapping logic is fragile and the helper name is misleading. _is_response_accepted_type is documented as checking "if the given object is an accepted type for a Response body," but _should_wrap_nested_attribute repurposes it to decide whether to skip _BindingWrapper wrapping. These are conceptually different decisions that happen to overlap today. If someone later adds a type to RESPONSE_ACCEPTED_TYPES (e.g. a new body type) or needs to exclude a non-body JsProxy from wrapping (e.g. a Headers object accessed via RPC), the coupling will silently break one of the two call sites. Consider either (a) giving _should_wrap_nested_attribute its own set of excluded types, or (b) renaming _is_response_accepted_type to something more general (e.g. _is_known_js_builtin) with a docstring that reflects both use cases.

2. (Low) Minor: Response.__init__ accesses body.constructor.name twice on the error path. After calling _is_response_accepted_type(body) (which already reads constructor.name), line 429 reads body.constructor.name again for the error message. Not a bug, but since constructor access can have side effects through __getattr__ on wrapped objects, you could store the result from the helper to avoid the duplicate access. This is a pre-existing issue though, not introduced by this PR.

[ask-bonk]

Review posted successfully on PR #128. Here's a summary of what I flagged:

1. (Medium) Fragile coupling between RESPONSE_ACCEPTED_TYPES and binding-wrapping logic. The _is_response_accepted_type helper is named and documented for Response body validation, but it's being reused in _should_wrap_nested_attribute to decide wrapping -- a conceptually different decision. This coupling could silently break if either set of types diverges in the future. Suggested renaming or decoupling.

2. (Low) Pre-existing: duplicate body.constructor.name access on the error path in Response.__init__. Minor, not introduced by this PR.

The fix itself is a reasonable workaround for the described bug, and the test covers the motivating case well.

github run

[dom96]

Thanks! I actually ran into this yesterday when I upgraded one of my Workers to the latest SDK. Nice to see you caught as well.