diff --git a/src/langbot/pkg/box/connector.py b/src/langbot/pkg/box/connector.py index deda0b89..2257910d 100644 --- a/src/langbot/pkg/box/connector.py +++ b/src/langbot/pkg/box/connector.py @@ -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: diff --git a/src/langbot/pkg/box/service.py b/src/langbot/pkg/box/service.py index 6c8e8299..a4ef0dd3 100644 --- a/src/langbot/pkg/box/service.py +++ b/src/langbot/pkg/box/service.py @@ -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. ' diff --git a/src/langbot/pkg/skill/manager.py b/src/langbot/pkg/skill/manager.py index a053697f..ddb2125c 100644 --- a/src/langbot/pkg/skill/manager.py +++ b/src/langbot/pkg/skill/manager.py @@ -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 ' diff --git a/tests/unit_tests/box/test_box_service.py b/tests/unit_tests/box/test_box_service.py index 44f42ec1..c7fe7c61 100644 --- a/tests/unit_tests/box/test_box_service.py +++ b/tests/unit_tests/box/test_box_service.py @@ -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) diff --git a/tests/unit_tests/provider/test_skill_tools.py b/tests/unit_tests/provider/test_skill_tools.py index 00e04bfa..847480c1 100644 --- a/tests/unit_tests/provider/test_skill_tools.py +++ b/tests/unit_tests/provider/test_skill_tools.py @@ -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.