mirror of
https://github.com/langbot-app/LangBot.git
synced 2026-06-11 00:06:04 +00:00
refactor(box): remove legacy in-process runtime code and clean up smells
After the architecture settled on always using an independent Box Runtime service, several pieces of compatibility code and design shortcuts were left behind. This commit cleans them up: - Remove `LocalBoxRuntimeClient` and `create_box_runtime_client` from production code (moved to test-only helper). - Remove unused `_clip_bytes` method from backend. - Remove `__langbot_session_placeholder__` hack by making `BoxSpec.cmd` default to empty and validating non-empty only in `runtime.execute()`. - Extract `get_box_config()` helper to eliminate 5× duplicated config access boilerplate. - Remove `session_id`/`host_path`/`host_path_mode` from the LLM-facing tool schema to enforce request-scoped session isolation. - Fix dual shutdown path: `NativeToolLoader.shutdown()` no longer calls `box_service.shutdown()` (handled by `Application.dispose()`). - Simplify `_assert_session_compatible` with a loop. - Inline client creation in `BoxRuntimeConnector`. - Remove redundant `BOX__RUNTIME_URL` env var from docker-compose (auto-detected by code). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -248,12 +248,6 @@ class CLISandboxBackend(BaseSandboxBackend):
|
||||
timed_out=False,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _clip_bytes(data: bytes, limit: int = _MAX_RAW_OUTPUT_BYTES) -> str:
|
||||
"""Decode bytes to str, discarding bytes beyond *limit*."""
|
||||
clipped = data[:limit]
|
||||
return CLISandboxBackend._clip_captured_bytes(clipped, len(data), limit=limit)
|
||||
|
||||
@staticmethod
|
||||
def _clip_captured_bytes(data: bytes, total_size: int, limit: int = _MAX_RAW_OUTPUT_BYTES) -> str:
|
||||
text = data.decode('utf-8', errors='replace').strip()
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
"""BoxRuntimeClient abstraction for local and remote Box Runtime access."""
|
||||
"""BoxRuntimeClient abstraction for remote Box Runtime access."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -16,8 +16,7 @@ from .errors import (
|
||||
BoxSessionNotFoundError,
|
||||
BoxValidationError,
|
||||
)
|
||||
from .models import BoxExecutionResult, BoxExecutionStatus, BoxSpec
|
||||
from .runtime import BoxRuntime
|
||||
from .models import BoxExecutionResult, BoxExecutionStatus, BoxSpec, get_box_config
|
||||
from ..utils import platform
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -34,9 +33,7 @@ _ERROR_CODE_MAP: dict[str, type[BoxError]] = {
|
||||
|
||||
|
||||
def resolve_box_runtime_url(ap: 'core_app.Application') -> str:
|
||||
box_config = getattr(ap, 'instance_config', None)
|
||||
box_config_data = getattr(box_config, 'data', {}) if box_config is not None else {}
|
||||
runtime_url = str(box_config_data.get('box', {}).get('runtime_url', '')).strip()
|
||||
runtime_url = str(get_box_config(ap).get('runtime_url', '')).strip()
|
||||
if runtime_url:
|
||||
return runtime_url
|
||||
|
||||
@@ -45,16 +42,6 @@ def resolve_box_runtime_url(ap: 'core_app.Application') -> str:
|
||||
return 'http://127.0.0.1:5410'
|
||||
|
||||
|
||||
def create_box_runtime_client(
|
||||
ap: 'core_app.Application',
|
||||
runtime_url: str | None = None,
|
||||
) -> 'RemoteBoxRuntimeClient':
|
||||
return RemoteBoxRuntimeClient(
|
||||
base_url=runtime_url or resolve_box_runtime_url(ap),
|
||||
logger=ap.logger,
|
||||
)
|
||||
|
||||
|
||||
class BoxRuntimeClient(abc.ABC):
|
||||
"""Abstract interface that BoxService uses to talk to a Box Runtime."""
|
||||
|
||||
@@ -83,41 +70,6 @@ class BoxRuntimeClient(abc.ABC):
|
||||
async def create_session(self, spec: BoxSpec) -> dict: ...
|
||||
|
||||
|
||||
class LocalBoxRuntimeClient(BoxRuntimeClient):
|
||||
"""In-process client that wraps a real BoxRuntime directly."""
|
||||
|
||||
def __init__(self, logger: logging.Logger, runtime: BoxRuntime | None = None):
|
||||
self._runtime = runtime or BoxRuntime(logger=logger)
|
||||
|
||||
@property
|
||||
def runtime(self) -> BoxRuntime:
|
||||
return self._runtime
|
||||
|
||||
async def initialize(self) -> None:
|
||||
await self._runtime.initialize()
|
||||
|
||||
async def execute(self, spec: BoxSpec) -> BoxExecutionResult:
|
||||
return await self._runtime.execute(spec)
|
||||
|
||||
async def shutdown(self) -> None:
|
||||
await self._runtime.shutdown()
|
||||
|
||||
async def get_status(self) -> dict:
|
||||
return await self._runtime.get_status()
|
||||
|
||||
async def get_sessions(self) -> list[dict]:
|
||||
return self._runtime.get_sessions()
|
||||
|
||||
async def get_backend_info(self) -> dict:
|
||||
return await self._runtime.get_backend_info()
|
||||
|
||||
async def delete_session(self, session_id: str) -> None:
|
||||
await self._runtime.delete_session(session_id)
|
||||
|
||||
async def create_session(self, spec: BoxSpec) -> dict:
|
||||
return await self._runtime.create_session(spec)
|
||||
|
||||
|
||||
class RemoteBoxRuntimeClient(BoxRuntimeClient):
|
||||
"""HTTP client that talks to a standalone Box Runtime service."""
|
||||
|
||||
|
||||
@@ -6,7 +6,8 @@ import sys
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from .errors import BoxRuntimeUnavailableError
|
||||
from .client import create_box_runtime_client, resolve_box_runtime_url
|
||||
from .client import RemoteBoxRuntimeClient, resolve_box_runtime_url
|
||||
from .models import get_box_config
|
||||
from ..utils import platform
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -24,7 +25,7 @@ class BoxRuntimeConnector:
|
||||
self.configured_runtime_url = self._load_configured_runtime_url()
|
||||
self.runtime_url = self.configured_runtime_url or resolve_box_runtime_url(ap)
|
||||
self.manages_local_runtime = self._should_manage_local_runtime()
|
||||
self.client = create_box_runtime_client(ap, runtime_url=self.runtime_url)
|
||||
self.client = RemoteBoxRuntimeClient(base_url=self.runtime_url, logger=ap.logger)
|
||||
self.runtime_subprocess: asyncio.subprocess.Process | None = None
|
||||
self.runtime_subprocess_task: asyncio.Task | None = None
|
||||
|
||||
@@ -54,9 +55,7 @@ class BoxRuntimeConnector:
|
||||
self.runtime_subprocess_task = None
|
||||
|
||||
def _load_configured_runtime_url(self) -> str:
|
||||
box_config = getattr(self.ap, 'instance_config', None)
|
||||
box_config_data = getattr(box_config, 'data', {}) if box_config is not None else {}
|
||||
return str(box_config_data.get('box', {}).get('runtime_url', '')).strip()
|
||||
return str(get_box_config(self.ap).get('runtime_url', '')).strip()
|
||||
|
||||
def _should_manage_local_runtime(self) -> bool:
|
||||
return not self.configured_runtime_url and platform.get_platform() != 'docker'
|
||||
|
||||
@@ -10,6 +10,13 @@ DEFAULT_BOX_IMAGE = 'python:3.11-slim'
|
||||
DEFAULT_BOX_MOUNT_PATH = '/workspace'
|
||||
|
||||
|
||||
def get_box_config(ap) -> dict:
|
||||
"""Return the 'box' section from instance config, with safe fallbacks."""
|
||||
instance_config = getattr(ap, 'instance_config', None)
|
||||
config_data = getattr(instance_config, 'data', {}) if instance_config is not None else {}
|
||||
return config_data.get('box', {})
|
||||
|
||||
|
||||
class BoxNetworkMode(str, enum.Enum):
|
||||
OFF = 'off'
|
||||
ON = 'on'
|
||||
@@ -26,7 +33,7 @@ class BoxHostMountMode(str, enum.Enum):
|
||||
|
||||
|
||||
class BoxSpec(pydantic.BaseModel):
|
||||
cmd: str
|
||||
cmd: str = ''
|
||||
workdir: str = '/workspace'
|
||||
timeout_sec: int = 30
|
||||
network: BoxNetworkMode = BoxNetworkMode.OFF
|
||||
@@ -44,10 +51,7 @@ class BoxSpec(pydantic.BaseModel):
|
||||
@pydantic.field_validator('cmd')
|
||||
@classmethod
|
||||
def validate_cmd(cls, value: str) -> str:
|
||||
value = value.strip()
|
||||
if not value:
|
||||
raise ValueError('cmd must not be empty')
|
||||
return value
|
||||
return value.strip()
|
||||
|
||||
@pydantic.field_validator('workdir')
|
||||
@classmethod
|
||||
|
||||
@@ -6,7 +6,7 @@ import datetime as dt
|
||||
import logging
|
||||
|
||||
from .backend import BaseSandboxBackend, DockerBackend, PodmanBackend
|
||||
from .errors import BoxBackendUnavailableError, BoxSessionConflictError, BoxSessionNotFoundError
|
||||
from .errors import BoxBackendUnavailableError, BoxSessionConflictError, BoxSessionNotFoundError, BoxValidationError
|
||||
from .models import BoxExecutionResult, BoxExecutionStatus, BoxSessionInfo, BoxSpec
|
||||
|
||||
_UTC = dt.timezone.utc
|
||||
@@ -36,6 +36,8 @@ class BoxRuntime:
|
||||
self._backend = await self._select_backend()
|
||||
|
||||
async def execute(self, spec: BoxSpec) -> BoxExecutionResult:
|
||||
if not spec.cmd:
|
||||
raise BoxValidationError('cmd must not be empty')
|
||||
session = await self._get_or_create_session(spec)
|
||||
|
||||
async with session.lock:
|
||||
@@ -183,38 +185,18 @@ class BoxRuntime:
|
||||
self.logger.warning(f'Failed to clean up box session {session_id}: {exc}')
|
||||
|
||||
def _assert_session_compatible(self, session: BoxSessionInfo, spec: BoxSpec):
|
||||
if session.network != spec.network:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with network={session.network.value}'
|
||||
)
|
||||
if session.image != spec.image:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with image={session.image}'
|
||||
)
|
||||
if session.host_path != spec.host_path:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with host_path={session.host_path}'
|
||||
)
|
||||
if session.host_path_mode != spec.host_path_mode:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with host_path_mode={session.host_path_mode.value}'
|
||||
)
|
||||
if session.cpus != spec.cpus:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with cpus={session.cpus}'
|
||||
)
|
||||
if session.memory_mb != spec.memory_mb:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with memory_mb={session.memory_mb}'
|
||||
)
|
||||
if session.pids_limit != spec.pids_limit:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with pids_limit={session.pids_limit}'
|
||||
)
|
||||
if session.read_only_rootfs != spec.read_only_rootfs:
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with read_only_rootfs={session.read_only_rootfs}'
|
||||
)
|
||||
_COMPAT_FIELDS = (
|
||||
'network', 'image', 'host_path', 'host_path_mode',
|
||||
'cpus', 'memory_mb', 'pids_limit', 'read_only_rootfs',
|
||||
)
|
||||
for field in _COMPAT_FIELDS:
|
||||
session_val = getattr(session, field)
|
||||
spec_val = getattr(spec, field)
|
||||
if session_val != spec_val:
|
||||
display = session_val.value if hasattr(session_val, 'value') else session_val
|
||||
raise BoxSessionConflictError(
|
||||
f'sandbox_exec session {spec.session_id} already exists with {field}={display}'
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _session_to_dict(info: BoxSessionInfo) -> dict:
|
||||
|
||||
@@ -81,7 +81,6 @@ async def handle_create_session(request: web.Request) -> web.Response:
|
||||
body = await request.json()
|
||||
session_id = request.match_info['session_id']
|
||||
body['session_id'] = session_id
|
||||
body.setdefault('cmd', '__langbot_session_placeholder__')
|
||||
spec = BoxSpec.model_validate(body)
|
||||
session_info = await runtime.create_session(spec)
|
||||
return web.json_response(session_info, status=201)
|
||||
|
||||
@@ -12,7 +12,7 @@ import pydantic
|
||||
from .client import BoxRuntimeClient
|
||||
from .connector import BoxRuntimeConnector
|
||||
from .errors import BoxError, BoxValidationError
|
||||
from .models import BUILTIN_PROFILES, BoxExecutionResult, BoxProfile, BoxSpec
|
||||
from .models import BUILTIN_PROFILES, BoxExecutionResult, BoxProfile, BoxSpec, get_box_config
|
||||
|
||||
_INT_ADAPTER = pydantic.TypeAdapter(int)
|
||||
_UTC = _dt.timezone.utc
|
||||
@@ -189,9 +189,7 @@ class BoxService:
|
||||
}
|
||||
|
||||
def _load_allowed_host_mount_roots(self) -> list[str]:
|
||||
box_config = getattr(self.ap, 'instance_config', None)
|
||||
box_config_data = getattr(box_config, 'data', {}) if box_config is not None else {}
|
||||
configured_roots = box_config_data.get('box', {}).get('allowed_host_mount_roots', [])
|
||||
configured_roots = get_box_config(self.ap).get('allowed_host_mount_roots', [])
|
||||
|
||||
normalized_roots: list[str] = []
|
||||
for root in configured_roots:
|
||||
@@ -203,9 +201,7 @@ class BoxService:
|
||||
return normalized_roots
|
||||
|
||||
def _load_default_host_workspace(self) -> str | None:
|
||||
box_config = getattr(self.ap, 'instance_config', None)
|
||||
box_config_data = getattr(box_config, 'data', {}) if box_config is not None else {}
|
||||
default_host_workspace = str(box_config_data.get('box', {}).get('default_host_workspace', '')).strip()
|
||||
default_host_workspace = str(get_box_config(self.ap).get('default_host_workspace', '')).strip()
|
||||
if not default_host_workspace:
|
||||
return None
|
||||
return os.path.realpath(os.path.abspath(default_host_workspace))
|
||||
@@ -252,9 +248,7 @@ class BoxService:
|
||||
raise BoxValidationError(f'host_path is outside allowed_host_mount_roots: {allowed_roots}')
|
||||
|
||||
def _load_profile(self) -> BoxProfile:
|
||||
box_config = getattr(self.ap, 'instance_config', None)
|
||||
box_config_data = getattr(box_config, 'data', {}) if box_config is not None else {}
|
||||
profile_name = str(box_config_data.get('box', {}).get('profile', 'default')).strip() or 'default'
|
||||
profile_name = str(get_box_config(self.ap).get('profile', 'default')).strip() or 'default'
|
||||
|
||||
profile = BUILTIN_PROFILES.get(profile_name)
|
||||
if profile is None:
|
||||
|
||||
@@ -28,8 +28,7 @@ class NativeToolLoader(loader.ToolLoader):
|
||||
return await self.ap.box_service.execute_sandbox_tool(parameters, query)
|
||||
|
||||
async def shutdown(self):
|
||||
if getattr(self.ap, 'box_service', None) is not None:
|
||||
await self.ap.box_service.shutdown()
|
||||
pass
|
||||
|
||||
def _build_sandbox_exec_tool(self) -> resource_tool.LLMTool:
|
||||
return resource_tool.LLMTool(
|
||||
@@ -64,23 +63,6 @@ class NativeToolLoader(loader.ToolLoader):
|
||||
'enum': ['off', 'on'],
|
||||
'default': 'off',
|
||||
},
|
||||
'session_id': {
|
||||
'type': 'string',
|
||||
'description': 'Optional sandbox session id. Defaults to the current request id for reuse.',
|
||||
},
|
||||
'host_path': {
|
||||
'type': 'string',
|
||||
'description': (
|
||||
'Optional absolute host directory path to mount into the sandbox as /workspace. '
|
||||
'The path must be under an allowed host mount root.'
|
||||
),
|
||||
},
|
||||
'host_path_mode': {
|
||||
'type': 'string',
|
||||
'description': 'Mount mode for host_path. Use rw to create or modify host files.',
|
||||
'enum': ['ro', 'rw'],
|
||||
'default': 'rw',
|
||||
},
|
||||
'env': {
|
||||
'type': 'object',
|
||||
'description': 'Optional environment variables to expose inside the sandbox.',
|
||||
|
||||
Reference in New Issue
Block a user