Repository navigation
LOU-K2: opt built-in I/O tools into the sandbox seam - #16
Merged
Merged
Conversation
The SDK's fail-closed sandboxing seam (ToolDescriptor.requiresSandbox / sandboxExecute, SandboxAdapter, executeToolWithSandboxGuard) was fully implemented and wired into AgentExecutor/resume.ts but unused by every shipped built-in tool, so the feature was dead in practice. Flags the tools that do meaningful external I/O - http (arbitrary model-chosen outbound requests), github, jira (external API calls scoped by config, still real network reach), and slack (webhook POST) - with requiresSandbox: true and a real sandboxExecute() that routes the actual outbound fetch through the configured SandboxAdapter via a new shared helper (sandboxFetch.ts): it runs the request as a Node subprocess through sandbox.run(), so a real isolation backend (e.g. SubprocessSandbox) gets a genuine chance to mediate the call, while NoopSandbox reproduces the exact same network request unsandboxed callers always got. currentDate/dayName are left unflagged - they do no I/O, so a sandbox boundary isn't a meaningful security property for them. execute() is left untouched on every tool (not stubbed out): this codebase has real call sites (examples/ops-pipeline's guardedPr.ts, and existing tests) that invoke descriptor.tool.execute() directly, bypassing executeToolWithSandboxGuard entirely - stubbing execute() to fail closed would have broken them. sandboxExecute() is added as a genuine parallel path used by guard-routed callers (AgentExecutor, resume.ts); execute() remains real and directly callable exactly as before. - github.ts/jira.ts: added a register() override that wraps every fetch-performing tool's execute() with withSandboxedFetch(), which swaps globalThis.fetch for the sandboxed implementation only for the duration of that call - avoiding a hand-written sandboxed reimplementation of all ~50 register*() methods. GitHub's already-out-of-scope tools (OUT_OF_SCOPE_TOOLS) are left unflagged since they throw before ever reaching the network. - http.ts: refactored makeHttpRequest() into a transport-parameterized performHttpRequest() so the SSRF-checking/redirect-following logic is shared between the direct (undici) and sandboxed transports. - slack.ts: added postSlackAlertViaSandbox(), mirroring postSlackAlert() but posting through sandboxHttpFetch(). - Extended http.test.ts/github.test.ts/slack.test.ts and added jira.test.ts (no prior test file existed) to prove: (a) behavior is unchanged under NoopSandbox, (b) a custom SandboxAdapter is genuinely invoked for the outbound call, (c) the tool fails closed per the existing guard contract when sandboxExecute is missing. - Also exported slack.ts from tools/built-in/index.ts (previously missing from the barrel). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The SDK's fail-closed sandboxing seam (
ToolDescriptor.requiresSandbox/sandboxExecute,SandboxAdapter,executeToolWithSandboxGuard()) was fully implemented and wired intoAgentExecutor/resume.ts, but no shipped built-in tool used it —requiresSandboxhad zero matches undersrc/tools/built-in/. This PR opts the tools that do meaningful external I/O into the seam.Which tools got
requiresSandbox: true, and whyhttpTool(http.ts) — makes arbitrary, model-chosen outbound HTTP requests. The single highest-risk built-in tool for a sandbox boundary to matter on.GitHubTools(github.ts) — every in-scope tool (everything except the already-OUT_OF_SCOPE_TOOLS-gated ones, which throw before ever reaching the network and are deliberately left unflagged) calls the real GitHub API.JiraTools(jira.ts) — every tool calls the caller-configured Jira instance.slackTool(slack.ts) — POSTs to a webhook URL (env var or options).currentDateTool,dayNameTool— no I/O, so a sandbox boundary isn't a meaningful security property for them (per the ticket's own guidance not to force this everywhere).How sandboxing is implemented
A new shared helper,
src/tools/built-in/sandboxFetch.ts:sandboxHttpFetch(sandbox, request)— encodes the request as JSON, runs it as a Node one-liner viasandbox.run('node', ['-e', script], ...), and decodes the subprocess's JSON stdout back into a realResponse. This is the only way to route an outbound HTTP call throughSandboxAdapter, since the adapter only exposesrun()/writeFile(), not an injectable fetch. UnderNoopSandboxthis performs the exact same network request as callingfetch()in-process, just via a childnodeprocess.withSandboxedFetch(sandbox, fn)— swapsglobalThis.fetchfor a sandbox-routed implementation only for the duration offn(), restoring the original afterward (even on throw). Used bygithub.ts/jira.tsso their ~50register*()methods (which all call the ambient globalfetch) don't each need a hand-written sandboxed reimplementation.Important design decision —
execute()is left completely unchanged on every tool, not stubbed out. This codebase has real call sites that invokedescriptor.tool.execute()directly, entirely bypassingexecuteToolWithSandboxGuard()— e.g.examples/ops-pipeline/guardedPr.tsandexamples/ops-pipeline/index.tscallslackTool.tool.execute!(...)/githubCreatePrTool.tool.execute!(...)directly, andgithub.test.ts's existing tests do the same. Stubbingexecute()to fail closed (as the ticket's fallback suggestion allows "if the guard makes it unreachable") would have broken those real, already-shipped paths, sosandboxExecute()is added as a genuine parallel path used only by guard-routed callers (AgentExecutor,resume.ts); direct callers keep working exactly as before.Per-file changes:
http.ts— refactoredmakeHttpRequest()into a transport-parameterizedperformHttpRequest()so the SSRF-checking/redirect-following logic (isBlockedHost, per-hop checks,maxRedirects) is shared between the direct (undici) transport and the new sandboxed transport (makeHttpRequestViaSandbox()), rather than duplicated.github.ts/jira.ts— added/extended aregister()override that wraps every fetch-performing tool'sexecute()withwithSandboxedFetch()forsandboxExecute().slack.ts— addedpostSlackAlertViaSandbox(), mirroringpostSlackAlert()but posting throughsandboxHttpFetch(). Also exported fromtools/built-in/index.ts(it was missing from that barrel before this change).Tests
Extended
http.test.ts,github.test.ts,slack.test.ts, and addedjira.test.ts(no prior test file existed for Jira). Each proves:NoopSandbox— real local-server round-trips forhttp/slack/jira(whose target URL is caller-configurable); forgithub(whose base URL is hardcoded toapi.github.com),execute()is asserted to still work unsandboxed exactly as before.SandboxAdapteris genuinely invoked (sandbox.run('node', ...)) for the outbound call, with its canned response correctly parsed back into the tool's result.executeToolWithSandboxGuardcontract) whensandboxExecuteis missing.Verification
npm run typecheck— clean.npm run lint— 5 pre-existing errors insrc/execution/__fixtures__/sample-repo/**(unrelated fixture files, predate this change), 369 pre-existing warnings; no new errors from touched files.npx vitest run src/tools src/execution/sandbox-wiring.test.ts examples— 125 passed, 0 failed.npx vitest runhas 5 pre-existing failures unrelated to this change (missing optional peer depollama-ai-provider, andpackages/create-loushy-agenttests needing adist/build) — confirmed identical onmainby stashing this change and re-running.Out of scope
Did not touch the published audit artifact, per the ticket.
🤖 Generated with Claude Code