Skip to content

feat(sdk): Python SDK scaffold — arcbox hello-world loop (CORE-58) - #547

Open
AprilNEA wants to merge 20 commits into
masterfrom
feat/sdk-python
Open

feat(sdk): Python SDK scaffold — arcbox hello-world loop (CORE-58)#547
AprilNEA wants to merge 20 commits into
masterfrom
feat/sdk-python

Conversation

@AprilNEA

@AprilNEA AprilNEA commented Aug 3, 2026

Copy link
Copy Markdown
Member

Phase 1 of CORE-58 for Python: the hello-world closed loop against the local daemon, at parity with the merged TypeScript SDK (#545). PyPI name arcbox is free (simple index 404s), so the locked first-choice name is used.

Scope

  • Sandbox / AsyncSandbox: create (id minted client-side, Events subscription armed before Create, waits for READY, best-effort Remove on failure), connect (resumes PAUSED, waits out STARTING/PAUSING, typed error on terminal states), auto-paginating list, info / kill / pause, with / async with disposal (swallows only NotFound).
  • commands.run overloaded on background: foreground CommandResult with exit-as-data + .expect() + check= (subprocess.run semantics); background CommandHandle with iterable output, long-polled wait_for_exit (30 s slices), process-group kill. Output re-read from offset 0 with retention-gap truncated reporting.
  • files: read_bytes / read_text / write_bytes / write_text, 256 MiB cap, mode default 0o644 with the wire's mode-0-is-default reservation documented (the TS mode-handling fix mirrored).
  • Typed error hierarchy in arcbox.errors, derived from the errors.proto registry (unknown codes stay on the base class, code preserved); coarse Connect-code routing + HTTP-status fallback; ConnectionFailedError with the abctl daemon start suggestion.
  • Env conventions: ARCBOX_SOCKET / ARCBOX_API_URL / ARCBOX_API_KEY / ARCBOX_DATA_DIR / ARCBOX_PROFILE, mirroring paths.rs and the TS SDK (option > env > default).

Transport

Hand-written Connect client over httpx (sdk/python/src/arcbox/_async/_client.py), UDS via httpx.AsyncHTTPTransport(uds=...) with the http://arcbox placeholder authority:

  • Unary: POST with content-type: application/proto, connect-protocol-version: 1; a configured request_timeout is sent as connect-timeout-ms and mirrored into the httpx deadline; non-200 bodies are Connect error JSON.
  • Server-streaming: application/connect+proto, 5-byte envelopes (1 flag byte + u32 BE length). Entering the stream context sends the request — that is what arms an Events subscription before Create. Iteration decodes envelopes incrementally (32 MiB frame sanity cap), ends deterministically on the EndStreamResponse frame (flag 0b10), whose error member maps through the registry. Compressed frames (flag 0b01) are rejected — compression is never negotiated.
  • Client-streaming (WriteFile): the request sequence is known up front, so the envelopes are sent as one body; the single response message + EndStream are decoded from the buffered response.
  • Error bodies / EndStream JSON are parsed with typed msgspec Structs; ErrorInfo details are base64-decoded (padded and unpadded) and parsed as protobuf.

Sync tree

Async core under arcbox/_async/; arcbox/_sync/ is generated by scripts/gen_sync.py (unasync + a project token map: Async* class prefixes, httpx flavors, contextlib counterparts), then normalized with the project's own ruff so the committed tree passes the gates verbatim. Lockstep is enforced twice: gen_sync.py --check (wired into pytest and the prek hooks) and a parity test asserting identical public surfaces modulo async markers. Sync streaming is a genuine blocking iterator over httpx.Client.stream — no smuggled event loop.

Toolchain

uv (uv_build, uv.lock committed), ruff (lint+format, E/F/W/I/UP/B/SIM/RUF), pyright strict (authoritative; generated _gen excluded, codegen scripts relax only unknown-type diagnostics), pytest + anyio. Evaluated: ty 0.0.65 (16 false positives on protobuf generated members — dev-dep only), pyrefly 1.2.0 (clean, informational). msgspec justified by typed validation at the one JSON seam. sdk/python/prek.yaml carries scoped local hooks (named that way because the repo gitignores .pre-commit-config.yaml).

e2e

sdk/python/tests/test_e2e.py is the ARCBOX_SDK_E2E=1-gated hello world (sync + async flavors, skipped cleanly otherwise), and tests/e2e --test sdk_py mirrors the sdk_ts harness (isolated daemon, WatchSetupStatus readiness, uv sync --frozen + uv run pytest). fmt/clippy clean. Not yet run against live hardware in this PR.

Deferred (tracked in README Status)

PTY, ports, wait_for_port / wait_for_log, stdin, filesystem path verbs, Template statics, events(), set_lifecycle, GetCapabilities handshake, the SDK-side 300 s idle-kill default (applied once the daemon enforces the lifecycle knobs), auto_resume=False header sugar, CI workflow job (token restriction — TODO in README), release-please registration (PR #546 owns that concern).

AprilNEA added 12 commits August 4, 2026 01:07
uv-managed project (uv_build backend, committed lockfile), ruff as
linter+formatter (E/F/W/I/UP/B/SIM/RUF), pyright strict, pytest.
Package name arcbox (PyPI simple index answers 404 — the name is free),
Python >= 3.10. Runtime deps: httpx (UDS transport), protobuf (wire
types), msgspec (typed Connect error-body parsing).
scripts/gen_proto.py drives the protoc bundled with grpcio-tools over
rpc/arcbox-protocol/proto/arcbox/sandbox/v1, flattens the package, and
rewrites intra-package imports to relative ones so the modules are
location-independent under arcbox._gen. Output (py + pyi) is committed
and never exported; bundled protoc 7.35.1 matches the pinned protobuf
runtime.
errors.py mirrors the TS hierarchy exactly and derives the registry
mapping from the generated ErrorCode enum; unknown codes stay on the
base class with the raw code preserved. _connection.py resolves
option > env > default (ARCBOX_SOCKET / ARCBOX_API_URL / ARCBOX_API_KEY
/ ARCBOX_DATA_DIR / ARCBOX_PROFILE) like the TS SDK and paths.rs.
_envelope.py owns the Connect streaming envelope (1 flag byte + u32
length) and the JSON error bodies, decoded with typed msgspec Structs
at the single untrusted-input boundary.
SandboxInfo/SandboxSummary/CommandResult/OutputChunk plus the proto
mapping helpers, shared by the async and sync trees. Exit is data:
command_result_from_execution turns signal death into 128+signal with
the signal name, raises SandboxDiedError when the execution ended
without an observed exit, and expect() is the opt-in raise. Python time
units are seconds (floats), mirroring the design doc's per-language
convention.
AsyncConnectClient serves the three RPC shapes the phase-1 surface
uses: unary (POST, application/proto, connect-timeout-ms mirrored into
the httpx deadline), server-streaming (AsyncServerStream — entering the
context sends the request, which is what arms a subscription before
Create; iteration decodes envelopes and ends deterministically on the
EndStreamResponse), and client-streaming with the request sequence
known up front. UDS via httpx.AsyncHTTPTransport(uds=...); injected
http_client is the mock seam. _boundary.wrap_errors is the single
transport-to-exception boundary.
Mirrors the TS SDK's phase-1 surface: AsyncArcBox.create mints the id
client-side and arms the Events subscription before Create (residual
registration window covered by an immediate Inspect plus keepalive
re-inspects), cleans up best-effort on a failed create; connect resumes
PAUSED and waits out STARTING/PAUSING; list auto-paginates.
commands.run is overloaded on background (foreground exit-as-data with
check= subprocess semantics; background handle with streamed output,
long-poll wait_for_exit in 30s slices, group kill). files does
whole-file read/write with the 256 MiB cap and the mode=0-is-default
wire reservation documented.
scripts/gen_sync.py runs unasync over arcbox/_async with the project's
extra token map (Async* class prefixes, httpx flavors, contextlib
counterparts), stamps a do-not-edit header, and normalizes the result
with the project's own ruff so the committed tree passes the repo gates
verbatim; --check regenerates into a scratch dir and fails on drift.
Streaming in the sync tree is a genuine blocking iterator over
httpx.Client.stream — no smuggled event loop. arcbox/__init__ exports
both surfaces; pyright strict passes on the generated tree.
Mirrors the TS SDK's unit suites: env-resolution precedence incl. the
ARCBOX_PROFILE daemon parity cases; registry ErrorInfo details (padded
and unpadded base64), unknown-code preservation, coarse Connect-code
routing, HTTP-status fallback; wrap_errors mapping for missing-socket
and deadline expiry; envelope roundtrip incl. byte-by-byte feeds, the
frame sanity cap, and EndStreamResponse decoding.
MockTransport suites drive the real request paths: foreground run
(output re-read from offset 0, retention-gap truncation), shell-string
sugar, check= semantics incl. the background+check runtime rejection,
background streaming and group kill on both trees; files write mode
default/explicit, empty-file done chunk, 256 KiB chunking, oversized
fail-before-send, read assembly. The parity suite reruns the unasync
transform (lockstep) and asserts identical public surfaces modulo
async markers — the sentinel default gained a stable repr so
signatures compare equal. test_e2e.py is the ARCBOX_SDK_E2E=1-gated
hello world (sync + async), skipped cleanly otherwise.
README covers install, both hello-world flavors, connection env table,
regen + sync-tree workflow, the toolchain verdicts (pyright
authoritative; ty 0.0.65 currently false-positives on protobuf
generated members; pyrefly 1.2.0 clean but informational), uv publish
flow, the maturin/PyO3 future note, and the phase-1 status/deferred
list. prek.yaml carries sdk/python-scoped local hooks (ruff, pyright,
sync lockstep, pytest) for prek or classic pre-commit — named prek.yaml
because the repo gitignores .pre-commit-config.yaml, so it is passed
with -c.
Mirrors sdk_ts: release binaries + staged dev boot assets + isolated
data dir, readiness via WatchSetupStatus, then
'uv sync --frozen' + 'uv run pytest tests/test_e2e.py' with
ARCBOX_SDK_E2E=1 and ARCBOX_SOCKET at the daemon's socket
(ARCBOX_API_URL/ARCBOX_DATA_DIR scrubbed). Wider pytest ceiling than
the TS runner: the suite boots two sandboxes (sync + async flavors).
fmt and clippy -D warnings clean.
@linear-code

linear-code Bot commented Aug 3, 2026

Copy link
Copy Markdown

CORE-58

@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

Adds the initial Python SDK and its local-daemon hello-world workflow.

  • Implements synchronous and asynchronous sandbox lifecycle, command execution, output streaming, and file operations.
  • Adds a hand-written Connect-over-httpx transport with typed protocol and daemon errors.
  • Adds generated protobuf bindings, sync-tree generation and parity checks, unit tests, and opt-in end-to-end coverage.
  • Introduces Python packaging, dependency locking, formatting, linting, and type-checking configuration.

Confidence Score: 4/5

The PR is not yet safe to merge because wait_for_exit can exceed its requested deadline during the final immediate poll.

After sleeping through the remaining sub-second budget, wait_for_exit performs another daemon request without rechecking the deadline, and that request is allowed up to five seconds before timing out.

Files Needing Attention: sdk/python/src/arcbox/_async/commands.py and generated sdk/python/src/arcbox/_sync/commands.py

Important Files Changed

Filename Overview
sdk/python/src/arcbox/_async/_client.py Implements unary, server-streaming, and client-streaming Connect transport, including required terminal-frame validation.
sdk/python/src/arcbox/_async/commands.py Implements foreground and background command handling, but the final immediate exit poll remains capable of exceeding the requested wait deadline.
sdk/python/src/arcbox/_async/sandbox.py Implements sandbox creation, connection, state transitions, listing, lifecycle operations, and disposal.
sdk/python/src/arcbox/_async/files.py Implements bounded whole-file reads and chunked writes through the streaming transport.
sdk/python/scripts/gen_sync.py Generates and verifies the blocking SDK surface from the asynchronous implementation.
tests/e2e/src/sdk_py.rs Adds the isolated-daemon harness for the opt-in Python SDK end-to-end tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    User[Python SDK caller] --> Surface{Sync or async surface}
    Surface --> Sandbox[Sandbox lifecycle]
    Surface --> Commands[Command execution]
    Surface --> Files[File operations]
    Sandbox --> Transport[Connect-over-httpx transport]
    Commands --> Transport
    Files --> Transport
    Transport --> Daemon[Local ArcBox daemon over Unix socket]
Loading

Reviews (4): Last reviewed commit: "fix(sdk): poll before sleeping in the su..." | Re-trigger Greptile

Comment thread sdk/python/src/arcbox/_async/_client.py
Comment thread sdk/python/src/arcbox/_async/commands.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb4b56d7da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/python/src/arcbox/_async/_client.py
Comment thread sdk/python/src/arcbox/_async/_client.py
@AprilNEA

AprilNEA commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Live-daemon e2e validation — PASS

cargo test -p arcbox-e2e --test sdk_py -- --ignored --nocapture at c5beab3 on Apple M5 Max / macOS 26.4 (nested virt available).

Setup (mirrors the sdk_ts validation): isolated tempdir daemon, VZ backend, boot assets 0.8.0 (the assets.lock pin, refreshed into boot-assets/dev via cargo xtask dev boot-assets), freshly rebuilt release arcbox-daemon/abctl plus musl arcbox-agent/vm-agent (the exact sandbox-smoke recipe, then SKIP_BUILD=1), readiness via WatchSetupStatus, then uv sync --frozen + uv run pytest tests/test_e2e.py with ARCBOX_SDK_E2E=1 and ARCBOX_SOCKET at the isolated socket.

Timings

phase result
daemon_ready (spawn → READY, VZ) 25.4 s
sdk_pytest (both flavors, incl. 2 sandbox boots) 8.1 s (pytest-internal 7.24 s)
harness total 101.7 s
daemon shutdown exit status 0

Assertions exercisedtest_sync_hello_world + test_async_hello_world, both PASSED:

  • files.write_textread_text roundtrip (/tmp/hello.txt)
  • foreground commands.run(["/bin/cat", ...]).expect().stdout exact match
  • non-zero exit as data: run("exit 3")exit_code == 3, no raise (sync flavor)
  • background run: iterable output stream (stdout-only filter, line3 observed), wait_for_exit(30).exit_code == 0
  • process-group kill("SIGKILL")signal == "SIGKILL", exit_code == 137 (sync flavor)
  • info().state in ("ready", "running"); disposal via kill() (sync) / async with context exit (async)

No SDK changes were needed; local gates at the validated sha: ruff check+format ✅, pyright strict 0 errors ✅, pytest 58 passed/2 skipped ✅, gen_sync.py --check lockstep ✅.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

Four issues worth fixing before merge, all reproduced with probes against this branch. Two are correctness bugs in the hand-rolled Connect layer (an error body silently loses its HTTP status; a truncated client-stream reports success), one is a hang on connect() to a PAUSING sandbox, one is a leaked HTTP stream when a caller breaks out of commands.output.

This is a strong scaffold. The parts I went looking for bugs in and did not find are worth naming, because they're the parts that are easy to get wrong:

  • Exit-as-data with .expect() / run(check=True) as the opt-in raise matches subprocess.run semantics exactly, and background=True + check=True is rejected up front rather than silently ignored.
  • Client-side id minting for both sandboxes and executions, with the Events subscription armed before Create and an immediate Inspect covering the residual registration window. That is the CORE-67 rule applied correctly, including the re-Inspect on every keepalive frame.
  • One error boundary. wrap_errors handlers are ordered ArcBoxError(ConnectError, ConnectTimeout)TimeoutExceptionHTTPError, which is the correct subclass order — reversing any pair would swallow the specific case.
  • Lockstep is enforced twice and independently: gen_sync.py --check (wired into both pytest and prek) plus a public-surface parity test. Belt and braces on the codegen seam is the right call.
  • read_bytes tolerates the empty non-final keepalive chunks filesystem.proto documents; wait_for_exit slices as min(30, max(1, ceil(remaining))) so it never accidentally sends the 0 that means "poll immediately"; and _collect_result's chunk.offset > next_* check is exactly the offset-jump gap signal process.proto specifies for retention drops.

Unit suite passes locally on 3.12: 50 passed, 2 skipped.

Notes, not asks

  • No CI job references any sdk/ path — I grepped .github/workflows/ and found none, including for the already-merged TypeScript SDK. So the README's TODO(CI) is accurate and this is a repo-wide gap rather than something this PR introduced. Flagging it only so it doesn't get lost: right now nothing runs pytest, pyright, gen_sync.py --check, or gen_proto.py --check on a PR, which means the two lockstep guards you built are currently only enforced on a developer's machine.
  • requires-python = ">=3.10" is neither type-checked nor test-matrixed — the pyright config sets no pythonVersion and .python-version pins 3.14. I checked the runtime story and it holds up (Generator[None] is under TYPE_CHECKING with postponed annotations, and there's no match, ExceptionGroup, tomllib, typing.Self, PEP 695 generic, or datetime.UTC anywhere in the tree), so this is about keeping the floor honest as the SDK grows, not a live break.
  • files.write_bytes builds the full list of chunk messages and then b"".joins the whole body, so a 256 MiB file peaks around 3× that in memory. Fine for the scaffold; worth revisiting if large-file writes become a real path.
  • tests/e2e/src/sdk_py.rs looks right — the env_remove("ARCBOX_API_URL") / env_remove("ARCBOX_DATA_DIR") hygiene is the detail that would otherwise leak a developer's daemon into the test. It's #[ignore]d and needs VZ on M3+, so I could not exercise it here. Not adding an xtask e2e prebuild arm is a valid choice per xtask/AGENTS.md (it takes the self-build fallback).

Two nits with no line to anchor to

_sync/sandbox.py:64 still reads :class:`AsyncSandbox` in the generated docstring — a gap in gen_sync.py's token map (or, equivalently, a docstring in the async source that should not name the class directly).

tests/test_sync_parity.py:96 asserts only inspect.iscoroutinefunction, which returns False for async generator functions. No async generator currently survives into the sync tree, so this is latent rather than broken — but commands.output is exactly the shape it would miss, so adding and not inspect.isasyncgenfunction(member) closes the door before it matters.


Mode: Review (initial)
Files reviewed: 49
Commits reviewed: 13
Base: master
Head: feat/sdk-python (c5beab3)
Prior pullfrog review: none

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread sdk/python/src/arcbox/_envelope.py
Comment thread sdk/python/src/arcbox/_async/_client.py
Comment thread sdk/python/src/arcbox/_async/sandbox.py Outdated
Comment thread sdk/python/src/arcbox/_async/commands.py
A WriteFile response body that ends after the response-message envelope
but before the EndStreamResponse frame (connection cut, proxy
truncation) was returned as success, hiding the server's terminal
status. Track whether the terminal frame arrived and reject EOF without
it, matching the server-streaming path's determinism.
The server slice was rounded up to a 1 s minimum, so wait_for_exit(0.1)
blocked ~1 s past its documented bound. The wire's granularity is whole
seconds, so floor the slice instead, sleep out any sub-second remainder
client-side, and finish with an immediate poll (timeout_seconds=0, the
documented poll form). The +5 s HTTP grace stays: it engages only when
the daemon is unresponsive, turning a wedged wait into a typed error.
Nothing ever closed an SDK-constructed httpx client, so its pool and
Unix-socket descriptors lived until GC. ConnectClient now records
whether it built the client and aclose()/close() releases only owned
ones — an injected Connection.http_client always belongs to the caller.
ArcBox/AsyncArcBox become context managers (plus close/aclose); the
Sandbox classmethod sugar transfers ownership of its hidden entry
point's client to the handle, which closes it on context exit, and the
list() sugar closes when iteration finishes. UP028 is ignored for the
generated sync tree only: unasync's mirror of 'async for … yield' has
no yield-from form.
Every ConnectWireError field was defaulted, so any well-formed JSON
object decoded as an empty Connect error and the HTTP-status fallback
table was reachable only for invalid JSON — a gateway's 401
{"detail": ...} produced a bare unknown-code error instead of
AuthenticationError. The spec always sends code, so make it required:
a body without it fails typed validation and falls back to the status
table, keeping one decoder at the untrusted boundary.
A PAUSING sandbox's next stop is PAUSED — never READY — and no
lifecycle event marks that edge, so routing PAUSING into the readiness
wait parked connect() forever on a keepalive-fed Events stream. Poll
the checkpoint out (0.5 s Inspect cadence), then route on the settled
state: PAUSED resumes as before, READY returns, terminal states raise.
Inherited from the TS SDK, which has the same routing (reported on the
review thread; to be fixed there separately).
Breaking out of the output iteration left the async generator — and its
open HTTP stream — suspended until the event loop's asyncgen finalizer
got around to it. commands.output now returns AsyncOutputStream /
OutputStream: still directly iterable (full consumption closes as
before), and an async context manager whose scope exit closes the
underlying stream at the break instead of at finalization. Exported,
paired in the parity test, and documented on the property.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — minor suggestions inline.

Re-reviewed the three new commits (14d09251, 68b287c9, b69a8d97) on top of my previous review of c5beab35.

What landed, and what I verified

  • client_stream terminal-frame guard (14d09251) — real fix, matches the server-streaming path's determinism, and test_write_without_the_terminal_frame_is_an_error_not_success covers it. My thread on this is retired.
  • HTTP client ownership (_owns_http / _owns_client, ArcBox/AsyncArcBox as context managers, test_disposal.py) — I traced every path: an injected client is never closed by the SDK, a handle minted from a live ArcBox correctly declines ownership, create/connect clean up the hidden box on BaseException, and there is no double-close or use-after-close. Sound, with one exception noted inline.
  • wait_for_exit sub-second slice — I checked this against the wire contract rather than my previous review's praise. WaitExecutionRequest.timeout_seconds documents 0 = return immediately — a poll, so deliberately sending 0 here is contract-correct, not the bug my earlier note implied. The new [0] assertion in test_sub_second_wait_honors_its_deadline is right; only the ordering is worth a second look (inline).
  • "asyncio": "time" token map + UP028 per-file-ignore — both necessary and correctly scoped. The only asyncio use in the async tree is asyncio.sleep, the duplicate import time is folded by the isort pass (confirmed: the committed _sync/commands.py header has exactly one), and the sync list generator's for ... yield genuinely has no async for counterpart to preserve under yield from.
  • README ownership paragraph — accurate and matches the code.

Still open from the previous review (unchanged in this range; each has a live thread):

  • _envelope.unary_error still discards the HTTP status for any valid-JSON body, because every ConnectWireError field is defaulted — _HTTP_FALLBACK_CODES is unreachable for a non-Connect JSON error body.
  • connect() on a PAUSING/PAUSED sandbox still routes into _wait_ready, which knows no PAUSING/PAUSED/RESUMED event kinds and never calls Resume. (Inherited from the TypeScript SDK — worth fixing in both.)
  • _async/commands.py::_stream_output still leaks the httpx stream on a consumer break.

Not blocking merge from my side, but approval is withheld only because those threads are unresolved.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread sdk/python/src/arcbox/_async/sandbox.py
Comment thread sdk/python/src/arcbox/_async/commands.py Outdated
@AprilNEA

AprilNEA commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Addendum — review fixes + re-validation. Seven review findings landed as six fixes (one was a duplicate already fixed): 14d0925 (client_stream requires the terminal EndStreamResponse), 68b287c (sub-second wait_for_exit bound), b69a8d9 (deterministic disposal of SDK-owned HTTP clients; ArcBox/AsyncArcBox context managers), a7e5056 (non-Connect JSON error bodies route to the HTTP fallback via required code), 78e0491 (connect() no longer waits for READY on a PAUSING sandbox), e96f889 (commands.output closes its HTTP stream at an early break via OutputStream/AsyncOutputStream). The live-daemon e2e was re-run green after each batch (latest at e96f889: daemon_ready 21 s, pytest 6.5 s, both flavors PASSED, daemon exit 0); ruff/pyright-strict/pytest (71 passed)/sync-lockstep all green.

Comment thread sdk/python/src/arcbox/_async/commands.py Outdated
An already-exited command paid the full sub-second remainder before
its state was ever read. The tail is now poll–sleep–poll: an immediate
poll returns a finished command at once; only a still-running one
sleeps out the remainder for one final poll, so an exit inside the
budget still yields a result instead of a false TimeoutError. The
bound is exceeded only by that final immediate poll's round trip,
which exit-as-data semantics make irreducible. Also states plainly in
the list() docstring that abandoning the iterator defers client
closure to generator finalization, with the ArcBox/aclosing guidance.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Re-reviewed the four commits since my last review (a7e50567, 78e04915, e96f889d, b3fb8bca). Every finding I had open is now genuinely fixed, each with a regression test that can actually fail. Approving.

Fix verification — I re-traced each one rather than taking the reply at its word

  • unary_error discarded the HTTP status for any valid-JSON body (a7e50567) — ConnectWireError.code is now required. I confirmed the mechanism holds end to end: msgspec.ValidationError subclasses msgspec.DecodeError, so the existing except msgspec.DecodeError in unary_error catches the missing-code failure and routes it into _HTTP_FALLBACK_CODES — no change was needed at the catch site, which is why the fix is one field. The fallback table has the entries the new test's probes need (401 → unauthenticated, 502 → unavailable), and test_valid_json_without_a_connect_code_also_falls_back uses exactly the two bodies from my original probe. The comment explaining why code is required (a reverse proxy's {"error": "Bad Gateway"}) is the right thing to have written down. One consequence worth knowing but not worth changing: EndStreamPayload.error shares the struct, so an EndStream frame carrying {"error": {}} now reports "malformed EndStreamResponse" instead of an empty error — strictly better, and no real peer sends that.

  • connect() hung forever on a PAUSING/PAUSED sandbox (78e04915) — PAUSING is out of the _wait_ready branch and is now polled out with a 0.5 s Inspect loop before routing. I checked the premise the fix rests on instead of assuming it: guest/arcbox-agent/src/sandbox/convert.rs::event_kind() maps only created/ready/running/idle/stopping/stopped/failed/removed — PAUSING/PAUSED/RESUMED are declared in the proto but never published. So there is no event to wait on and polling is the only correct mechanism, exactly as the code comment claims. test_connect_settles_a_pausing_sandbox_then_resumes pins both halves (a second Inspect ran; exactly one Resume).

  • _stream_output leaked the httpx stream on a consumer break (e96f889d) — fixed structurally with AsyncOutputStream/OutputStream rather than a docstring. I verified the unwind path: aclose() throws GeneratorExit, which is a BaseException, and _boundary.wrap_errors catches only ArcBoxErrorhttpx.ConnectError/ConnectTimeoutTimeoutExceptionHTTPError — never BaseException — so GeneratorExit passes straight through and async with self._attach() exits cleanly instead of being converted into an ArcBoxError. Adding the pair to test_sync_parity.py::PAIRS means the surface can't drift.

  • AsyncSandbox.list's close guarantee and the wait_for_exit sub-second latency floor (b3fb8bca) — the docstring now promises what async generators can actually deliver, and the wait path landed as poll–sleep–poll. Your amendment to my proposal is the better shape and I was wrong to omit it: literal poll-then-sleep-then-raise would report a spurious TimeoutError for a process that exits during the remainder, trading exit-as-data for a false failure. The three tests bracket it properly — already-exited returns on one timeout_seconds=0 poll with no sleep, an in-budget exit is caught by the final poll, and the deadline test asserts both polls. I read MockDaemon.wait_states to confirm these can fail rather than pass vacuously: it pops one state per WaitExecution and only then serves EXITED, so [RUNNING] * 2 genuinely forces the timeout path and a single [RUNNING] genuinely forces the in-budget-exit path. Nice detail in the corrected list docstring: scoping the contextlib.aclosing hint to "in the async flavor" keeps it accurate after the unasync pass, since the transform rewrites aclosingclosing in code but leaves docstrings alone.

Not raised as findings (verified, deliberately not new threads)

  • The generated _sync/commands.py:105 docstring still reads :class:`AsyncOutputStream` — a third instance of the known unasync gap (it rewrites code tokens, not string contents), alongside the pre-existing _sync/sandbox.py:69 and :384. gen_sync.py's EXTRA_REPLACEMENTS already has the right entry; unasync just doesn't apply it to literals. I flagged this class once before, so I'm not re-opening it per instance — but if you ever want it gone, a post-pass over the generated docstrings would close all three at once.
  • The README's for chunk in job.output: example needs no update: it consumes the stream to completion, which was always deterministic. Only the early-break idiom needed the context manager, and that is now documented on the class itself.

Caveat on how I verified: this runner has no uv/pytest/msgspec, so I confirmed the new tests by reading them and tracing the mock daemon's behavior, not by executing the suite. The wire-contract and unwind-semantics claims above are grounded in the proto and in _boundary.py, not in a test run.

Pullfrog  | View workflow run | Using Claude Opus𝕏

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.

1 participant