mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-08 03:00:28 -04:00
fix(backend/copilot): address reviewer feedback on E2B bridge API surface
- Rename _bridge_to_sandbox to bridge_to_sandbox (public) since it is imported cross-module from tool_adapter.py (item 4) - Extract duplicated bridge+append-annotation pattern into shared bridge_and_annotate() helper used by both e2b_file_tools and tool_adapter (item 5) - Add tests verifying bridge_and_annotate is called from _read_file_handler in tool_adapter when a sandbox is active (item 2) - Add unit tests for bridge_and_annotate helper itself
This commit is contained in:
@@ -32,7 +32,7 @@ from backend.copilot.context import (
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Default number of lines returned by ``read_file`` when the caller does not
|
||||
# specify a limit. Also used as the threshold in ``_bridge_to_sandbox`` to
|
||||
# specify a limit. Also used as the threshold in ``bridge_to_sandbox`` to
|
||||
# decide whether the model is requesting the full file (and thus whether the
|
||||
# bridge copy is worthwhile).
|
||||
_DEFAULT_READ_LIMIT = 2000
|
||||
@@ -153,11 +153,11 @@ async def _handle_read_file(args: dict[str, Any]) -> dict[str, Any]:
|
||||
if not result.get("isError"):
|
||||
sandbox = _get_sandbox()
|
||||
if sandbox is not None:
|
||||
bridged = await _bridge_to_sandbox(sandbox, file_path, offset, limit)
|
||||
if bridged:
|
||||
result["content"][0][
|
||||
"text"
|
||||
] += f"\n[Sandbox copy available at {bridged}]"
|
||||
annotation = await bridge_and_annotate(
|
||||
sandbox, file_path, offset, limit
|
||||
)
|
||||
if annotation:
|
||||
result["content"][0]["text"] += annotation
|
||||
return result
|
||||
|
||||
result = _get_sandbox_and_path(file_path)
|
||||
@@ -336,7 +336,7 @@ _BRIDGE_SHELL_MAX_BYTES = 32 * 1024 # 32 KB
|
||||
_BRIDGE_SKIP_BYTES = 50 * 1024 * 1024 # 50 MB
|
||||
|
||||
|
||||
async def _bridge_to_sandbox(
|
||||
async def bridge_to_sandbox(
|
||||
sandbox: Any, file_path: str, offset: int, limit: int
|
||||
) -> str | None:
|
||||
"""Best-effort copy of a host-side SDK file into the E2B sandbox.
|
||||
@@ -406,6 +406,22 @@ async def _bridge_to_sandbox(
|
||||
return None
|
||||
|
||||
|
||||
async def bridge_and_annotate(
|
||||
sandbox: Any, file_path: str, offset: int, limit: int
|
||||
) -> str | None:
|
||||
"""Bridge a host file to the sandbox and return a newline-prefixed annotation.
|
||||
|
||||
Combines ``bridge_to_sandbox`` with the standard annotation suffix so
|
||||
callers don't need to duplicate the pattern. Returns a string like
|
||||
``"\\n[Sandbox copy available at /tmp/abc-file.txt]"`` on success, or
|
||||
``None`` if bridging was skipped or failed.
|
||||
"""
|
||||
sandbox_path = await bridge_to_sandbox(sandbox, file_path, offset, limit)
|
||||
if sandbox_path is None:
|
||||
return None
|
||||
return f"\n[Sandbox copy available at {sandbox_path}]"
|
||||
|
||||
|
||||
# Local read (for SDK-internal paths)
|
||||
|
||||
|
||||
|
||||
@@ -17,10 +17,11 @@ from .e2b_file_tools import (
|
||||
_BRIDGE_SHELL_MAX_BYTES,
|
||||
_BRIDGE_SKIP_BYTES,
|
||||
_DEFAULT_READ_LIMIT,
|
||||
_bridge_to_sandbox,
|
||||
_check_sandbox_symlink_escape,
|
||||
_read_local,
|
||||
_sandbox_write,
|
||||
bridge_and_annotate,
|
||||
bridge_to_sandbox,
|
||||
resolve_sandbox_path,
|
||||
)
|
||||
|
||||
@@ -371,12 +372,12 @@ class TestSandboxWrite:
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _bridge_to_sandbox — copy SDK-internal files into E2B sandbox
|
||||
# bridge_to_sandbox — copy SDK-internal files into E2B sandbox
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _make_bridge_sandbox() -> SimpleNamespace:
|
||||
"""Build a sandbox mock suitable for _bridge_to_sandbox tests."""
|
||||
"""Build a sandbox mock suitable for bridge_to_sandbox tests."""
|
||||
run_result = SimpleNamespace(stdout="", stderr="", exit_code=0)
|
||||
commands = SimpleNamespace(run=AsyncMock(return_value=run_result))
|
||||
files = SimpleNamespace(write=AsyncMock())
|
||||
@@ -391,7 +392,7 @@ class TestBridgeToSandbox:
|
||||
f.write_text('{"ok": true}')
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -409,7 +410,7 @@ class TestBridgeToSandbox:
|
||||
f.write_text("content")
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=10, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -424,7 +425,7 @@ class TestBridgeToSandbox:
|
||||
f.write_text("content")
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
await _bridge_to_sandbox(sandbox, str(f), offset=0, limit=100)
|
||||
await bridge_to_sandbox(sandbox, str(f), offset=0, limit=100)
|
||||
|
||||
sandbox.commands.run.assert_not_called()
|
||||
sandbox.files.write.assert_not_called()
|
||||
@@ -434,7 +435,7 @@ class TestBridgeToSandbox:
|
||||
"""Bridging a non-existent file logs but does not propagate errors."""
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
await _bridge_to_sandbox(
|
||||
await bridge_to_sandbox(
|
||||
sandbox, str(tmp_path / "ghost.txt"), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -449,7 +450,7 @@ class TestBridgeToSandbox:
|
||||
sandbox = _make_bridge_sandbox()
|
||||
sandbox.commands.run.side_effect = RuntimeError("E2B timeout")
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -462,7 +463,7 @@ class TestBridgeToSandbox:
|
||||
f.write_bytes(b"x" * (_BRIDGE_SHELL_MAX_BYTES + 1))
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -481,7 +482,7 @@ class TestBridgeToSandbox:
|
||||
f.write_bytes(binary_data)
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -500,7 +501,7 @@ class TestBridgeToSandbox:
|
||||
f.write_bytes(binary_data)
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -522,7 +523,7 @@ class TestBridgeToSandbox:
|
||||
fh.write(b"\0")
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
result = await _bridge_to_sandbox(
|
||||
result = await bridge_to_sandbox(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
@@ -530,3 +531,37 @@ class TestBridgeToSandbox:
|
||||
|
||||
sandbox.commands.run.assert_not_called()
|
||||
sandbox.files.write.assert_not_called()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# bridge_and_annotate — shared helper wrapping bridge_to_sandbox + annotation
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestBridgeAndAnnotate:
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_annotation_on_success(self, tmp_path):
|
||||
"""On success, returns a newline-prefixed annotation with the sandbox path."""
|
||||
f = tmp_path / "data.json"
|
||||
f.write_text('{"ok": true}')
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
annotation = await bridge_and_annotate(
|
||||
sandbox, str(f), offset=0, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
expected_path = _expected_bridge_path(str(f))
|
||||
assert annotation == f"\n[Sandbox copy available at {expected_path}]"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_none_when_skipped(self, tmp_path):
|
||||
"""When bridging is skipped (e.g. offset != 0), returns None."""
|
||||
f = tmp_path / "data.json"
|
||||
f.write_text("content")
|
||||
sandbox = _make_bridge_sandbox()
|
||||
|
||||
annotation = await bridge_and_annotate(
|
||||
sandbox, str(f), offset=10, limit=_DEFAULT_READ_LIMIT
|
||||
)
|
||||
|
||||
assert annotation is None
|
||||
|
||||
@@ -38,7 +38,7 @@ from backend.copilot.tools import TOOL_REGISTRY
|
||||
from backend.copilot.tools.base import BaseTool
|
||||
from backend.util.truncate import truncate
|
||||
|
||||
from .e2b_file_tools import E2B_FILE_TOOL_NAMES, E2B_FILE_TOOLS, _bridge_to_sandbox
|
||||
from .e2b_file_tools import E2B_FILE_TOOL_NAMES, E2B_FILE_TOOLS, bridge_and_annotate
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from e2b import AsyncSandbox
|
||||
@@ -393,9 +393,9 @@ async def _read_file_handler(args: dict[str, Any]) -> dict[str, Any]:
|
||||
text = "".join(selected)
|
||||
sandbox = _current_sandbox.get(None)
|
||||
if sandbox is not None:
|
||||
bridged = await _bridge_to_sandbox(sandbox, resolved, offset, limit)
|
||||
if bridged:
|
||||
text += f"\n[Sandbox copy available at {bridged}]"
|
||||
annotation = await bridge_and_annotate(sandbox, resolved, offset, limit)
|
||||
if annotation:
|
||||
text += annotation
|
||||
return _mcp_ok(text)
|
||||
except FileNotFoundError:
|
||||
return _mcp_err(f"File not found: {file_path}")
|
||||
|
||||
@@ -619,3 +619,95 @@ class TestSDKDisallowedTools:
|
||||
def test_webfetch_tool_is_disallowed(self):
|
||||
"""WebFetch is disallowed due to SSRF risk."""
|
||||
assert "WebFetch" in SDK_DISALLOWED_TOOLS
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _read_file_handler — bridge_and_annotate integration
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestReadFileHandlerBridge:
|
||||
"""Verify that _read_file_handler calls bridge_and_annotate when a sandbox is active."""
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _init_context(self):
|
||||
set_execution_context(
|
||||
user_id="test",
|
||||
session=None, # type: ignore[arg-type]
|
||||
sandbox=None,
|
||||
sdk_cwd="/tmp/copilot-bridge-test",
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bridge_called_when_sandbox_active(self, tmp_path, monkeypatch):
|
||||
"""When a sandbox is set, bridge_and_annotate is called and its annotation appended."""
|
||||
from backend.copilot.context import _current_sandbox
|
||||
|
||||
from .tool_adapter import _read_file_handler
|
||||
|
||||
test_file = tmp_path / "tool-results" / "data.json"
|
||||
test_file.parent.mkdir(parents=True, exist_ok=True)
|
||||
test_file.write_text('{"ok": true}\n')
|
||||
|
||||
monkeypatch.setattr(
|
||||
"backend.copilot.sdk.tool_adapter.is_allowed_local_path",
|
||||
lambda path, cwd: True,
|
||||
)
|
||||
|
||||
fake_sandbox = object()
|
||||
token = _current_sandbox.set(fake_sandbox) # type: ignore[arg-type]
|
||||
try:
|
||||
bridge_calls: list[tuple] = []
|
||||
|
||||
async def fake_bridge_and_annotate(sandbox, file_path, offset, limit):
|
||||
bridge_calls.append((sandbox, file_path, offset, limit))
|
||||
return "\n[Sandbox copy available at /tmp/abc-data.json]"
|
||||
|
||||
monkeypatch.setattr(
|
||||
"backend.copilot.sdk.tool_adapter.bridge_and_annotate",
|
||||
fake_bridge_and_annotate,
|
||||
)
|
||||
|
||||
result = await _read_file_handler(
|
||||
{"file_path": str(test_file), "offset": 0, "limit": 2000}
|
||||
)
|
||||
|
||||
assert result["isError"] is False
|
||||
assert len(bridge_calls) == 1
|
||||
assert bridge_calls[0][0] is fake_sandbox
|
||||
assert "/tmp/abc-data.json" in result["content"][0]["text"]
|
||||
finally:
|
||||
_current_sandbox.reset(token)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bridge_not_called_without_sandbox(self, tmp_path, monkeypatch):
|
||||
"""When no sandbox is set, bridge_and_annotate is not called."""
|
||||
from .tool_adapter import _read_file_handler
|
||||
|
||||
test_file = tmp_path / "tool-results" / "data.json"
|
||||
test_file.parent.mkdir(parents=True, exist_ok=True)
|
||||
test_file.write_text('{"ok": true}\n')
|
||||
|
||||
monkeypatch.setattr(
|
||||
"backend.copilot.sdk.tool_adapter.is_allowed_local_path",
|
||||
lambda path, cwd: True,
|
||||
)
|
||||
|
||||
bridge_calls: list[tuple] = []
|
||||
|
||||
async def fake_bridge_and_annotate(sandbox, file_path, offset, limit):
|
||||
bridge_calls.append((sandbox, file_path, offset, limit))
|
||||
return "\n[Sandbox copy available at /tmp/abc-data.json]"
|
||||
|
||||
monkeypatch.setattr(
|
||||
"backend.copilot.sdk.tool_adapter.bridge_and_annotate",
|
||||
fake_bridge_and_annotate,
|
||||
)
|
||||
|
||||
result = await _read_file_handler(
|
||||
{"file_path": str(test_file), "offset": 0, "limit": 2000}
|
||||
)
|
||||
|
||||
assert result["isError"] is False
|
||||
assert len(bridge_calls) == 0
|
||||
assert "Sandbox copy" not in result["content"][0]["text"]
|
||||
|
||||
Reference in New Issue
Block a user