fix(py-sdk): robust target parsing + empty-response handling in Sandbox._send (closes #256)#258
Merged
Merged
Conversation
…ox._send Closes #256 (thanks @Bug-Keeper — accurate report, exact code citations). Two real defects in sdk/python/forkd/sandbox.py `_send`: 1. `host, _, port_s = self.target.rpartition(":")` mangled IPv6 targets: `[::1]:8888` → host `"[::1]"` (brackets kept), which `socket.create_connection` can't resolve. Latent today (default is `10.42.0.2:8888`) but `target` / `FORKD_TARGET` is user-configurable. Fixed with a `_split_host_port` helper using `urllib.parse.urlsplit`, which strips the brackets and raises a clear ValueError on a malformed target instead of a bare `int()` failure. 2. `return json.loads(buf.decode())` on an empty `buf` raised an opaque JSONDecodeError when the agent closed the connection without replying (crash / mid-response death). Now raises a plain "empty response" RuntimeError — matching the TS SDK's empty-body handling in controller.ts. Also turns a recv timeout into a clear TimeoutError rather than a bare socket.timeout. (The issue title says "loses data with custom timeout" — the actual defects are the two above; no data loss, but the report's substance and proposed fixes were correct.) Adds sdk/python/tests/ (the SDK had no tests — only example.py) with 12 regression cases covering IPv4/IPv6/hostname/malformed parsing and the empty-response + roundtrip wire behavior against an in-process socket server (no firecracker/daemon needed). New `sdk-python` CI job installs the package and runs them so they can't rot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #256. Thanks @Bug-Keeper — accurate report with exact code citations.
Two real defects in
sdk/python/forkd/sandbox.py_sendhost, _, port_s = self.target.rpartition(":")turns[::1]:8888into host"[::1]"(brackets retained), whichsocket.create_connectioncan't use. Latent today (default10.42.0.2:8888) buttarget/FORKD_TARGETis user-configurable. Fixed via a_split_host_porthelper onurllib.parse.urlsplit(strips brackets; raises a clearValueErroron a malformed target).JSONDecodeError.json.loads(buf.decode())on an emptybuf(agent closed without replying) gave a baffling parse error. Now a plainRuntimeError("empty response …"), matching the TS SDK's empty-body handling incontroller.ts. A recv timeout is also surfaced as a clearTimeoutError.Tests + CI
sdk/pythonhad no tests (onlyexample.py). Addssdk/python/tests/test_send.py— 12 cases over IPv4/IPv6/hostname/malformed parsing + empty-response/roundtrip wire behavior against an in-process socket server (no firecracker/daemon needed). Newsdk-pythonCI job installs the package + runs them so they don't rot.Test plan
pytest sdk/python/tests/→ 12 passed locallysdk-pythonjob)🤖 Generated with Claude Code