Skip to content

feat: update vended tools/plugins for sandbox compatibility"#2649

Open
gautamsirdeshmukh wants to merge 4 commits into
strands-agents:mainfrom
gautamsirdeshmukh:feat/sandbox-tools
Open

feat: update vended tools/plugins for sandbox compatibility"#2649
gautamsirdeshmukh wants to merge 4 commits into
strands-agents:mainfrom
gautamsirdeshmukh:feat/sandbox-tools

Conversation

@gautamsirdeshmukh

@gautamsirdeshmukh gautamsirdeshmukh commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

Update vended tools (bash, fileEditor) and plugins (context offloader, skills) to be Sandbox compatible.

This is the fourth PR split off from strands-agents/sdk-typescript#1011, preceded by the base interface merged in strands-agents/sdk-typescript#1090, Docker/SSH implementations merged in strands-agents/sdk-typescript#1110 and Agent integration merged in #2563.

What's New

  • Vended tools (bash, file editor) as now sandbox-aware, all routing handled under the hood
  • Vended plugins (context offloader, skills) are now sandbox-aware, all routing handled under the hood
    • Added SandboxStorage file storage implementation for context offloader
  • getTools() overridable method added to sandboxes enabling default tool registration
    • Empty by default, bash and file editor auto-registered for Docker and SSH sandboxes
  • makeBash() factory for generating bash tool instances with environment-appropriate descriptions

Related Issues

Documentation PR

Not yet

Type of Change

New feature

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli

  • I ran all tests + added several

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment thread strands-ts/src/vended-tools/bash/bash.ts Outdated
Comment thread strands-ts/src/agent/agent.ts
Comment thread strands-ts/src/sandbox/__tests__/default.test.node.ts
Comment thread strands-ts/src/vended-plugins/context-offloader/storage.ts
Comment thread strands-ts/src/sandbox/not-a-sandbox-local-environment.ts
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Issue: This PR adds sandbox as a new public property on the LocalAgent interface and the AgentConfig type. It also introduces statFile as a new abstract method on the Sandbox base class (a breaking change for external implementors), and adds getTools() to Sandbox. These are moderate-to-substantial API surface changes that affect customers implementing Sandbox or LocalAgent.

Per the API Bar Raising process, this PR should have the needs-api-review label. The PR description documents the feature well but doesn't include the specific API review materials (complete API signatures with defaults, example usage snippets from a customer perspective, module exports map).

Suggestion: Add the needs-api-review label and include a brief API summary section in the PR description showing:

  1. New AgentConfig.sandbox option with usage example
  2. New abstract statFile() on Sandbox (breaking for existing custom implementations)
  3. getTools() override point on Sandbox

Comment thread strands-ts/src/sandbox/base.ts
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Assessment: Comment

Well-structured PR that makes vended tools sandbox-aware with a clean architecture (default slot pattern, environment-conditional entry points). The code is modular and well-tested.

Review Categories
  • API Semantics: The sandbox: false option's documented intent ("distinct opt-out") doesn't match its runtime behavior (same as omitting). The getter uses || which treats false and undefined identically — clarify the documentation or fix the implementation.
  • Breaking Changes: New abstract statFile() on Sandbox breaks existing custom implementations. Since Sandbox was introduced recently, this is likely acceptable but should be documented.
  • Exports Completeness: SandboxTimeoutError, SandboxAbortError, DockerSandbox, SshSandbox, and NotASandboxLocalEnvironment are not re-exported from the top-level index, which contradicts the "Prefer Flat Namespaces" decision record.
  • Implementation Coupling: The instanceof NotASandboxLocalEnvironment check in bash.ts is a capability-routing decision hardcoded to a concrete class rather than an interface/capability flag.
  • Feature Parity: NotASandboxLocalEnvironment silently ignores the env option in ExecuteOptions, unlike the Docker/SSH implementations.
  • API Review Process: This PR introduces new public API surface (AgentConfig.sandbox, LocalAgent.sandbox, Sandbox.statFile, Sandbox.getTools) and should have the needs-api-review label per the bar-raising process.

The overall design is solid — default slot for environment-specific behavior, clean test separation by environment, and good use of the streaming infrastructure.

Comment thread strands-ts/src/agent/agent.ts
Comment thread strands-ts/src/sandbox/base.ts Outdated
Comment thread strands-ts/src/vended-tools/bash/bash.ts Outdated
Comment thread strands-ts/src/vended-plugins/context-offloader/storage.ts Outdated
Comment thread strands-ts/src/sandbox/base.ts
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Issue: The PR introduces or modifies several public API surfaces (new sandbox config on Agent, new statFile abstract method, new getTools() on Sandbox, new SandboxStorage export, new DefaultNotConfiguredError). The PR description documents use cases and what's new, but doesn't include API signatures with defaults or module export changes in the format expected by the API bar-raising process.

Suggestion: Consider adding the needs-api-review label given the scope of public API changes — particularly the new abstract method on Sandbox (breaking) and the new property on LocalAgent interface (breaking). These merit explicit API reviewer sign-off per the bar-raising guidelines.

Comment thread strands-ts/src/sandbox/__tests__/default.test.node.ts
Comment thread strands-ts/src/sandbox/not-a-sandbox-local-environment.ts
Comment thread strands-ts/src/sandbox/posix-shell.ts Outdated
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Assessment: Comment

Well-architected feature that cleanly integrates sandbox awareness into vended tools and plugins. The defaultSlot pattern, node-specific entry point, and tool-routing approach are all solid design choices.

Review Categories
  • API Breaking Changes: Adding statFile as abstract on Sandbox and sandbox on LocalAgent are breaking changes for downstream implementors — these should be called out and either mitigated (default implementations) or versioned appropriately.
  • API Design: The sandbox: false config option documents a future distinction that has no runtime effect today — this adds API surface area without clear value. The instanceof NotASandboxLocalEnvironment check in bash is a fragile discrimination mechanism.
  • Public API Completeness: SandboxTimeoutError and SandboxAbortError should be exported so consumers can catch them by type.
  • API Bar-Raising: Given the scope of public API additions (new abstract methods, new interface properties, new exported classes), this PR likely warrants the needs-api-review label.

Good test coverage overall and the architecture cleanly separates concerns between the sandbox abstraction, the default-slot mechanism, and the tool routing.

if (input.mode === 'execute' && !input.command) {
throw new Error('command is required when mode is "execute"')
}
export interface MakeBashOptions {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any reason we don't support input schema, and name too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updating to take name and inputSchema along with description

}

override getTools(): Tool[] {
return [fileEditor, makeBash({ description: SANDBOX_BASH_DESCRIPTION })]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's namespace tool names

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updating to namespace with sandbox_{toolname} the way MCP adds mcp_{toolname}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-hooks Features or requests that might be implementable via hooks area-server Related to strands Server, sandbox, runtime container area-tool Tool behavior/api enhancement New feature or request size/xl typescript Pull requests that update typescript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants