mirror of
https://github.com/langbot-app/LangBot.git
synced 2026-07-25 13:56:08 +00:00
fix(box): trust Box-reported skill paths when filesystem is not shared
In separated deployments (Docker Compose, k8s sidecar, --standalone-box, remote runtime.endpoint) the Box runtime owns its own filesystem, so the skill package_root it reports via list_skills is not resolvable on the LangBot side. LangBot's reload_skills and build_skill_extra_mounts validated those paths with os.path.isdir() against its own filesystem, which silently dropped every skill in such deployments — breaking the sandbox skill feature for the nsjail/SaaS backend. Add BoxService.shares_filesystem_with_box, derived from the connector transport (stdio = shared, WebSocket = separated), with an explicit override seam for tests/embedders. Gate both isdir() guards on it: keep local validation in shared-fs stdio mode, trust Box-reported paths otherwise. The Box runtime only reports skills found on its own filesystem, so those paths are valid there by construction. Adds topology-derivation tests (real connector, no mocks) and skill-retention tests for both shared and separated filesystems.
This commit is contained in:
@@ -120,13 +120,19 @@ class BoxRuntimeConnector(ManagedRuntimeConnector):
|
|||||||
self._relay_port = parsed.port or _DEFAULT_PORT
|
self._relay_port = parsed.port or _DEFAULT_PORT
|
||||||
self._filtered_box_config = _filter_config_for_runtime(_get_box_config(ap))
|
self._filtered_box_config = _filter_config_for_runtime(_get_box_config(ap))
|
||||||
|
|
||||||
def _uses_websocket(self) -> bool:
|
def uses_websocket(self) -> bool:
|
||||||
"""Whether the connector should use WebSocket to reach the Box runtime.
|
"""Whether the connector should use WebSocket to reach the Box runtime.
|
||||||
|
|
||||||
True when:
|
True when:
|
||||||
- Running inside Docker (Box runtime is a separate container)
|
- Running inside Docker (Box runtime is a separate container)
|
||||||
- The ``--standalone-box`` CLI flag was passed
|
- The ``--standalone-box`` CLI flag was passed
|
||||||
- An explicit ``runtime.endpoint`` was configured
|
- An explicit ``runtime.endpoint`` was configured
|
||||||
|
|
||||||
|
When this is True the Box runtime lives in a separate process with its
|
||||||
|
own filesystem view (container, pod sidecar, or remote host), so paths
|
||||||
|
it reports (e.g. skill ``package_root``) are NOT resolvable on the
|
||||||
|
LangBot side. When False, Box runs as a stdio child process that shares
|
||||||
|
LangBot's filesystem.
|
||||||
"""
|
"""
|
||||||
return bool(
|
return bool(
|
||||||
self.configured_runtime_endpoint
|
self.configured_runtime_endpoint
|
||||||
@@ -134,6 +140,10 @@ class BoxRuntimeConnector(ManagedRuntimeConnector):
|
|||||||
or platform.use_websocket_to_connect_box_runtime()
|
or platform.use_websocket_to_connect_box_runtime()
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# Backwards-compatible private alias.
|
||||||
|
def _uses_websocket(self) -> bool:
|
||||||
|
return self.uses_websocket()
|
||||||
|
|
||||||
async def initialize(self) -> None:
|
async def initialize(self) -> None:
|
||||||
if self._uses_websocket():
|
if self._uses_websocket():
|
||||||
if platform.get_platform() == 'win32' and not self.configured_runtime_endpoint:
|
if platform.get_platform() == 'win32' and not self.configured_runtime_endpoint:
|
||||||
|
|||||||
@@ -67,6 +67,10 @@ class BoxService:
|
|||||||
self._available = False
|
self._available = False
|
||||||
self._connector_error: str = ''
|
self._connector_error: str = ''
|
||||||
self._reconnecting = False
|
self._reconnecting = False
|
||||||
|
# Optional explicit override for shares_filesystem_with_box. None means
|
||||||
|
# "derive from the connector transport". Set by tests / embedders that
|
||||||
|
# know the real LangBot<->Box filesystem topology.
|
||||||
|
self._shares_filesystem_with_box_override: bool | None = None
|
||||||
|
|
||||||
@property
|
@property
|
||||||
def enabled(self) -> bool:
|
def enabled(self) -> bool:
|
||||||
@@ -148,6 +152,32 @@ class BoxService:
|
|||||||
def available(self) -> bool:
|
def available(self) -> bool:
|
||||||
return self._available
|
return self._available
|
||||||
|
|
||||||
|
@property
|
||||||
|
def shares_filesystem_with_box(self) -> bool:
|
||||||
|
"""Whether LangBot and the Box runtime share a filesystem view.
|
||||||
|
|
||||||
|
This is True only when Box runs as a local stdio child process of
|
||||||
|
LangBot (same container/host). In that case paths the Box runtime
|
||||||
|
reports — notably skill ``package_root`` — resolve identically on the
|
||||||
|
LangBot side, so LangBot may validate them against its own filesystem.
|
||||||
|
|
||||||
|
It is False for every separated deployment (Docker Compose, k8s
|
||||||
|
sidecar, ``--standalone-box``, or an explicit ``runtime.endpoint``),
|
||||||
|
where the Box runtime owns its own filesystem and LangBot must trust
|
||||||
|
the paths it reports rather than checking them locally.
|
||||||
|
|
||||||
|
When Box is wired up with an injected client (tests, custom embeds)
|
||||||
|
there is no connector to introspect; we conservatively report False so
|
||||||
|
LangBot never wrongly drops Box-reported skills. An explicit override
|
||||||
|
can be set via ``_shares_filesystem_with_box`` (used by tests and any
|
||||||
|
embedder that knows the real topology).
|
||||||
|
"""
|
||||||
|
if self._shares_filesystem_with_box_override is not None:
|
||||||
|
return self._shares_filesystem_with_box_override
|
||||||
|
if self._runtime_connector is None:
|
||||||
|
return False
|
||||||
|
return not self._runtime_connector.uses_websocket()
|
||||||
|
|
||||||
async def execute_spec_payload(
|
async def execute_spec_payload(
|
||||||
self,
|
self,
|
||||||
spec_payload: dict,
|
spec_payload: dict,
|
||||||
@@ -220,14 +250,24 @@ class BoxService:
|
|||||||
all skill packages mounted, regardless of which skill is currently
|
all skill packages mounted, regardless of which skill is currently
|
||||||
activated.
|
activated.
|
||||||
|
|
||||||
Skills whose ``package_root`` is missing or no longer a directory on
|
Path validation is filesystem-topology dependent. When LangBot and the
|
||||||
the LangBot-visible filesystem are skipped with a warning instead of
|
Box runtime share a filesystem (local stdio mode), a skill whose
|
||||||
being passed through to the backend. Without this guard the three
|
``package_root`` is missing or no longer a directory is skipped with a
|
||||||
backends behave inconsistently on a stale mount: nsjail refuses to
|
warning instead of being passed through to the backend. Without that
|
||||||
start the sandbox (failing every exec in the session), Docker
|
guard the three backends behave inconsistently on a stale mount: nsjail
|
||||||
silently auto-creates a root-owned empty directory on the host, and
|
refuses to start the sandbox (failing every exec in the session),
|
||||||
E2B silently skips the upload — none of which surfaces an
|
Docker silently auto-creates a root-owned empty directory on the host,
|
||||||
actionable error to the agent or operator.
|
and E2B silently skips the upload — none of which surfaces an
|
||||||
|
actionable error.
|
||||||
|
|
||||||
|
When Box runs as a separate process (Docker Compose, k8s sidecar,
|
||||||
|
``--standalone-box``, or a remote ``runtime.endpoint``), the
|
||||||
|
``package_root`` reported by ``list_skills`` is the Box runtime's own
|
||||||
|
filesystem path and is NOT resolvable on the LangBot side. Validating
|
||||||
|
it locally would wrongly drop every skill, so LangBot trusts the path
|
||||||
|
and lets the Box runtime resolve it. The Box runtime only ever reports
|
||||||
|
skills it discovered on its own filesystem, so the path is valid there
|
||||||
|
by construction.
|
||||||
"""
|
"""
|
||||||
skill_mgr = getattr(self.ap, 'skill_mgr', None)
|
skill_mgr = getattr(self.ap, 'skill_mgr', None)
|
||||||
if skill_mgr is None:
|
if skill_mgr is None:
|
||||||
@@ -235,13 +275,15 @@ class BoxService:
|
|||||||
|
|
||||||
from ..provider.tools.loaders import skill as skill_loader
|
from ..provider.tools.loaders import skill as skill_loader
|
||||||
|
|
||||||
|
validate_locally = self.shares_filesystem_with_box
|
||||||
|
|
||||||
visible_skills = skill_loader.get_visible_skills(self.ap, query)
|
visible_skills = skill_loader.get_visible_skills(self.ap, query)
|
||||||
mounts: list[dict] = []
|
mounts: list[dict] = []
|
||||||
for skill_name, skill_data in visible_skills.items():
|
for skill_name, skill_data in visible_skills.items():
|
||||||
package_root = str(skill_data.get('package_root', '') or '').strip()
|
package_root = str(skill_data.get('package_root', '') or '').strip()
|
||||||
if not package_root:
|
if not package_root:
|
||||||
continue
|
continue
|
||||||
if not os.path.isdir(package_root):
|
if validate_locally and not os.path.isdir(package_root):
|
||||||
self.ap.logger.warning(
|
self.ap.logger.warning(
|
||||||
f'Skill "{skill_name}" package_root missing on filesystem '
|
f'Skill "{skill_name}" package_root missing on filesystem '
|
||||||
f'({package_root}); skipping mount to prevent sandbox failures. '
|
f'({package_root}); skipping mount to prevent sandbox failures. '
|
||||||
|
|||||||
@@ -46,6 +46,13 @@ class SkillManager:
|
|||||||
self.ap.logger.info('Box runtime unavailable; skill cache is empty.')
|
self.ap.logger.info('Box runtime unavailable; skill cache is empty.')
|
||||||
return
|
return
|
||||||
|
|
||||||
|
# LangBot may only validate Box-reported paths against its own
|
||||||
|
# filesystem when the two share one (local stdio mode). In separated
|
||||||
|
# deployments (Docker Compose, k8s sidecar, --standalone-box, remote
|
||||||
|
# endpoint) the package_root lives on the Box runtime's filesystem and
|
||||||
|
# is not resolvable here, so we trust what Box reports.
|
||||||
|
validate_locally = bool(getattr(box_service, 'shares_filesystem_with_box', False))
|
||||||
|
|
||||||
try:
|
try:
|
||||||
dropped = 0
|
dropped = 0
|
||||||
for skill_data in await box_service.list_skills():
|
for skill_data in await box_service.list_skills():
|
||||||
@@ -53,7 +60,7 @@ class SkillManager:
|
|||||||
if not skill_name:
|
if not skill_name:
|
||||||
continue
|
continue
|
||||||
package_root = str(skill_data.get('package_root', '') or '').strip()
|
package_root = str(skill_data.get('package_root', '') or '').strip()
|
||||||
if package_root and not os.path.isdir(package_root):
|
if validate_locally and package_root and not os.path.isdir(package_root):
|
||||||
self.ap.logger.warning(
|
self.ap.logger.warning(
|
||||||
f'Skill "{skill_name}" reported by Box runtime but '
|
f'Skill "{skill_name}" reported by Box runtime but '
|
||||||
f'package_root missing on LangBot filesystem '
|
f'package_root missing on LangBot filesystem '
|
||||||
|
|||||||
@@ -190,6 +190,66 @@ async def test_box_service_without_explicit_client_initializes_internal_connecto
|
|||||||
connector.initialize.assert_awaited_once()
|
connector.initialize.assert_awaited_once()
|
||||||
|
|
||||||
|
|
||||||
|
class TestSharesFilesystemWithBox:
|
||||||
|
"""``shares_filesystem_with_box`` must reflect the real LangBot<->Box
|
||||||
|
filesystem topology, which is derived from the connector transport:
|
||||||
|
|
||||||
|
- stdio (local child process) → shared filesystem → True
|
||||||
|
- WebSocket (Docker / sidecar / --standalone-box / remote) → separated → False
|
||||||
|
|
||||||
|
This drives whether LangBot validates Box-reported skill paths locally.
|
||||||
|
Getting it wrong silently drops every skill in separated deployments.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def test_true_for_stdio_connector(self, monkeypatch: pytest.MonkeyPatch):
|
||||||
|
# Non-Docker Unix, no endpoint, not standalone → stdio transport.
|
||||||
|
monkeypatch.setattr('langbot.pkg.utils.platform.get_platform', lambda: 'linux')
|
||||||
|
monkeypatch.setattr('langbot.pkg.utils.platform.standalone_box', False)
|
||||||
|
|
||||||
|
service = BoxService(make_app(Mock()))
|
||||||
|
|
||||||
|
assert service._runtime_connector is not None
|
||||||
|
assert service._runtime_connector.uses_websocket() is False
|
||||||
|
assert service.shares_filesystem_with_box is True
|
||||||
|
|
||||||
|
def test_false_for_websocket_connector_via_endpoint(self, monkeypatch: pytest.MonkeyPatch):
|
||||||
|
monkeypatch.setattr('langbot.pkg.utils.platform.get_platform', lambda: 'linux')
|
||||||
|
monkeypatch.setattr('langbot.pkg.utils.platform.standalone_box', False)
|
||||||
|
app = make_app(Mock())
|
||||||
|
app.instance_config.data['box']['runtime']['endpoint'] = 'ws://pod-x-box:5410'
|
||||||
|
|
||||||
|
service = BoxService(app)
|
||||||
|
|
||||||
|
assert service._runtime_connector is not None
|
||||||
|
assert service._runtime_connector.uses_websocket() is True
|
||||||
|
assert service.shares_filesystem_with_box is False
|
||||||
|
|
||||||
|
def test_false_for_websocket_connector_in_docker(self, monkeypatch: pytest.MonkeyPatch):
|
||||||
|
monkeypatch.setattr('langbot.pkg.utils.platform.get_platform', lambda: 'docker')
|
||||||
|
monkeypatch.setattr('langbot.pkg.utils.platform.standalone_box', False)
|
||||||
|
|
||||||
|
service = BoxService(make_app(Mock()))
|
||||||
|
|
||||||
|
assert service.shares_filesystem_with_box is False
|
||||||
|
|
||||||
|
def test_false_when_client_injected_without_connector(self):
|
||||||
|
# Injected client (no connector) → unknown topology → conservative False
|
||||||
|
# so LangBot never wrongly drops Box-reported skills.
|
||||||
|
service = BoxService(make_app(Mock()), client=Mock(spec=BoxRuntimeClient))
|
||||||
|
|
||||||
|
assert service._runtime_connector is None
|
||||||
|
assert service.shares_filesystem_with_box is False
|
||||||
|
|
||||||
|
def test_explicit_override_wins(self):
|
||||||
|
service = BoxService(make_app(Mock()), client=Mock(spec=BoxRuntimeClient))
|
||||||
|
|
||||||
|
service._shares_filesystem_with_box_override = True
|
||||||
|
assert service.shares_filesystem_with_box is True
|
||||||
|
|
||||||
|
service._shares_filesystem_with_box_override = False
|
||||||
|
assert service.shares_filesystem_with_box is False
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_box_service_get_sessions_delegates_to_client():
|
async def test_box_service_get_sessions_delegates_to_client():
|
||||||
client = Mock()
|
client = Mock()
|
||||||
@@ -1342,11 +1402,16 @@ class TestBuildSkillExtraMounts:
|
|||||||
the backend never sees a bad mount.
|
the backend never sees a bad mount.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def _make_service(self, logger, skills):
|
def _make_service(self, logger, skills, *, shares_filesystem=True):
|
||||||
app = make_app(logger)
|
app = make_app(logger)
|
||||||
app.skill_mgr = SimpleNamespace(skills=skills)
|
app.skill_mgr = SimpleNamespace(skills=skills)
|
||||||
client = Mock(spec=BoxRuntimeClient)
|
client = Mock(spec=BoxRuntimeClient)
|
||||||
return BoxService(app, client=client)
|
service = BoxService(app, client=client)
|
||||||
|
# Tests construct BoxService with an injected client (no connector), so
|
||||||
|
# set the topology explicitly. Most cases exercise the shared-fs (local
|
||||||
|
# stdio) path where local package_root validation applies.
|
||||||
|
service._shares_filesystem_with_box_override = shares_filesystem
|
||||||
|
return service
|
||||||
|
|
||||||
def test_skips_skill_with_missing_package_root(self):
|
def test_skips_skill_with_missing_package_root(self):
|
||||||
logger = Mock()
|
logger = Mock()
|
||||||
@@ -1373,6 +1438,30 @@ class TestBuildSkillExtraMounts:
|
|||||||
for call in logger.warning.call_args_list
|
for call in logger.warning.call_args_list
|
||||||
)
|
)
|
||||||
|
|
||||||
|
def test_trusts_box_paths_when_filesystem_not_shared(self):
|
||||||
|
"""In separated deployments (Docker Compose, k8s sidecar,
|
||||||
|
--standalone-box, remote endpoint) the Box runtime owns its own
|
||||||
|
filesystem. package_root values it reports are NOT resolvable on the
|
||||||
|
LangBot side, so LangBot must trust them rather than dropping every
|
||||||
|
skill via a local isdir() check."""
|
||||||
|
logger = Mock()
|
||||||
|
skills = {
|
||||||
|
'a': {'name': 'a', 'package_root': '/box/skills/a'},
|
||||||
|
'b': {'name': 'b', 'package_root': '/box/skills/b'},
|
||||||
|
}
|
||||||
|
service = self._make_service(logger, skills, shares_filesystem=False)
|
||||||
|
|
||||||
|
mounts = service.build_skill_extra_mounts(make_query())
|
||||||
|
|
||||||
|
assert mounts == [
|
||||||
|
{'host_path': '/box/skills/a', 'mount_path': '/workspace/.skills/a', 'mode': 'rw'},
|
||||||
|
{'host_path': '/box/skills/b', 'mount_path': '/workspace/.skills/b', 'mode': 'rw'},
|
||||||
|
]
|
||||||
|
# No skill is dropped, so no "missing" warning should be logged.
|
||||||
|
assert not any(
|
||||||
|
'package_root missing' in str(call.args[0]) for call in logger.warning.call_args_list
|
||||||
|
)
|
||||||
|
|
||||||
def test_skips_skill_with_empty_package_root(self):
|
def test_skips_skill_with_empty_package_root(self):
|
||||||
logger = Mock()
|
logger = Mock()
|
||||||
skills = {
|
skills = {
|
||||||
@@ -1383,6 +1472,14 @@ class TestBuildSkillExtraMounts:
|
|||||||
|
|
||||||
assert service.build_skill_extra_mounts(make_query()) == []
|
assert service.build_skill_extra_mounts(make_query()) == []
|
||||||
|
|
||||||
|
def test_empty_package_root_skipped_even_when_not_shared(self):
|
||||||
|
"""An empty package_root is always invalid regardless of topology."""
|
||||||
|
logger = Mock()
|
||||||
|
skills = {'no_root': {'name': 'no_root', 'package_root': ''}}
|
||||||
|
service = self._make_service(logger, skills, shares_filesystem=False)
|
||||||
|
|
||||||
|
assert service.build_skill_extra_mounts(make_query()) == []
|
||||||
|
|
||||||
def test_returns_empty_when_no_skill_manager(self):
|
def test_returns_empty_when_no_skill_manager(self):
|
||||||
logger = Mock()
|
logger = Mock()
|
||||||
app = make_app(logger)
|
app = make_app(logger)
|
||||||
|
|||||||
@@ -62,15 +62,17 @@ class TestSkillManagerCache:
|
|||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_reload_skills_drops_box_skills_with_missing_package_root(self):
|
async def test_reload_skills_drops_box_skills_with_missing_package_root(self):
|
||||||
"""When Box reports a skill whose package_root is gone from the
|
"""When LangBot shares a filesystem with Box (local stdio mode) and Box
|
||||||
LangBot-visible filesystem, the cache must drop it instead of
|
reports a skill whose package_root is gone from that shared filesystem,
|
||||||
keeping a stale entry that would later produce a bad mount."""
|
the cache must drop it instead of keeping a stale entry that would later
|
||||||
|
produce a bad mount."""
|
||||||
from langbot.pkg.skill.manager import SkillManager
|
from langbot.pkg.skill.manager import SkillManager
|
||||||
|
|
||||||
with tempfile.TemporaryDirectory() as live_dir:
|
with tempfile.TemporaryDirectory() as live_dir:
|
||||||
ghost_dir = os.path.join(live_dir, '_does_not_exist')
|
ghost_dir = os.path.join(live_dir, '_does_not_exist')
|
||||||
box_service = SimpleNamespace(
|
box_service = SimpleNamespace(
|
||||||
available=True,
|
available=True,
|
||||||
|
shares_filesystem_with_box=True,
|
||||||
list_skills=AsyncMock(
|
list_skills=AsyncMock(
|
||||||
return_value=[
|
return_value=[
|
||||||
_make_skill_data(name='alive', package_root=live_dir),
|
_make_skill_data(name='alive', package_root=live_dir),
|
||||||
@@ -90,6 +92,37 @@ class TestSkillManagerCache:
|
|||||||
warning_messages = [str(call.args[0]) for call in ap.logger.warning.call_args_list]
|
warning_messages = [str(call.args[0]) for call in ap.logger.warning.call_args_list]
|
||||||
assert any('ghost' in msg and 'package_root missing' in msg for msg in warning_messages)
|
assert any('ghost' in msg and 'package_root missing' in msg for msg in warning_messages)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_reload_skills_trusts_box_paths_when_filesystem_not_shared(self):
|
||||||
|
"""In separated deployments (Docker Compose, k8s sidecar,
|
||||||
|
--standalone-box, remote endpoint) the package_root reported by Box
|
||||||
|
lives on the Box runtime's filesystem and is not resolvable on the
|
||||||
|
LangBot side. The cache must keep every Box-reported skill rather than
|
||||||
|
dropping them all via a local isdir() check."""
|
||||||
|
from langbot.pkg.skill.manager import SkillManager
|
||||||
|
|
||||||
|
box_service = SimpleNamespace(
|
||||||
|
available=True,
|
||||||
|
shares_filesystem_with_box=False,
|
||||||
|
list_skills=AsyncMock(
|
||||||
|
return_value=[
|
||||||
|
_make_skill_data(name='alpha', package_root='/box/skills/alpha'),
|
||||||
|
_make_skill_data(name='beta', package_root='/box/skills/beta'),
|
||||||
|
]
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
|
ap = _make_ap()
|
||||||
|
ap.box_service = box_service
|
||||||
|
mgr = SkillManager(ap)
|
||||||
|
|
||||||
|
await mgr.reload_skills()
|
||||||
|
|
||||||
|
assert sorted(mgr.skills) == ['alpha', 'beta']
|
||||||
|
# No skill dropped → no "package_root missing" warning.
|
||||||
|
warning_messages = [str(call.args[0]) for call in ap.logger.warning.call_args_list]
|
||||||
|
assert not any('package_root missing' in msg for msg in warning_messages)
|
||||||
|
|
||||||
|
|
||||||
class TestSkillActivationHelper:
|
class TestSkillActivationHelper:
|
||||||
"""Skill activation is now Tool-Call based.
|
"""Skill activation is now Tool-Call based.
|
||||||
|
|||||||
Reference in New Issue
Block a user