Skip to content

Add Pyodide sandbox code interpreter MCP server - #22

Open
Ali Keramati (alikera) wants to merge 21 commits into
mainfrom
alikera-port-sandbox-code-interpreter-pyodide
Open

Add Pyodide sandbox code interpreter MCP server#22
Ali Keramati (alikera) wants to merge 21 commits into
mainfrom
alikera-port-sandbox-code-interpreter-pyodide

Conversation

@alikera

@alikera Ali Keramati (alikera) commented Aug 27, 2026

Copy link
Copy Markdown

Adds a pyodide-based code interpreter, originally written by Susana Palmaz (@susana-palmaz): code_interpreter, list_sandbox_files and search_sandbox_files.

Security

Pyodide isn't an isolation boundary. Agent code can reach the Node host: host files, process execution, env vars. docs/sandbox_code_interpreter.md sets out the threat model.

code_interpreter now refuses to run unless THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 is set by whatever launches the server. It's deliberately not in servers.yaml, so the unsafe mode isn't the default. The file tools still work without it.

Trusted code only, and no secrets in that environment. Real confinement needs a container and isn't in this PR.

Testing

gpt-4o ran the revenue test end to end and passed: it wrote pandas against a CSV in the workspace, carried the DataFrame across two calls, and got every regional total right to the cent.

Separately drove the tool directly against support/tau_bench/airline_data (5.2 MB of flights, reservations and users). Aggregations matched host CPython exactly, and a file rewritten from inside the session came back byte-identical. Also round-tripped the document and plotting libraries it advertises.

CI: 152 passed, 2 skipped, 6 xfailed. The xfails probe the escapes above and should start passing once the worker is confined.

Fixed while porting

  • Output over 64 KB broke the worker (asyncio's default line limit); a mid-sized DataFrame hits it
  • Malformed frames desynced the stream, so the next call returned the previous call's result
  • search_sandbox_files crashed on patterns like '', ***, /etc/passwd
  • Vendored wheels weren't checked against PyPI's sha256

Copilot AI lite review requested due to automatic review settings August 27, 2026 01:34
Comment thread servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py Fixed

Copilot AI 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.

Pull request overview

Ports a Pyodide-based sandbox/code-interpreter MCP server into this repo, including a Node.js worker that hosts Pyodide, Python-side orchestration via FastMCP, workspace copy-on-write isolation, and a full integration test suite/documentation.

Changes:

  • Adds a new sandbox MCP server (mcp_sandbox.py) backed by a long-lived Node/Pyodide worker with a JSON-over-stdio protocol.
  • Implements workspace file listing/search tools plus copy-on-write isolation via NODEFS hooks and a per-session seeded workspace copy.
  • Introduces Node worker packaging/CI wiring (npm install, vendored wheels postinstall) and extensive pytest coverage + docs.

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs NPM postinstall script to vendor pure-Python wheels from PyPI into wheels/.
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py Host-side workspace file listing/search helpers with traversal protections.
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs Declares PyPI-only packages to preinstall via micropip (vendored when possible).
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs Node worker: loads Pyodide, installs packages, mounts /workspace, applies COW protections, executes code.
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package.json Declares pyodide dependency and postinstall wheel-vendoring hook.
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json Locks Node dependencies for the worker.
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py Python supervisor for the Node worker (spawn, handshake, timeouts, serialization, effects log).
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/init.py Ensures setuptools package discovery of the sandbox subpackage.
servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py FastMCP server defining sandbox tools + session lifecycle and workspace seeding.
servers/thinkingbox_tools/tests/test_sandbox_server.py Integration tests for tool discovery, file ops, isolation/COW behavior, and interpreter semantics.
servers/thinkingbox_tools/pyproject.toml Packages sandbox worker assets as Python package data.
servers/servers.yaml Registers the new sandbox server entry.
docs/sandbox_code_interpreter.md Architecture/design documentation and operational guidance for the sandbox server.
.gitignore Ignores node_modules/ for the new Node worker.
.github/workflows/test-thinkingbox-tools.yml Adds Node setup + npm ci for the sandbox worker prior to pytest.
Files not reviewed (1)
  • servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json: Generated file
Suppressed comments (1)

servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs:56

  • This wheel download fetch() also has no timeout, so a stuck transfer can hang npm install/npm ci. Adding a timeout here keeps postinstall bounded and matches the intended “best-effort vendoring” behavior.
    console.log(`[download-wheels] Downloading: ${wheel.filename}`);
    const wRes = await fetch(wheel.url);
    if (!wRes.ok) {
        console.warn(`[download-wheels] Download failed (${wRes.status}) for ${wheel.url} — skipping`);
        return;
    }
    await writeFile(dest, Buffer.from(await wRes.arrayBuffer()));

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py Outdated
Comment thread servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py
@alikera

Ali Keramati (alikera) commented Aug 27, 2026 via email

Copy link
Copy Markdown
Author

@github-actions

Copy link
Copy Markdown

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

The following issues were found:
  • ❌ 1 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 1 package(s) with unknown licenses.
See the Details below.

Vulnerabilities

servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json

NameVersionVulnerabilitySeverity
ws8.19.0ws: Memory exhaustion DoS from tiny fragments and data chunkshigh
Only included vulnerabilities with severity high or higher.

License Issues

.github/workflows/test-thinkingbox-tools.yml

PackageVersionLicenseIssue Type
actions/setup-node7.*.*NullUnknown License
Denied Licenses: GPL-2.0, GPL-3.0, AGPL-3.0

OpenSSF Scorecard

PackageVersionScoreDetails
actions/actions/setup-node 7.*.* 🟢 6.4
Details
CheckScoreReason
Code-Review🟢 10all changesets reviewed
Maintained🟢 1021 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Binary-Artifacts🟢 9binaries present in source code
Packaging⚠️ -1packaging workflow not detected
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
Pinned-Dependencies🟢 5dependency not pinned by hash detected -- score normalized to 5
License🟢 10license file detected
Fuzzing⚠️ 0project is not fuzzed
Signed-Releases⚠️ -1no releases found
Security-Policy🟢 9security policy file detected
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
SAST🟢 10SAST tool is run on all commits
npm/ws 8.19.0 🟢 5.5
Details
CheckScoreReason
Packaging⚠️ -1packaging workflow not detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Code-Review⚠️ 0Found 1/29 approved changesets -- score normalized to 0
Maintained🟢 1018 commit(s) and 4 issue activity found in the last 90 days -- score normalized to 10
Binary-Artifacts🟢 10no binaries found in the repo
Security-Policy🟢 10security policy file detected
Pinned-Dependencies⚠️ 0dependency not pinned by hash detected -- score normalized to 0
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Fuzzing⚠️ 0project is not fuzzed
License🟢 10license file detected
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
Signed-Releases⚠️ -1no releases found
SAST⚠️ 0SAST tool is not run on all commits -- score normalized to 0
npm/@types/emscripten 1.41.5 🟢 6.6
Details
CheckScoreReason
Code-Review🟢 9Found 29/30 approved changesets -- score normalized to 9
Packaging⚠️ -1packaging workflow not detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Maintained🟢 1030 commit(s) and 1 issue activity found in the last 90 days -- score normalized to 10
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
Security-Policy🟢 10security policy file detected
License🟢 9license file detected
Signed-Releases⚠️ -1no releases found
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: some github tokens can't read classic branch protection rules: https://github.com/ossf/scorecard-action/blob/main/docs/authentication/fine-grained-auth-token.md
SAST⚠️ 0SAST tool is not run on all commits -- score normalized to 0
Pinned-Dependencies🟢 8dependency not pinned by hash detected -- score normalized to 8
Binary-Artifacts🟢 10no binaries found in the repo
Fuzzing⚠️ 0project is not fuzzed
npm/pyodide 0.29.4 🟢 5.6
Details
CheckScoreReason
Maintained🟢 1030 commit(s) and 22 issue activity found in the last 90 days -- score normalized to 10
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Code-Review⚠️ 2Found 6/24 approved changesets -- score normalized to 2
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Security-Policy⚠️ 0security policy file not detected
License🟢 10license file detected
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: some github tokens can't read classic branch protection rules: https://github.com/ossf/scorecard-action/blob/main/docs/authentication/fine-grained-auth-token.md
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Signed-Releases⚠️ 0Project has not signed or included provenance with any releases.
Binary-Artifacts🟢 7binaries present in source code
Packaging🟢 10packaging workflow detected
Fuzzing⚠️ 0project is not fuzzed
SAST🟢 3SAST tool is not run on all commits -- score normalized to 3
Pinned-Dependencies🟢 6dependency not pinned by hash detected -- score normalized to 6

Scanned Files

  • .github/workflows/test-thinkingbox-tools.yml
  • servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json

@alikera

Copy link
Copy Markdown
Author

Security review: the Pyodide worker is not an isolation boundary

I audited the escape vectors raised in review. They reproduce. Posting findings and a proposed design before making broad changes, as requested.

Audit results

Measured against the pyodide@0.29.4 install this PR pins, loading Pyodide exactly as pyodide_worker.mjs does. Each probe measured reachability only — nothing destructive was executed, and the file-read probe used a sentinel file the audit created itself, not a real system file.

Capability Result
import jsjs.process reachable (JsProxy)
pyodide_js._api exposed to Python reachable
_api.loadBinaryFile exposed reachable
Host file read outside session dir via _api.loadBinaryFile READ_OK
JsProxy.constructor.constructor (Function ctor) reachable
process visible via Function ctor process-visible
process.env readable 76 variables
node:fs via dynamic import() FS_MODULE_OK
node:child_process via dynamic import() CHILD_PROCESS_OK

So agent-supplied Python has host filesystem read/write, process execution, and full environment-variable access. The NODEFS copy-on-write layer is irrelevant to this: it guards /workspace, and these vectors bypass /workspace entirely.

This is consistent with Pyodide's own documented position — it provides memory safety via WASM, but deliberately exposes a Python↔JS FFI and is explicitly not a security sandbox for untrusted code.

The documentation is currently wrong

docs/sandbox_code_interpreter.md states:

The WASM runtime is the trust boundary: user code cannot reach the host filesystem, network, or processes except through the FS bridges we explicitly expose.

That is false as written, and it's the most dangerous thing in this PR — it would lead a reader to route untrusted input here. I'm correcting the threat model in this PR regardless of what we decide about isolation.

Why in-process hardening cannot fix this

Worth stating explicitly, because these are the obvious first attempts:

  • jsglobals: {} — does not help. Function bodies are evaluated in the global scope, not the restricted object. Any reachable JS function gives .constructor.constructor, and Function("return process")() returns the real process regardless.
  • Deleting/unregistering modules — does not help. Pyodide's internals capture live JsProxy references at load time; those survive later deletion of the module bindings.
  • Node's permission model (--permission) — useful defense-in-depth, insufficient alone. It is still experimental, does not gate process.env reads, and is not a substitute for an OS boundary.

The common failure is that all three try to remove names while leaving live references reachable inside the same process and privilege domain.

Proposed design: move the boundary to the OS

Run the worker under an owner-approved OS/container boundary, so escaping Pyodide only lands the attacker in an already-empty jail.

Preferred — containerized worker:

  • --network=none (no network)
  • read-only root filesystem; --tmpfs /tmp
  • only the session directory bind-mounted read-write; no other host path
  • --cap-drop=ALL, --security-opt=no-new-privileges, non-root user
  • explicitly empty environment (no inherited secrets) — currently 76 vars are visible
  • --pids-limit, memory and CPU limits (also gives us a real answer for runaway code, which today only has a wall-clock timeout)

Fallback where a container runtime is unavailable: fail closed. CodeInterpreter._start() should refuse to launch and return a clear error rather than silently running unconfined, with an explicit opt-out env var for trusted local development.

Defense-in-depth (not load-bearing): Node permission flags, dropping _api bindings, jsglobals restriction.

Affected files if we proceed

File Change
toolslib/sandbox/code_interpreter.py spawn via container runtime; fail closed when absent
toolslib/sandbox/sandbox.Dockerfile new — pinned Node + pyodide + vendored wheels
mcp_sandbox.py session-dir mount wiring
pyodide_worker.mjs defense-in-depth hardening
.github/workflows/test-thinkingbox-tools.yml build/pull image before tests
tests/test_sandbox_isolation.py new — capability regression tests
docs/sandbox_code_interpreter.md threat model rewrite

That is a rewrite of the execution model and a CI-shape change, so I am not doing it unilaterally in this port. It needs an owner decision on runtime (Docker vs Podman vs rootless), base image, and whether eval throughput can absorb per-session container startup.

What I am changing in this PR now (bounded)

  1. Correct the threat model in the docs and the code_interpreter tool description — remove the false "cannot reach the host" claims, state plainly that this is not a security boundary and must only run trusted//first-party agent code until isolation lands.
  2. Fix the workspace link issue — reject symlinks/junctions/reparse points whose target escapes workspace_dir; materialize in-workspace links as real copies.
  3. Add regression tests — link-handling tests that fail against the unfixed code, plus capability tests marked xfail(strict=True) so they are recorded as known-broken and will fail loudly the moment isolation makes them pass, forcing the marker to be removed.

Keeping this as a draft.

Comment thread servers/thinkingbox_tools/tests/test_sandbox_isolation.py Fixed
Comment thread servers/thinkingbox_tools/tests/test_sandbox_isolation.py Fixed
Comment thread servers/thinkingbox_tools/tests/test_sandbox_isolation.py Fixed
Comment thread servers/thinkingbox_tools/tests/test_sandbox_isolation.py Fixed
@alikera

Copy link
Copy Markdown
Author

Note on the Dependency Review "unknown license" notice

The Dependency Review bot comment on this PR flags:

We could not detect a license for the following dependencies:
.github/workflows/test-thinkingbox-tools.yml » actions/setup-node@7.*.*

This is a false positive and is not failing the check. actions/setup-node is MIT-licensed (GitHub's own license API returns spdx_id: MIT with a LICENSE file present). The action cannot resolve licenses for GitHub Actions referenced by floating major tag, so it reports "unknown" rather than "missing".

From the same run:

  • Vulnerabilitiesdid not detect any vulnerable packages with severity level "high" or higher
  • Denieddid not detect any denied packages
  • The gate is deny-licenses: GPL-2.0, GPL-3.0, AGPL-3.0; unknown is not denied
  • The review check reports pass

I deliberately did not silence it by pinning to a commit SHA. Every action in this repo uses a floating major tag — actions/checkout@v7, setup-python@v7, setup-uv@v7, codeql-action/*@v4, dependency-review-action@v5 — so setup-node@v7 follows the existing convention, and this notice would appear for any action added to any workflow here. If SHA-pinning is wanted, it should be done for all actions in a separate PR rather than for one action in this one.

Separately, the same bot comment carries a deprecation warning for the deny-licenses option. That lives in .github/workflows/dep-review.yml, which this PR does not touch — worth tracking separately.

Current status on fbd09bb: all six checks pass with zero annotations, no open code-scanning alerts, and no unresolved review threads. CodeQL reports "No new alerts in code changed by this pull request."

@alikera

Ali Keramati (alikera) commented Aug 28, 2026

Copy link
Copy Markdown
Author

One thing worth deciding before this merges: the interpreter refusing to start without THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 wasn't part of the original ask, and it does mean eval runs need that variable exported.

Happy to drop it (one commit) and keep just the port, the bug fixes, and the docs correction, if the lighter version is preferred.

Comment thread servers/thinkingbox_tools/tests/test_sandbox_isolation.py Fixed
Comment thread servers/thinkingbox_tools/tests/test_sandbox_isolation.py Fixed
@alikera
Ali Keramati (alikera) marked this pull request as ready for review August 31, 2026 07:53
@alikera
Ali Keramati (alikera) requested a review from a team August 31, 2026 07:53
Ali Keramati (alikera) and others added 15 commits August 31, 2026 09:24
Ports the pyodide-based Code Interpreter / sandbox MCP server from an
internal implementation by Susana Palmaz.

The server runs agent-supplied Python inside Pyodide (CPython-in-WASM) hosted
by a long-lived Node worker, exposes workspace files at /workspace/, and
isolates writes with copy-on-write at the NODEFS layer.

Adapted for this public repo:
- Added the MIT license header required by .pre-commit-config.yaml to every
  ported .py file.
- Added toolslib/sandbox/__init__.py so setuptools find_packages() discovers
  the subpackage (the original tree relied on implicit namespace packages).
- Rewrote absolute doc links to repo-relative paths.
- Added a [tool.setuptools.package-data] section carrying only the sandbox
  worker assets; no new runtime dependencies were needed.
- Pinned actions/setup-node@v4 in CI rather than relying on the runner's
  preinstalled Node.

Fixes made while porting:
- download-wheels.mjs: a rejected fetch() (offline/blocked host/TLS error)
  escaped Promise.all and failed `npm install` outright, contradicting the
  documented "falls back to runtime fetch" behavior. Now caught per package.
- sandbox.py list_files(): os.walk() descends from the resolved base, so
  relative_to(self.workspace_dir) raised ValueError whenever the workspace
  path was aliased (symlinked dir, macOS /tmp, Windows 8.3 short path).
  Now relative to the same resolved root.
- sandbox.py: return "/" separated paths, as the tool contract and the
  /workspace/<path> concatenation require.
- package-lock.json: npm audit fix for transitive ws (8.19.0 -> 8.21.3),
  clearing 2 high advisories. package.json and pyodide are unchanged.

Co-authored-by: Susana Palmaz Lopez-Pelaez <susanapa@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
v4 targets the deprecated Node.js 20 runtime and CI emitted a deprecation
annotation. v7 also matches how this repo pins actions/checkout@v7 and
astral-sh/setup-uv@v7.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
package-lock.json: revert the ws 8.19.0 -> 8.21.3 bump and restore the
upstream lockfile byte-for-byte. My `npm audit fix` had been resolved
through a corporate npm proxy, which rewrote ws's `resolved` URL to an
internal Azure Artifacts host and downgraded its `integrity` from sha512
to sha1. Both are unacceptable in a public repo: the URL leaks an internal
endpoint and may be unreachable for outside contributors, and the sha1
value is a supply-chain integrity downgrade.

The proxy strips `dist.integrity` and rewrites tarball URLs, so a correct
public lockfile entry cannot be produced from this network. Reverting is
preferable to committing an integrity hash that cannot be verified against
the public registry. This restores the two `ws` advisories; ws is a
transitive dependency of pyodide (`ws: ^8.5.0`) that the sandbox worker
never uses, and the bump should come from Dependabot or a pyodide upgrade
resolved against registry.npmjs.org.

Also from review feedback:
- Drop unused imports `json` (mcp_sandbox) and `field` (code_interpreter);
  both were flagged by CodeQL.
- Explain the bare `except Exception: pass` in CodeInterpreter._kill,
  flagged by CodeQL as an empty except clause.
- Bound both fetch() calls in the npm postinstall script with
  AbortSignal.timeout, so a stalled connection fails fast instead of
  hanging `npm install` indefinitely.

Co-authored-by: Susana Palmaz Lopez-Pelaez <susanapa@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Dependency Review runs with `fail-on-severity: high`, so the upstream
lockfile's ws@8.19.0 is a hard blocker (GHSA-96hv-2xvq-fx4p). The previous
commit had reverted to it after my `npm audit fix` was found to have written
an internal Azure Artifacts URL and a sha1 integrity, so neither version was
mergeable.

This pins ws 8.21.3 with the correct public URL and a genuine sha512.

The integrity was derived without access to registry.npmjs.org (blocked from
my machine) and then independently verified, rather than copied from the
corporate proxy, which strips `dist.integrity`:

1. Downloaded ws@8.19.0 through the proxy and computed its sha512. It matched
   the upstream lockfile's known-good value exactly, establishing that the
   proxy mirrors the public registry byte-for-byte.
2. Computed the sha512 of ws@8.21.3 the same way.
3. `npm ci` validates the tarball against this hash and succeeds, and `npm
   audit` now reports 0 vulnerabilities.

CI re-verifies against the real registry.npmjs.org, so a wrong hash would
fail the build rather than pass silently.

Only version/resolved/integrity change; ws 8.21.3 carries the same license,
engines and peerDependencies metadata as 8.19.0, and pyodide's `ws: ^8.5.0`
range is unaffected.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Security review found that agent-supplied Python can escape Pyodide into the
Node host. Verified against the pinned pyodide 0.29.4, through the real
code_interpreter tool: `import js` exposes `process`; `pyodide_js._api` is
reachable and its `loadBinaryFile` reads host files outside the session
directory; any JsProxy yields `.constructor.constructor` (the Function
constructor), from which `node:fs` and `node:child_process` resolve via
dynamic import(); and all 76 parent environment variables are readable.

In-process mitigations do not close this. `jsglobals: {}` fails because
Function bodies evaluate in global scope, not the restricted object.
Unregistering modules fails because Pyodide internals hold live JsProxy
references captured at load time. Node's --permission model is experimental,
does not gate process.env, and is not a substitute for an OS boundary.
Closing this requires confining the worker in an owner-approved OS/container
boundary; that design is proposed on the PR and is deliberately not
implemented here, as it rewrites the execution model and the CI shape.

Threat model corrected. The docs previously claimed "user code cannot reach
the host filesystem, network, or processes", which is false. The docs now
carry an explicit warning and a Threat model section, the "run untrusted
Python safely" requirement is removed, and the code_interpreter tool
description no longer advertises itself as sandboxed. mcp_sandbox carries a
module-level warning that only trusted first-party agent code may run here.

Workspace link handling fixed. __reserved__init previously re-pointed links
found in the source workspace, and NODEFS follows host links transparently,
so a link escaping workspace_dir was readable from inside /workspace. Now:
links resolving inside the workspace are materialized as real copies, links
resolving outside are rejected, and linked directories are rejected outright
to avoid traversal and copytree recursion loops. Detection uses the Windows
reparse-point attribute in addition to S_ISLNK, because os.path.islink()
returns False for junctions -- which can be created without elevation -- and
fails closed on entries that cannot be lstat'd.

Adds tests/test_sandbox_isolation.py:
- Link-handling regression tests. Verified to fail against the pre-fix code:
  the junction case seeded `junc/hidden.txt` from outside the workspace.
- A host capability audit marked xfail(strict=True). These fail today by
  design, recording the gap in the suite rather than omitting it; strict
  means they will XPASS and fail the build once confinement lands, forcing
  the markers to be removed. The probes measure reachability only -- they use
  a sentinel file the test creates, never a real system file, and never
  execute a command or open a socket.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
None of this is isolation. Pyodide remains reachable from agent code (see
docs "Threat model"); these changes narrow the blast radius and stop the
unconfined default, they do not create a boundary.

Fail closed. CodeInterpreter._start now refuses to spawn a worker unless
THINKINGBOX_SANDBOX_ALLOW_UNCONFINED is set, so running agent code with the
MCP server's privileges is a deliberate, auditable choice instead of the
default. A documentation warning is not a control. servers.yaml sets the
variable explicitly so the decision is visible in configuration, and the test
suites opt in through an autouse fixture rather than the product defaulting
to permissive.

Allowlisted worker environment. The worker was inheriting the parent
environment wholesale (76 variables were readable from agent code). It is now
spawned with PATH plus a few operational variables. Defense-in-depth: an
escape no longer automatically yields exported secrets, but it is still an
escape.

Reparse detection corrected. The previous check treated *any* reparse point as
a link, which would have rejected OneDrive / Files On-Demand placeholders and
so any OneDrive-backed workspace. Detection now follows the Win32
IsReparseTagNameSurrogate rule (bit 29): junctions and symlinks name another
location and are traversal risks; cloud placeholders, dedup and WIM/container
mappings are the same file with different backing storage and are ordinary
files. Unreadable entries still fail closed.

Capability tests reworked. They were blanket xfail(strict=True), which would
have absorbed a broken harness as an expected failure. Each probe now
distinguishes three outcomes: a probe that cannot run fails the suite loudly,
a confirmed reachable capability xfails at runtime with a per-capability
reason, and an absent capability simply passes -- so no markers need removing
once the worker is confined. Verified by fault injection: a malformed probe
result produces FAILED, not XFAIL. node:fs and node:child_process are now
tested separately rather than as one combined probe.

Adds tests for the fail-closed gate, the environment allowlist, name-surrogate
semantics across six real reparse tags, and a simulated cloud placeholder.
Removes the unused fixture unpacking flagged by CodeQL by yielding a namespace
instead of a tuple.

Windows is documented as unsupported rather than claimed: micropip mishandles
the file:///C:/... URLs used for vendored wheels, so the worker does not start
there without a workaround. The reparse-point logic is unit-tested, but there
is no Windows CI job, so Windows should not be treated as supported.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CodeQL flagged _capability_probe for mixing explicit returns with a fall-
through path, because it cannot infer that pytest.fail() does not return.
Restructured so the function has a single explicit return.

Behavior is unchanged and re-verified: a probe that cannot run still fails
the suite loudly rather than being absorbed as an expected failure.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Six corrections from review, all of which were defects introduced by the
previous hardening commit rather than pre-existing issues.

servers.yaml no longer sets THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1. That file
is the documented normal startup path, so shipping the opt-in in it made the
unsafe mode the default and defeated the fail-closed gate entirely. Operators
must now export it in the environment that launches the server. Verified both
ways: without it the interpreter refuses to start, with it execution proceeds.

Probes no longer swallow their own failures. One caught bare `Exception`, so
any error at all -- a dead worker, a typo, a loader fault -- was recorded as
'ABSENT' and reported as confinement. The dynamic imports also used
`.catch(() => 'ABSENT')`, which did the same for any promise rejection. Probes
now catch only the specific errors a confining policy would raise
(ImportError, AttributeError, PermissionError); anything else propagates and
fails the test.

The environment probe was unfalsifiable. It asserted `len(process.env) == 0`,
but the worker is deliberately given PATH, so the condition could never hold
even under perfect confinement. It now exports a uniquely-named secret into
the parent *before* the worker starts and checks whether that specific value
is visible. This test passes today: the allowlist does keep the parent's
secrets out of the worker, which the previous formulation could not show.

Filesystem and process probes now assert on effects rather than on whether a
module name resolves. The fs probe performs a test-owned write into a pytest
temp directory and the host is then checked for the file; the process probe
runs a node one-liner whose only effect is creating a marker file in a pytest
temp directory. Both assert the host-side effect matches the reported outcome,
so a probe that lies is caught. Nothing outside the temp directories is
touched and no network is used.

TMPDIR/TEMP/TMP now match their comment. The code claimed to give the worker
its own temp root while actually copying the parent's values. CodeInterpreter
now creates a directory it owns, points the worker at it, and removes it on
close; when no directory is supplied the variables are omitted entirely.

Stale claims removed: the docs said the full parent environment is readable
(it is allowlisted now, though process.env itself still is), and the test
module still described blanket xfail(strict=True) after that approach was
replaced.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two corrections to how the constraint is worded, in all five places it
appeared (docs, servers.yaml, the module comment, and the fail-closed error).

"Trusted, first-party agent code" conflated provenance with trust. An agent
developed in-house is not a trusted *code source*: it can be induced to emit
hostile code by attacker-controlled input. That is concrete here rather than
theoretical, because this server exists to read workspace documents with
pandas/openpyxl/pypdf, so a malicious spreadsheet or PDF is an input to the
agent that writes the code. The wording now says trusted code only and calls
out that first-party provenance is not itself trust.

The secrets constraint was also justified with reasoning that the environment
allowlist has since made obsolete: it said to keep secrets out of the server's
environment "because the worker is spawned from it". The worker no longer
inherits that environment, and a passing test asserts a parent secret is not
visible. The real requirement is broader, so it is now stated as a secret-free
execution context: an escape runs with this user's OS privileges and can read
credentials from disk, which withholding environment variables does nothing
about. The allowlist is described as reducing exposure, not as a boundary.

No behavior change; wording and comments only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The port added a server that nothing in this repo used. All ten existing
scenarios target the airline, banking and email servers, so `code_interpreter`
had no scenario and was effectively dead code here. The 139 unit tests drive
the server directly through fastmcp.Client with a tmp_path built in Python,
which never exercised the path an eval actually takes: servers.yaml ->
world_state -> the scenario's declared tools.

Adds dataset/scenario/sandbox_code_interpreter.yaml, wired the same way as the
tau_bench scenarios ($THINKINGBOX_DATA expansion, which mcp_sandbox already
supports via os.path.expandvars), plus a small workspace fixture under
support/sandbox_workspace/ and test cases asserting on the code_execution
effects rather than on prose alone.

The fixture has known ground truth (revenue = units * unit_price, summed per
region: East 7312.50, North 5297.35, South 4923.50, West 4032.00), so a test
can distinguish a computed answer from an invented one.

Verified end to end against the real server before committing: world_state
passes through __reserved__init verbatim, $THINKINGBOX_DATA resolves to the
fixture, all three declared tools exist and respond, pandas returns the correct
ground truth, and -- importantly -- an agent write to reports/sales.csv
succeeds inside the session while leaving the committed fixture byte-identical.
That last check exercises the NODEFS copy-on-write layer against real
repository files rather than a synthetic temp directory; a regression there
would surface as a dirty working tree in CI.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…grity

Findings from three independent reviews, each reproduced before fixing.

Output larger than 64 KiB broke the interpreter. create_subprocess_exec was
called without `limit=`, so asyncio's 64 KiB StreamReader default applied to a
protocol that puts one JSON frame per line. Reproduced: a 100 KB frame raises
"ValueError: Separator is found, but chunk is longer than limit" and a 500 KB
frame raises the not-found variant, while both read cleanly at a higher limit.
This is not an edge case for a data-analysis tool -- printing a moderately
sized DataFrame exceeds 64 KiB -- and the ValueError was not caught, so it
surfaced as a generic "Internal error" while leaving the worker alive with an
unread partial frame. STREAM_LIMIT is now 64 MiB and over-limit frames reset
the worker with an explanatory message.

Malformed frames desynchronized every later call. `json.loads` on the response
line was unguarded, so a non-protocol line raised while the real response
stayed buffered; the next execute() then consumed that stale frame and
returned the previous call's result. Frames that are not JSON objects are now
treated as fatal protocol errors that kill and reset the worker. The startup
handshake had the same unguarded parse, which additionally leaked a live
Pyodide process on every retry.

search_sandbox_files crashed on patterns an agent can plausibly emit.
Path.glob raises on an empty pattern, on a malformed "***", and on absolute
paths such as "/etc/passwd". The pattern is model-supplied and the tool is
agent-facing, so these now read as "no matches" instead of raising out of the
call.

Vendored wheels were installed without integrity checking. PyPI publishes a
sha256 in the release metadata that download-wheels.mjs ignored. These wheels
are installed into the interpreter, so a corrupted or substituted file is code
execution. Downloads are now verified against the published digest and refused
on mismatch, and cached wheels are re-verified rather than trusted by filename
(the cache lives in a writable working directory). Confirmed the comparison is
correct: all 11 locally cached wheels match PyPI's published digests.

The new eval scenario could not actually run. The interpreter fails closed and
the opt-in is set only by the unit suites' autouse fixture, so a scenario run
would fail with a confusing "agent did not use the code interpreter". The
requirement is now documented in both the scenario and the test case rather
than papered over by re-adding the opt-in to servers.yaml, which would defeat
the gate.

Adds five regression tests, four of which were confirmed to fail against the
unfixed code.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The threat model listed --permission as weak-but-useful defense-in-depth.
Testing shows that is wrong in a way worth recording: Pyodide cannot start
under it at all. Pyodide calls process.binding during staticInit, which the
permission system denies unconditionally, and no flag re-enables it. Verified
on Node 24 -- the worker fails with ERR_ACCESS_DENIED even when every
permission flag is granted (--allow-fs-read=* --allow-fs-write=*
--allow-child-process --allow-worker --allow-wasi --allow-addons).

This matters for the isolation decision: the choice is not between weak
in-process confinement and strong OS confinement, it is between no in-process
confinement and OS confinement. Recorded so the next person does not spend the
same time discovering the flags do not apply.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two high-severity findings from a review of the previous fix commit, both
reproduced first.

Cancelling a call returned the wrong answer to the next one. The previous
commit added handlers for timeout, oversized frames, and malformed frames, but
not for cancellation -- and that was the one remaining path that produces a
silently wrong result instead of an error. If the caller goes away mid-request
(client disconnect, MCP cancellation, an outer wait_for), CancelledError
unwinds through the lock with the process still attached and the worker's reply
still unread. The next execute() then reads that stale frame. Reproduced
against the real CodeInterpreter: call 2 received "RESULT-OF-CALL-1". Nothing
upstream could catch it either, since CancelledError is a BaseException and
mcp_sandbox only catches Exception. The worker is now killed and the
cancellation re-raised.

A cached wheel that failed its digest was left on disk and still installed.
The check detected the bad file, warned, and returned -- but never removed it,
and the worker loads whatever readdir() returns without verifying anything. So
the check reported a compromise and then permitted it, which is worse than not
checking. Failed files are now deleted, so a failed repair degrades to
"absent" rather than "known-bad but present". Verified by planting a corrupt
tabulate wheel: it is detected, removed, and gone even when the refetch fails.
Writes also go through a temp file plus rename, so an interrupted install
cannot leave a truncated wheel that fails every later check with no way to
repair itself offline.

The docs claimed more than that mechanism delivers, so they now say plainly
that it is an integrity check against corruption, not a trust boundary: the
worker matches on distribution name and does not re-verify at load time, and a
file planted under a different version string is never examined. Anyone who can
write to that directory can already run code as this user.

Also tightened test_search_files_tolerates_unusable_patterns, where '[' and
'a[b' were passing vacuously -- they match literally rather than raising. The
patterns that genuinely raise are now asserted to raise before checking that
the tool swallows them.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The previous version asserted that 'C:\\Windows\\win.ini' raises from
Path.glob. That holds on Windows, where it is an absolute path, but on Linux it
is just a relative name containing backslashes, so glob returns no matches
instead of raising -- and CI failed there.

Only the pattern-syntax errors ('' and '***') raise on every platform, so those
are what pin the guard as load-bearing. Everything else is asserted only to
return no matches without raising, which is the property the tool actually
needs. Also folded in '../*' to keep the traversal guard covered.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CodeQL flagged the bare 'await task' inside pytest.raises as a statement with
no effect -- it cannot see that awaiting the task to observe its exception is
the point. Replaced with an explicit try/except that names the outcome, which
satisfies the analyzer and states the assertion more clearly.

Behaviour unchanged and still falsifiable: removing the CancelledError handler
makes the test fail with 'worker survived cancellation with an unread reply
pending'.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ali Keramati (alikera) and others added 6 commits August 31, 2026 09:24
Cancellation is now handled at every await in execute(), not just the read.
_ensure_started() and the request write/drain were unguarded, so a cancellation
there left the worker attached with a half-written request or an unconsumed
handshake, and the next call read a frame that did not belong to it. _start()
guards its handshake too: cancelled there, the child is spawned but not yet
stored anywhere, so it would have been orphaned outright.

_kill() now reaps reliably. It is reached almost exclusively from cancellation
handlers, where a bare `await proc.wait()` is cancelled again immediately and
returns before the child is collected. The wait is shielded and bounded, and
the process handle is detached first so an interrupted kill can never leave a
reusable reference behind.

Wheels are pinned by exact version, filename and SHA-256 in pypi-packages.mjs.
The worker previously resolved a package by scanning wheels/ for any file whose
distribution segment matched, so readdir() order decided which artifact was
installed -- a file named openpyxl-0.0.1-py3-none-any.whl would win. It now
accepts only the pinned filename and otherwise falls back to `name==version`
rather than a bare name. Demonstrated against the local cache: with pypdf
6.16.2 pinned and 6.16.1 present, the old matcher selected 6.16.1 while the new
one declines it.

download-wheels.mjs resolves that pinned release instead of whatever is current,
and cross-checks PyPI's published digest against the pin before downloading, so
a silent upstream change is a hard error rather than a new artifact.

Integrity failures now fail the install. A corrupt cached wheel that cannot be
removed, a file still present after removal, or a PyPI digest that disagrees
with the pin all raise IntegrityError, which is rethrown after the batch and
sets a non-zero exit. Only network failures still degrade to a runtime fetch.

The oversize test drives a real subprocess emitting a 200 KB line rather than
injecting a synthetic ValueError, with a control proving a 100 KB frame is
still returned intact. Cancellation is covered at all three points: startup,
drain, and read.

The eval assertion no longer accepts a hard-coded answer. Naming sales.csv in a
string literal counted as evidence of reading it, so `print("sales.csv: East
7312.50")` would have passed. It now requires an actual read call, and requires
the figure to appear in interpreter output while being absent from the code
that produced it. Verified both ways: the hard-coded form is rejected, the
computed form accepted.

The PR description said "the agent loaded all three files". No model run has
happened -- code_interpreter was driven directly -- so it now says direct tool
validation and states plainly that no code_execution trace exists yet.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A downloaded wheel whose bytes disagree with the pinned SHA-256 now raises
IntegrityError instead of warning and returning. Degrading to a runtime
micropip fetch meant the pinned artifact could not be obtained and the worker
would install whatever PyPI served next, which is the opposite of what pinning
is for. Regression test drives the real script against a local stub PyPI that
advertises the pinned digest and serves different bytes; confirmed falsifiable
by restoring the old behaviour, where it fails with "install succeeded despite
a digest mismatch".

Two env seams were added to make that testable -- THINKINGBOX_PYPI_BASE_URL and
THINKINGBOX_WHEELS_DIR -- both documented as test-only, with production
defaults unchanged.

_kill() no longer swallows cancellation unconditionally. It takes
during_cancellation, set only by the three callers that are already unwinding a
CancelledError and will re-raise it. Reached from a timeout or malformed-frame
handler, a cancellation arriving during the shielded reap is a new external
request to stop, and dropping it would leave the task running. Shielded reaping
is unchanged. Tests cover both directions.

close() gained the same treatment: it caught (TimeoutError, Exception), which
does not include CancelledError, so cancelling a teardown dropped the child
without killing it.

The startup-cancellation test now exercises the real _start(). It previously
substituted a fake that performed its own cleanup, so it verified the test's
own code rather than the implementation. Only create_subprocess_exec is
stubbed; the spawn, handshake read, cancellation handling, kill and reap are
all real. Writing it surfaced that _kill() deliberately retains the worker temp
dir for restart, so the test now asserts the actual contract -- close()
reclaims it -- instead of an eager delete that would break reuse.

The dataset assertion now compares exact structured output derived from the
fixture. Ground truth is computed from sales.csv at test time rather than
hard-coded, so editing the fixture changes what the test demands. It requires
every per-region total to appear in interpreter output and none of them to
appear as literals in the code that produced it. Verified against four cases:
computed totals accepted; totals hard-coded into a print() rejected; only the
top total rejected; unit sums instead of revenue rejected.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CodeQL flagged the empty except. The clause was not doing nothing -- it was
absorbing the expected cancellation -- but that intent was implicit. The test
now captures what was raised and asserts it is CancelledError, which states the
actual claim: _kill() must not mask the cancellation with a different error.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The previous commit asserted that awaiting a cancelled _kill(
during_cancellation=True) re-raises CancelledError. It does not, and must not:
swallowing is the whole point of that flag. The caller is inside an
�xcept CancelledError: block and re-raises immediately after, so a second
cancellation from _kill() would compete with it. The task therefore completes
normally, which is what the test now asserts.

My previous commit pushed this test in a failing state.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The assertion required one execution to both read sales.csv and emit the
totals. The interpreter is a stateful REPL -- a documented feature with its own
passing tests -- so a model may reasonably load the CSV in one call and
aggregate off the persisted DataFrame in the next. That is the better pattern,
and the test rejected it: the reading call prints nothing, and the call that
prints never names the file, so it was filtered out before the numeric check
ran. Confirmed against a simulated two-step session, which failed.

Evidence is now gathered across the session in order. Some execution must read
the fixture, and some execution at or after it must emit every per-region total
while not carrying those totals as literals in its own code.

The ordering requirement is load-bearing rather than incidental: totals printed
before anything was read cannot have come from the data, so that case is still
rejected.

Exact numeric comparison against ground truth derived from the fixture is
unchanged. Verified across nine cases -- the two-step split and a three-step
variant with an unrelated call between now pass, alongside the single-call
form, while hard-coded totals, totals emitted before any read, no read at all,
partial totals, unit sums instead of revenue, and totals after a failed read
are all still rejected.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
domain:data-analysis does not exist. thinkingbox/common/tag_types.py builds the
Domain enum from tag_taxonomy.yaml, falling back to the shipped example, and the
public fallback defines exactly three members: customer-service, hr and misc.
Hydrating the scenario against the public framework therefore fails before any
model is called.

misc is the honest fit; the other two do not describe this suite.

This repo's CI never caught it because nothing here hydrates scenarios --
test-thinkingbox-tools.yml runs the server package's pytest and nothing else --
so the tag stayed invalid through green CI. Found by running the scenario
end-to-end against a real model, which is the only thing that exercises this
path.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@alikera
Ali Keramati (alikera) force-pushed the alikera-port-sandbox-code-interpreter-pyodide branch from 7e46ff6 to 4bf1f0b Compare August 31, 2026 16:25
@alikera Ali Keramati (alikera) changed the title Port Pyodide sandbox code interpreter MCP server from AI.ThinkingBox.Data#208 Add Pyodide sandbox code interpreter MCP server Aug 31, 2026
@susana-palmaz

Copy link
Copy Markdown

We have moved away from pyodide and into using python WASM instead. Security and speed were the main concerns and we wanted to only support one backend. There are some downsides to the change, related to support for not pure cython based dependencies, but we can live with that. I don't think pyodide it should be re-introduced, but I would welcome a thorough review like the one done here for python wasm.
See merged PR where pyodide was removed: https://github.com/microsoft/AI.ThinkingBox.Data/pull/224

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.

4 participants