Repository navigation
LOU-F: MCP, sandboxing & provider expansion - #7
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ead of silently falling back to unsandboxed execution executeToolViaSandbox() previously ran a disconnected dummy command (process.execPath --version) through the sandbox adapter and then unconditionally called the tool's real tool.execute() in-process on the host, regardless of what sandbox adapter was configured. This violated LOU-F5's AC that a flagged tool never runs its execute() directly in-process, since even a real isolating adapter would never actually gate the tool's logic. Add ToolDescriptor.sandboxExecute(args, sandbox), an explicit contract a tool author implements to route its real work through the sandbox adapter's own run()/writeFile(). executeToolViaSandbox() now calls sandboxExecute() when present, and throws a clear, descriptive error when requiresSandbox is true but sandboxExecute is missing - failing closed rather than silently executing unsandboxed code while claiming it's isolated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the old sandbox-wiring tests (which only asserted sandbox.run() was called with a disconnected dummy command before tool.execute() ran anyway) with tests that verify the real contract: - A tool with sandboxExecute() routes real work through the sandbox adapter (writeFile/run called with tool-relevant parameters) and tool.execute() is never called. - A tool with requiresSandbox:true and no sandboxExecute() fails closed: tool.execute() and the sandbox adapter are both never touched, and a clear error surfaces. - An unflagged tool is unaffected (unchanged prior path). - NoopSandbox still genuinely executes a sandboxExecute-provided command, confirming the wiring is a real dependency on the adapter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FlowExecutor.executeToolCall() called toolDesc.tool.execute() directly with no requiresSandbox/sandboxExecute check at all - not even the fail-closed throw AgentExecutor.executeToolViaSandbox() has - so a requiresSandbox:true tool invoked through a Flow's tool-call node ran unsandboxed in-process, silently. resume.ts's post-approval tool execution had the exact same gap. Extracts the requiresSandbox/sandboxExecute branching into a shared executeToolWithSandboxGuard() helper (src/execution/sandboxGuard.ts) so AgentExecutor.ts, FlowExecutor.ts, and resume.ts all route tool invocation through the same seam instead of each reimplementing it - this exact bug already recurred once across two call sites, so a single shared choke point closes it for good. FlowExecutionContext gains an optional `sandbox` field (defaults to NoopSandbox), mirroring AgentExecutor's ExecuteOptions.sandbox pattern. Adds regression tests to FlowExecutor.test.ts mirroring the AgentExecutor sandbox-wiring tests: sandboxExecute is used and tool.execute is never called for a requiresSandbox tool; a requiresSandbox tool with no sandboxExecute fails closed and surfaces via FlowExecutor's existing success:false/error result shape; an unflagged tool behaves exactly as before. 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
Implements all 8 tickets from LOU-F (Phase 2-3), widening what an agent can safely reach without changing how
AgentExecutor/ToolRegistrywork:jsonSchemaToZod()converts an MCP tool's JSON SchemainputSchemainto an equivalent Zod schema (object/array/enum/optional fields); fails loudly on$ref/recursive schemas rather than silently mishandling them.McpToolLoaderconnects via@modelcontextprotocol/sdk, lists a server's tools, and synthesizes oneToolDescriptorper remote tool named<connection>__<tool>to avoid cross-server collisions — registers intoToolRegistryexactly like any built-in tool.SandboxAdapterinterface +NoopSandbox(zero-isolation default, preserves existing trusted-host behavior); a newrequiresSandboxflag onToolDescriptoropts individual tools into sandboxed execution.SubprocessSandbox(Docker-backed,NetworkMode: 'none',AutoRemove: true) as a real isolated implementation.AnthropicProviderimplementing the sameLLMProviderinterface asOpenAIProvider/OllamaProvider;resolveProvider("provider/model")parsing a Mastra-style spec string and resolving API keys from an env-var convention.A real gap found and fixed
QA didn't stop at "the sandbox wiring code path exists" — it asked whether a genuinely isolating adapter would actually isolate anything through it. It didn't: the original wiring wrote tool args to a file and ran a disconnected dummy command (
node --version) through the sandbox, then unconditionally executed the tool's real logic in-process anyway — meaning even withSubprocessSandboxconfigured, a flagged tool's actual code would still run unsandboxed on the host. This directly violated the ticket's own AC ("a flagged tool never runs its execute() directly in-process").Fixed with an explicit
sandboxExecute?: (args, sandbox) => Promise<unknown>contract onToolDescriptor— a tool author now implements this to route its real work through the sandbox adapter itself. If a tool is flaggedrequiresSandboxbut doesn't implementsandboxExecute,AgentExecutornow throws a clear error and refuses to run it, rather than silently falling back to unsandboxed execution (fail-closed, matching LOU-E's guardrail philosophy). Verified via spies that the tool's ownexecute()is never called on the sandboxed path, and that the no-sandboxExecutecase genuinely throws with zero calls to either path.Notes
Docker is not available in this environment, so
SubprocessSandbox's 2 daemon-dependent integration tests are runtime-guarded and skip gracefully; non-daemon unit tests (interface shape, container config) run and pass.@ai-sdk/anthropicwas pinned to^0.0.42to match this repo's existingai@4.3.19/@ai-sdk/openai@^0.0.42generation (latest@ai-sdk/anthropicis incompatible).npx vitest run: 591 passing, 2 skipped (Docker). 3 known pre-existing provider-suite failures (missing optional peer deps) — unrelated to this epic.npx tsc --noEmit: clean.🤖 Generated with Claude Code