Skip to content
This repository was archived by the owner on Jun 3, 2026. It is now read-only.

feat: base Sandbox interface#1090

Merged
gautamsirdeshmukh merged 1 commit into
strands-agents:mainfrom
gautamsirdeshmukh:feat/sandbox-interface
May 27, 2026
Merged

feat: base Sandbox interface#1090
gautamsirdeshmukh merged 1 commit into
strands-agents:mainfrom
gautamsirdeshmukh:feat/sandbox-interface

Conversation

@gautamsirdeshmukh

@gautamsirdeshmukh gautamsirdeshmukh commented May 21, 2026

Copy link
Copy Markdown
Contributor

Description

Introduces the Sandbox interface, the core abstraction that decouples tool logic from where code runs.

This is the base interface only. Concrete implementations (Docker, SSH), vended tools/plugins and the Agent integration ship in follow-up PRs (incrementally broken off from the parent mega-PR #1011).

What's Included

Core Interface (src/sandbox/base.ts)

  • Sandbox abstract class
    • 6 abstract methods:
      • executeStreaming(command, options?) — stream stdout/stderr from a shell command
      • executeCodeStreaming(code, language, options?) — stream output from code execution via interpreter
      • readFile(path) — read file as Uint8Array
      • writeFile(path, content) — write Uint8Array, creates parent directories
      • removeFile(path) — delete a file
      • listFiles(path) — list directory contents as FileInfo[]
    • 4 convenience methods on the base class:
      • execute(command, options?) — non-streaming wrapper over executeStreaming
      • executeCode(code, language, options?) — non-streaming wrapper over executeCodeStreaming
      • readText(path) — UTF-8 decode wrapper over readFile
      • writeText(path, content) — UTF-8 encode wrapper over writeFile

Shell-Based Defaults (src/sandbox/posix-shell.ts)

  • PosixShellSandbox abstract class (subclasses implement only executeStreaming)
    • Code execution via base64-encoded heredoc piped to interpreter
    • File read/write via base64 encoding over shell
    • Directory listing via ls -1ap with test -d guard

Related Issues

Documentation PR

Not yet

Type of Change

New feature

Testing

How have you tested the change?

  • I ran npm run check

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.

@github-actions github-actions Bot added the strands-running <strands-managed> Whether or not an agent is currently running label May 21, 2026
Comment thread strands-ts/src/sandbox/__tests__/shell.test.node.ts Outdated
Comment thread strands-ts/src/sandbox/index.ts Outdated
Comment thread strands-ts/src/sandbox/stream-process.ts
Comment thread strands-ts/src/sandbox/stream-process.ts
Comment thread strands-ts/src/index.ts
Comment thread strands-ts/src/sandbox/shell.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment

This PR introduces a clean, well-structured Sandbox abstraction with good separation between the abstract base and the shell-based implementation. The API design is thoughtful — streaming as the primitive with convenience wrappers, and the ShellSandbox layer that reduces implementor burden to a single method.

Review Themes
  • Code Duplication: The TestShellSandbox in the test file duplicates the TestSandbox fixture — should reuse the shared fixture.
  • Export Surface: streamProcess utility and ShellSandbox export decisions should be clarified — users implementing custom sandboxes will need these.
  • Documentation Gaps: StreamProcessOptions and streamProcess are missing TSDoc per project standards; ShellSandbox methods could benefit from implementation notes.
  • API Review: This introduces a new public abstraction (Sandbox) that customers will extend. Consider adding the needs-api-review label per API Bar Raising guidelines, as this is a new primitive/frequently-used abstraction.

The design is solid — streaming-first with non-streaming convenience methods is the right pattern, and the language validation guard in executeCodeStreaming is a good security measure.

@github-actions github-actions Bot removed the strands-running <strands-managed> Whether or not an agent is currently running label May 21, 2026
@gautamsirdeshmukh gautamsirdeshmukh force-pushed the feat/sandbox-interface branch from 0e736f1 to f008526 Compare May 21, 2026 16:54
@github-actions github-actions Bot added the strands-running <strands-managed> Whether or not an agent is currently running label May 21, 2026
Comment thread strands-ts/src/utils/shell-quote.ts
Comment thread strands-ts/src/sandbox/constants.ts
Comment thread strands-ts/src/sandbox/__tests__/posix-shell.test.node.ts
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment

Clean architecture with good separation of concerns. The previous review covered the main substantive issues (duplication, TSDoc gaps, export surface), and the author has committed to fixing all of them. This follow-up catches a few remaining gaps.

Additional Review Items
  • Documentation: AGENTS.md directory structure must be updated to reflect the new src/sandbox/ and src/utils/ directories (explicit requirement in AGENTS.md itself).
  • TSDoc completeness: shellQuote and LANGUAGE_PATTERN are exported but missing required TSDoc tags.
  • Test assertions: Several tests check individual fields (result.exitCode, result.stdout) rather than the full ExecutionResult object — per TESTING.md, full object assertions catch silent regressions.
  • API review: Reiterating the suggestion to add needs-api-review label, as this introduces a new primitive abstraction (Sandbox) per API Bar Raising guidelines.

@github-actions github-actions Bot removed the strands-running <strands-managed> Whether or not an agent is currently running label May 21, 2026
@gautamsirdeshmukh gautamsirdeshmukh force-pushed the feat/sandbox-interface branch from f008526 to 0711ac8 Compare May 21, 2026 17:18
@github-actions github-actions Bot added the strands-running <strands-managed> Whether or not an agent is currently running label May 21, 2026
Comment thread strands-ts/src/sandbox/base.ts
Comment thread strands-ts/eslint.config.js Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment

The architecture and API design are solid. Previous review items have been acknowledged and are being addressed. This review surfaces two remaining documentation gaps and one unnecessary change.

Review Themes
  • TSDoc completeness: The Sandbox abstract class methods are missing @returns tags, inconsistent with the existing pattern in src/models/model.ts where all abstract methods have them.
  • Minimal changes: Two unused ESLint globals (setInterval/clearInterval) were added that aren't used in this PR or anywhere in the codebase.
  • AGENTS.md update: Still outstanding — new src/sandbox/ and src/utils/ directories need to be added to the directory structure (explicit MUST requirement in AGENTS.md).

The streaming-first design with heredoc-based code execution is well thought out, and the security properties (base64 EOF markers can't collide, language validation, shell quoting) are correct.

@github-actions github-actions Bot removed the strands-running <strands-managed> Whether or not an agent is currently running label May 21, 2026
@gautamsirdeshmukh gautamsirdeshmukh force-pushed the feat/sandbox-interface branch from 0711ac8 to 2e707f5 Compare May 21, 2026 18:14
@github-actions github-actions Bot added the strands-running <strands-managed> Whether or not an agent is currently running label May 26, 2026
Comment thread strands-ts/src/index.ts
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Approve

Well-designed abstraction with correct security properties, comprehensive test coverage for the happy path, and clean streaming-first API. The code has been through multiple review iterations and all prior feedback has been addressed.

Review Themes
  • Test coverage gap: streamProcess (180 lines of complex async logic) has no dedicated unit tests — its error/edge paths (ENOENT, SIGKILL escalation, signal exit codes) are only partially exercised through integration tests.
  • Export surface: streamProcess and shellQuote are essential for custom sandbox implementations but not reachable by external consumers. This should be clarified in a follow-up.

The architecture is solid and aligns well with the SDK tenets — streaming as the primitive, convenience methods on top, and PosixShellSandbox reducing implementor burden to a single method.

@github-actions github-actions Bot removed the strands-running <strands-managed> Whether or not an agent is currently running label May 26, 2026
@gautamsirdeshmukh gautamsirdeshmukh force-pushed the feat/sandbox-interface branch from b6c9fc2 to f4383b0 Compare May 26, 2026 19:03
@github-actions github-actions Bot added the strands-running <strands-managed> Whether or not an agent is currently running label May 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Approve

Clean, well-designed abstraction with correct security properties and comprehensive test coverage. No new issues found after thorough analysis of shell quoting, event race conditions, streaming loop semantics, and type safety.

Review Details

Verified correct:

  • Shell quoting (shellQuote + dirname nesting) handles spaces, single quotes, and metacharacters
  • close/error event race in streamProcess is guarded by if (!done) — first event wins
  • btoa/atob pattern for binary-safe base64 is correct for both UTF-8 code and arbitrary bytes
  • QUOTING_STYLE=literal is portable (no-op on BSD ls, prevents quote characters on GNU ls)
  • Heredoc EOF marker is collision-proof via crypto.randomUUID()
  • execute() convenience method correctly throws if no ExecutionResult is yielded
  • ENOENT case is tested with full object assertion (toStrictEqual)

Prior feedback status: All 8 prior automated review items have been addressed. The two remaining open items (export surface for external implementors, dedicated streamProcess unit tests) are acknowledged as follow-up work.

@github-actions github-actions Bot removed the strands-running <strands-managed> Whether or not an agent is currently running label May 26, 2026
mkmeral
mkmeral previously approved these changes May 26, 2026
@gautamsirdeshmukh gautamsirdeshmukh force-pushed the feat/sandbox-interface branch from f4383b0 to 392a184 Compare May 27, 2026 04:12
@github-actions github-actions Bot added strands-running <strands-managed> Whether or not an agent is currently running and removed strands-running <strands-managed> Whether or not an agent is currently running labels May 27, 2026
@gautamsirdeshmukh gautamsirdeshmukh added this pull request to the merge queue May 27, 2026
Merged via the queue into strands-agents:main with commit bb696fc May 27, 2026
20 of 34 checks passed
@gautamsirdeshmukh gautamsirdeshmukh deleted the feat/sandbox-interface branch May 27, 2026 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants