Compare commits

..

1 Commits

Author SHA1 Message Date
RockChinQ
882c9ae8f5 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.
2026-06-07 12:46:52 -04:00
5 changed files with 205 additions and 16 deletions

View File

@@ -120,13 +120,19 @@ class BoxRuntimeConnector(ManagedRuntimeConnector):
self._relay_port = parsed.port or _DEFAULT_PORT
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.
True when:
- Running inside Docker (Box runtime is a separate container)
- The ``--standalone-box`` CLI flag was passed
- 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(
self.configured_runtime_endpoint
@@ -134,6 +140,10 @@ class BoxRuntimeConnector(ManagedRuntimeConnector):
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:
if self._uses_websocket():
if platform.get_platform() == 'win32' and not self.configured_runtime_endpoint:

View File

@@ -67,6 +67,10 @@ class BoxService:
self._available = False
self._connector_error: str = ''
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
def enabled(self) -> bool:
@@ -148,6 +152,32 @@ class BoxService:
def available(self) -> bool:
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(
self,
spec_payload: dict,
@@ -220,14 +250,24 @@ class BoxService:
all skill packages mounted, regardless of which skill is currently
activated.
Skills whose ``package_root`` is missing or no longer a directory on
the LangBot-visible filesystem are skipped with a warning instead of
being passed through to the backend. Without this guard the three
backends behave inconsistently on a stale mount: nsjail refuses to
start the sandbox (failing every exec in the session), Docker
silently auto-creates a root-owned empty directory on the host, and
E2B silently skips the upload — none of which surfaces an
actionable error to the agent or operator.
Path validation is filesystem-topology dependent. When LangBot and the
Box runtime share a filesystem (local stdio mode), a skill whose
``package_root`` is missing or no longer a directory is skipped with a
warning instead of being passed through to the backend. Without that
guard the three backends behave inconsistently on a stale mount: nsjail
refuses to start the sandbox (failing every exec in the session),
Docker silently auto-creates a root-owned empty directory on the host,
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)
if skill_mgr is None:
@@ -235,13 +275,15 @@ class BoxService:
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)
mounts: list[dict] = []
for skill_name, skill_data in visible_skills.items():
package_root = str(skill_data.get('package_root', '') or '').strip()
if not package_root:
continue
if not os.path.isdir(package_root):
if validate_locally and not os.path.isdir(package_root):
self.ap.logger.warning(
f'Skill "{skill_name}" package_root missing on filesystem '
f'({package_root}); skipping mount to prevent sandbox failures. '

View File

@@ -46,6 +46,13 @@ class SkillManager:
self.ap.logger.info('Box runtime unavailable; skill cache is empty.')
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:
dropped = 0
for skill_data in await box_service.list_skills():
@@ -53,7 +60,7 @@ class SkillManager:
if not skill_name:
continue
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(
f'Skill "{skill_name}" reported by Box runtime but '
f'package_root missing on LangBot filesystem '

View File

@@ -190,6 +190,66 @@ async def test_box_service_without_explicit_client_initializes_internal_connecto
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
async def test_box_service_get_sessions_delegates_to_client():
client = Mock()
@@ -1342,11 +1402,16 @@ class TestBuildSkillExtraMounts:
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.skill_mgr = SimpleNamespace(skills=skills)
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):
logger = Mock()
@@ -1373,6 +1438,30 @@ class TestBuildSkillExtraMounts:
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):
logger = Mock()
skills = {
@@ -1383,6 +1472,14 @@ class TestBuildSkillExtraMounts:
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):
logger = Mock()
app = make_app(logger)

View File

@@ -62,15 +62,17 @@ class TestSkillManagerCache:
@pytest.mark.asyncio
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
LangBot-visible filesystem, the cache must drop it instead of
keeping a stale entry that would later produce a bad mount."""
"""When LangBot shares a filesystem with Box (local stdio mode) and Box
reports a skill whose package_root is gone from that shared filesystem,
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
with tempfile.TemporaryDirectory() as live_dir:
ghost_dir = os.path.join(live_dir, '_does_not_exist')
box_service = SimpleNamespace(
available=True,
shares_filesystem_with_box=True,
list_skills=AsyncMock(
return_value=[
_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]
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:
"""Skill activation is now Tool-Call based.