Skip to content

include sandbox runtime in list request#227

Merged
Nathan Selvidge (nselvidge) merged 1 commit into
mainfrom
nate/include-sandbox-runtime
Apr 8, 2026
Merged

include sandbox runtime in list request#227
Nathan Selvidge (nselvidge) merged 1 commit into
mainfrom
nate/include-sandbox-runtime

Conversation

@nselvidge
Copy link
Copy Markdown
Contributor

In order to add support for python based sandboxes we need to pass the runtime into the list request. The change is backwards compatible and the value will be stripped on out of date data plane versions.

@starfolkai
Copy link
Copy Markdown
Contributor

starfolkai Bot commented Apr 8, 2026

Failed: Failed to send input to session

@starfolkai
Copy link
Copy Markdown
Contributor

starfolkai Bot commented Apr 8, 2026

Drop into this review session: sfk devbox session 46

Code Review — PR #227: include sandbox runtime in list request

Summary: One-liner that adds runtime_context to the function/sandbox-list POST body so the backend knows the caller's Python version when discovering evaluators.


Change is correct and well-placed

runtime_context is already computed at lines 98–101 (before the sandbox-list call) and already included in every function_data bundle sent to insert-functions (line 134). This PR simply propagates the same value one step earlier — consistent format, no duplication, no new computation.

# Already used in function_data at line 134:
"runtime_context": runtime_context,  # {"runtime": "python", "version": "3.13"}

# Now also sent to sandbox-list (the new line):
"runtime_context": runtime_context,

Makes sense semantically: the list endpoint needs to know which Python version the caller is running to return compatible evaluators.


Cassettes are stale but won't break tests

The three VCR cassettes in py/src/braintrust/cassettes/ were recorded without runtime_context in the sandbox-list request body:

# cassettes/test_register_sandbox_single_evaluator.yaml, line 75–77
body: '{"sandbox_spec": {"provider": "modal", "snapshot_ref": "im-icRxmsk1Sz9XPP2f8OblVU"},
  "entrypoints": [...], "project_id": "..."}'

After this PR the actual request body will also include "runtime_context": {"runtime": "python", "version": "3.X"}. However, the vcr_config in conftest.py doesn't set match_on, so VCR uses its default matchers (method, scheme, host, port, path, query) — body is not matched. Tests will replay correctly in CI.

That said, the cassettes no longer accurately document what the real request looks like. It would be worth re-recording them so future developers can see the full request shape:

cd py
BRAINTRUST_CLAUDE_AGENT_SDK_RECORD_MODE=all nox -s test_core -- src/braintrust/test_sandbox.py

(or the equivalent --vcr-record=all flag for the core session).

This is a minor nit — not a blocker.


No new test coverage for the new field

There's no assertion that runtime_context is actually included in the sandbox-list request. Given the change is tiny and the tests exercise the full call path, this is acceptable, but if you want stronger coverage consider a unit test that captures/mocks the outgoing request and asserts the payload shape.


Verdict

Approve. The change is minimal, correct, and consistent with how runtime_context is already used elsewhere in the same function. The only follow-up worth doing (not blocking) is re-recording the VCR cassettes so they reflect the updated request body.

@nselvidge Nathan Selvidge (nselvidge) merged commit 4324364 into main Apr 8, 2026
39 checks passed
@nselvidge Nathan Selvidge (nselvidge) deleted the nate/include-sandbox-runtime branch April 8, 2026 18:48
Abhijeet Prasad (AbhiPrasad) added a commit that referenced this pull request Apr 10, 2026
@AbhiPrasad
Copy link
Copy Markdown
Member

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