Skip to content

fix: preserve oversized sandbox JSON-RPC responses - #4371

Merged
epatey merged 13 commits into
UKGovernmentBEIS:mainfrom
yinli-systems:fix-bash-session-output-limit
Jul 30, 2026
Merged

fix: preserve oversized sandbox JSON-RPC responses#4371
epatey merged 13 commits into
UKGovernmentBEIS:mainfrom
yinli-systems:fix-bash-session-output-limit

Conversation

@yinli-systems

@yinli-systems yinli-systems commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fixes #4299.

Sandbox tool responses can exceed the host exec-output limit, which truncates a JSON-RPC frame before parsing. This change preserves the complete response through a bounded continuation protocol: the sandbox spills an oversized serialized response to a per-user, permission-checked file, returns small base64 chunks, and the host validates and reassembles them before JSON-RPC parsing.

bash_session also receives the host's effective SandboxEnvironmentLimits.MAX_EXEC_OUTPUT_SIZE value and bounds each PTY read, preventing a single interactive response from growing without limit while retaining subsequent output for later interactions. The continuation protocol is enabled only for the current sandbox CLI transport, so legacy transports keep their existing behavior.

The implementation validates handles, ownership, offsets, totals, completion, base64 and UTF-8; separates concurrent transfers; and releases temporary files on success and error. A root-owned sticky shared directory remains usable by non-root sandbox users without permitting unsafe ownership or modes.

Validation

  • Sandbox-tools unit tests: 11 passed.
  • Host asyncio-focused tests: 9 passed, 9 trio variants skipped.
  • Host trio-focused tests: 9 passed, 9 asyncio variants skipped.
  • Targeted mypy: no issues in 4 source/test files.
  • Targeted Ruff formatting and lint checks passed.
  • git diff --check passed.
  • The regression suite covers large Unicode payloads, malformed/out-of-order chunks, continuation errors, cleanup, concurrent handles, legacy transport behavior, root-owned shared-directory access, and bounded bash_session output with eventual tail delivery.

AI assistance

AI tooling assisted with implementation and test iteration. I reviewed the final design and diff, traced the transport and sandbox trust boundaries, and ran the validations listed above.

@yinli-systems
yinli-systems marked this pull request as ready for review June 27, 2026 10:53
@epatey

epatey commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks for digging into this — #4299 is a real problem (an unbounded-output command shouldn't be able to take down an entire eval), and bounding the response on the sandbox side before JSON-RPC serialization is a sensible shape for the fix. The test coverage is appreciated too.

I do have a concern about how the limit is determined, though. Inspect already has an affordance for callers to adjust the max exec output size: the 10 MiB default can be raised via the INSPECT_SANDBOX_MAX_EXEC_OUTPUT_SIZE environment variable (documented in docs/sandboxing.qmd) or programmatically via override_max_exec_output_size(). Returning large output isn't an error in itself, and some users bump this limit specifically because they expect large tool output. As written, the PR undercuts that in a few ways:

  • It hard-caps bash_session responses at 1 MiB — a 10× reduction from the current effective envelope even at default settings, so output in the 2–9 MiB range that works fine today would start getting truncated.
  • The env var is only honored downward (min(parsed, 1 MiB)), so a user who raised the limit per the docs would be silently clamped.
  • The os.getenv() call runs inside the sandbox container, where the host process's environment (and the ContextVar-based overrides) aren't visible — so in most real deployments the limit would effectively be a fixed 1 MiB regardless of configuration.

Would the existing affordances help your use case? If the failures you're seeing come from output that's legitimately large but bounded, raising INSPECT_SANDBOX_MAX_EXEC_OUTPUT_SIZE may already get you there today.
For the genuinely unbounded case, we'd still want a sandbox-side cap — but it probably needs to be driven by the host's effective limit (e.g. passed as a per-call RPC param with headroom for JSON framing/escaping) rather than a constant, so the documented knobs keep working.

Happy to discuss direction — thanks again for the contribution!

@yinli-systems
yinli-systems force-pushed the fix-bash-session-output-limit branch from 42fc17b to 9ddafe8 Compare July 7, 2026 16:23
@yinli-systems

yinli-systems commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, that makes sense. I pushed an update in 9ddafe87 to address this direction.

The sandbox-side cap is no longer a fixed 1 MiB value and no longer reads INSPECT_SANDBOX_MAX_EXEC_OUTPUT_SIZE from inside the container. The host now passes the effective SandboxEnvironmentLimits.MAX_EXEC_OUTPUT_SIZE into each bash_session RPC call as max_output_bytes, so the existing env var and override_max_exec_output_size() paths continue to determine the limit. The sandbox then applies that per-call value before JSON-RPC serialization, with a small amount of response headroom for JSON framing/escaping.

I also updated the tests to cover both sides of that contract:

  • sandbox RPC test: small max_output_bytes truncates before JSON-RPC response serialization
  • sandbox RPC test: larger max_output_bytes allows bounded output without truncation
  • host-side test: override_max_exec_output_size() is reflected in the RPC params sent to the sandbox

Validation run locally:

  • uv run ruff format --check ...
  • uv run ruff check ...
  • PYTHONPATH=src uv run pytest tests/tools/test_bash_session_output_limit.py
  • PYTHONPATH=src uv run pytest tests/test_bash_session_output_limit.py from src/inspect_sandbox_tools
  • git diff --check

@epatey

epatey commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the quick turnaround. Your update does what I suggested, and the test coverage is right on.

That said, after looking at underlying failure more deeply, I don't think we should merge a payload-capping fix at all. The bug is really in the transport layer: if the sandbox provider truncates the exec stream at the output cap, the host-side size check passes and a clipped JSON line flows straight into parse_json_rpc_response(). A transport that can hand its parser a truncated frame is just broken.

A more correct fix I have in mind is chunked egress in the shared JSON-RPC transport: when a response exceeds a threshold, the sandbox-side dispatcher returns the first chunk plus a continuation handle, and the host loops fetching chunks until it has the complete frame before parsing. Every wire exchange is small, and it fixes the same latent bug in text_editor, MCP, and exec_remote too. Parts of your work still fit that picture — bounding the accumulated PTY output and passing the host's limit down as max_output_bytes are both still wanted; only the framing-headroom piece goes away.

Since the transport fix is a bigger, cross-cutting change, a practical note in the meantime: if the output you're hitting is large but bounded, the shipping code plus raising INSPECT_SANDBOX_MAX_EXEC_OUTPUT_SIZE should work around this today — it's only the genuinely unbounded case that needs the transport fix.

I want to be upfront that if I do the fuller fix, it may take a little while to get prioritized. I have no expectation that you take it on — but if it's something you'd be interested in tackling, I'd be glad to support that; either way, let me know if the workaround covers your immediate need.

@yinli-systems

yinli-systems commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, that direction makes sense. I pushed an updated transport-level approach in db0258f.

What changed:

  • oversized sandbox JSON-RPC response frames are now returned as small chunk envelopes with a continuation handle, and SandboxJSONRPCTransport fetches continuations before handing the complete frame to parse_json_rpc_response();
  • this is wired at the shared sandbox CLI transport boundary, so it applies before host-side parsing rather than capping a partially serialized payload;
  • normal JSON-RPC error responses are still passed through unchanged;
  • the bash_session max_output_bytes propagation and bounded PTY accumulation remain, but the JSON framing headroom is removed.

I also added host-side transport tests plus sandbox-side chunking coverage, including the in-process text_editor path under a low chunk threshold.

Validated locally:

  • uv run ruff check src/inspect_ai/util/_sandbox/_json_rpc_transport.py tests/util/sandbox/test_json_rpc_transport.py tests/tools/test_bash_session_output_limit.py src/inspect_sandbox_tools/src/inspect_sandbox_tools/_cli/main.py src/inspect_sandbox_tools/src/inspect_sandbox_tools/_util/json_rpc_chunking.py src/inspect_sandbox_tools/src/inspect_sandbox_tools/_remote_tools/_bash_session/_process.py src/inspect_sandbox_tools/tests/test_json_rpc_chunking.py
  • PYTHONPATH=src uv run pytest tests/util/sandbox/test_json_rpc_transport.py tests/tools/test_bash_session_output_limit.py
  • PYTHONPATH=src uv run --extra dev python -m pytest tests/test_json_rpc_chunking.py tests/test_bash_session_output_limit.py

@epatey

epatey commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks @Kevin-Li-2025. I'll take a look today.

@yinli-systems yinli-systems changed the title fix: cap bash_session output before JSON-RPC serialization fix: preserve oversized sandbox JSON-RPC responses Jul 23, 2026
@i-am-marvin i-am-marvin mentioned this pull request Jul 29, 2026
Closed
epatey
epatey requested changes Jul 30, 2026
View reviewed changes

@epatey epatey left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks again for the continued work on this, @yinli-systems and apologies for the two-week gap on my end. The transport-level chunking is exactly the shape we discussed, and the implementation and test coverage are in good shape overall.

One area the shared chunk directory doesn't handle yet: mixed-user tool calls. Tools can be exec'd with a user= argument, so within a single eval the CLI may run as different uids. Two situations need to work:

  1. A user=-scoped call creates the directory first. The directory ends up owned by that uid, and a later root-invoked call fails the owner check in ensure_json_rpc_response_chunk_dir() (uid 1000 isn't in (0, 0)). Because that check runs at the top of _exec for all requests, every subsequent root tool call fails - not just chunked ones.
  2. The path is pre-occupied. The directory lives at a fixed path in a world-writable location, so sandbox code can create something else there first (e.g. a plain file). Today that also fails all tool calls, including small responses that never touch the chunk machinery.

Related design constraint: we deliberately keep the injected tooling hidden from the model - it lives in a dot-prefixed random-hash directory precisely so an agent doesn't stumble on the harness (see the _cli.py docstring). A fixed, self-describing path like /tmp/inspect-sandbox-tools-json-rpc-chunks cuts against that. So the chunk storage should (a) live under the hidden random tools directory or a similarly random hidden path, and (b) preserve per-identity isolation - e.g. per-uid 0700 subdirectories with files owned by the requesting identity, which also sidesteps the cross-user ownership problem in scenario 1. (For remote-dispatch tools, another option is holding chunks in memory in the long-lived server; only the in-process tools genuinely need disk.)

I've pushed two failing tests to this branch that pin down both scenarios: test_chunk_dir_accepts_agent_owned_dir_when_running_as_root and test_small_response_unaffected_by_unusable_chunk_dir. They're written against the behavior rather than a specific fix, so any approach that makes them pass works.

Also a heads-up: I bumped sandbox_tools_version.txt on your behalf, since this change requires publishing new sandbox-tools binaries — I'll handle that build/publish once the PR is green.

Thanks again for sticking with this.

yinli-systems and others added 5 commits July 31, 2026 01:06
@yinli-systems
fix: isolate sandbox JSON-RPC chunk storage by uid
7673c2a 
Signed-off-by: Kevin-Li-2025 <[email protected]>
@yinli-systems
Merge upstream main into fix-bash-session-output-limit
53d2d87 
Signed-off-by: Kevin-Li-2025 <[email protected]>
@epatey
Merge branch 'main' into fix-bash-session-output-limit
b837b9b
@epatey
fix: chmod chunk dir via fd, not path
dc59cb3 
Tolerating an agent-owned chunk dir means the owning sandbox uid can
swap the entry between the ownership check and the chmod that follows;
chmod on a path follows a symlink planted there, so root could be made
to widen an arbitrary path to 1733. Check and chmod through an
O_DIRECTORY|O_NOFOLLOW descriptor instead.

Existing chunk-dir tests simulated ownership by patching lstat/chmod,
which the fd path no longer calls; repointed to os.fstat/os.fchmod so
they still pin their scenarios.
@epatey
Merge branch 'main' into fix-bash-session-output-limit
c42ea2f
@epatey
epatey enabled auto-merge (squash) July 30, 2026 21:03
@epatey
epatey merged commit 2882359 into UKGovernmentBEIS:main Jul 30, 2026
19 checks passed
@epatey epatey mentioned this pull request Jul 30, 2026
Open
@dragonstyle dragonstyle mentioned this pull request Aug 3, 2026
Merged
3 of 5 tasks
@craig-meridian craig-meridian mentioned this pull request Aug 11, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@epatey epatey epatey approved these changes

Assignees

@epatey epatey

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bash_session crashes eval with JSONRPCResponse parse error when command produces unbounded stdout

2 participants

@yinli-systems @epatey