fix: address code review feedback for multiple marketplace registrations

- Fix silent exception swallowing in registry.py:
  - _resolve_from_all() now accumulates fetch errors and includes them in
    PluginNotFoundError when all marketplaces fail
  - list_plugins() now raises PluginResolutionError with details when all fail
  - Improves debugging: users see actual errors instead of 'plugin not found'

- Simplify agent_context.py deprecation logic:
  - Remove unnecessary effective_marketplace_path variable
  - Remove misleading comment about backward compatibility

- Improve path validation security in types.py:
  - Add _validate_repo_path() helper using os.path.normpath()
  - Catches path traversal tricks like 'safe/../../../etc'
  - Both MarketplaceRegistration and PluginSource use shared validation

- Fix state mutation atomicity in local_conversation.py:
  - Update both self.agent and self._state.agent within same lock context

- Add comprehensive tests:
  - TestErrorAccumulation: 3 tests for error accumulation behavior
  - TestPathValidation: 5 tests for path security validation

Author: openhands-agent

openhands-sdk/openhands/sdk/context/agent_context.py

@@ -135,26 +135,21 @@ def _load_auto_skills(self):
         if not self.load_user_skills and not self.load_public_skills:
             return self
 
-        # Handle backward compatibility: if marketplace_path is set but
-        # registered_marketplaces is empty, emit deprecation warning and
-        # convert to a default marketplace registration
-        effective_marketplace_path = self.marketplace_path
+        # Emit deprecation warning if using old marketplace_path field
         if self.marketplace_path is not None and not self.registered_marketplaces:
             warnings.warn(
                 "AgentContext.marketplace_path is deprecated. "
                 "Use registered_marketplaces instead.",
                 DeprecationWarning,
                 stacklevel=2,
             )
-            # For backward compatibility, we still use marketplace_path
-            # when registered_marketplaces is empty
 
         auto_skills = load_available_skills(
             work_dir=None,
             include_user=self.load_user_skills,
             include_project=False,
             include_public=self.load_public_skills,
-            marketplace_path=effective_marketplace_path,
+            marketplace_path=self.marketplace_path,
         )
 
         existing_names = {skill.name for skill in self.skills}

openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py

@@ -1188,17 +1188,17 @@ def load_plugin(self, plugin_ref: str) -> None:
             dict(self.agent.mcp_config) if self.agent.mcp_config else {}
         )
 
-        # Update agent with merged content
-        self.agent = self.agent.model_copy(
+        # Update agent and state atomically
+        # Create new agent first, then update both references together
+        new_agent = self.agent.model_copy(
             update={
                 "agent_context": merged_context,
                 "mcp_config": merged_mcp,
             }
         )
-
-        # Update agent in state for API observability
         with self._state:
-            self._state.agent = self.agent
+            self.agent = new_agent
+            self._state.agent = new_agent
 
         # Track resolved plugin
         resolved = ResolvedPluginSource.from_plugin_source(

openhands-sdk/openhands/sdk/plugin/registry.py

@@ -38,13 +38,29 @@ def __init__(self, plugin_name: str, matching_marketplaces: list[str]):
 class PluginNotFoundError(PluginResolutionError):
     """Raised when a plugin cannot be found in any registered marketplace."""
 
-    def __init__(self, plugin_name: str, marketplace_name: str | None = None):
+    def __init__(
+        self,
+        plugin_name: str,
+        marketplace_name: str | None = None,
+        fetch_errors: dict[str, Exception] | None = None,
+    ):
         self.plugin_name = plugin_name
         self.marketplace_name = marketplace_name
+        self.fetch_errors = fetch_errors or {}
+
         if marketplace_name:
             msg = (
                 f"Plugin '{plugin_name}' not found in marketplace '{marketplace_name}'"
             )
+        elif fetch_errors:
+            # All marketplaces failed to fetch - show the actual errors
+            error_details = "; ".join(
+                f"'{name}': {err}" for name, err in fetch_errors.items()
+            )
+            msg = (
+                f"Plugin '{plugin_name}' not found. "
+                f"All {len(fetch_errors)} marketplace(s) failed to fetch: {error_details}"
+            )
         else:
             msg = f"Plugin '{plugin_name}' not found in any registered marketplace"
         super().__init__(msg)
@@ -226,10 +242,13 @@ def _resolve_from_marketplace(
     def _resolve_from_all(self, plugin_name: str) -> PluginSource:
         """Resolve a plugin by searching all registered marketplaces."""
         matches: list[tuple[str, PluginSource]] = []
+        fetch_errors: dict[str, Exception] = {}
+        searched_count = 0
 
         for name, reg in self._registrations.items():
             try:
                 marketplace, repo_path = self._fetch_marketplace(reg)
+                searched_count += 1
                 plugin_entry = marketplace.get_plugin(plugin_name)
 
                 if plugin_entry is not None:
@@ -244,12 +263,16 @@ def _resolve_from_all(self, plugin_name: str) -> PluginSource:
                     matches.append((name, plugin_source))
 
             except Exception as e:
+                fetch_errors[name] = e
                 logger.warning(
                     f"Error searching marketplace '{name}' "
                     f"for plugin '{plugin_name}': {e}"
                 )
 
         if not matches:
+            # If all marketplaces failed to fetch, include errors in exception
+            if fetch_errors and searched_count == 0:
+                raise PluginNotFoundError(plugin_name, fetch_errors=fetch_errors)
             raise PluginNotFoundError(plugin_name)
 
         if len(matches) > 1:
@@ -269,18 +292,33 @@ def list_plugins(self, marketplace_name: str | None = None) -> list[str]:
 
         Returns:
             List of plugin names (may include duplicates if searching all).
+
+        Raises:
+            PluginResolutionError: If all marketplaces fail to fetch when listing all.
         """
         plugin_names: list[str] = []
 
         if marketplace_name:
             marketplace, _ = self.get_marketplace(marketplace_name)
             plugin_names.extend(p.name for p in marketplace.plugins)
         else:
+            fetch_errors: dict[str, Exception] = {}
             for name, reg in self._registrations.items():
                 try:
                     marketplace, _ = self._fetch_marketplace(reg)
                     plugin_names.extend(p.name for p in marketplace.plugins)
                 except Exception as e:
+                    fetch_errors[name] = e
                     logger.warning(f"Error listing plugins from '{name}': {e}")
 
+            # If all marketplaces failed, raise with details
+            if fetch_errors and not plugin_names and self._registrations:
+                error_details = "; ".join(
+                    f"'{name}': {err}" for name, err in fetch_errors.items()
+                )
+                raise PluginResolutionError(
+                    f"Failed to list plugins. "
+                    f"All {len(fetch_errors)} marketplace(s) failed: {error_details}"
+                )
+
         return plugin_names

openhands-sdk/openhands/sdk/plugin/types.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import json
+import os
 from pathlib import Path
 from typing import TYPE_CHECKING, Any, Literal
 
@@ -74,18 +75,41 @@ class MarketplaceRegistration(BaseModel):
     @classmethod
     def validate_repo_path(cls, v: str | None) -> str | None:
         """Validate repo_path is a safe relative path within the repository."""
-        if v is None:
-            return v
-        # Must be relative (no absolute paths)
-        if v.startswith("/"):
-            raise ValueError("repo_path must be relative, not absolute")
-        # No parent directory traversal
-        if ".." in Path(v).parts:
-            raise ValueError(
-                "repo_path cannot contain '..' (parent directory traversal)"
-            )
+        return _validate_repo_path(v)
+
+
+def _validate_repo_path(v: str | None) -> str | None:
+    """Validate that a repo_path is a safe relative path.
+
+    Ensures the path:
+    - Is relative (not absolute)
+    - Does not escape the repository root via '..' traversal
+    - Normalizes to a path within the repo even after resolution
+    """
+    if v is None:
         return v
 
+    # Must be relative (no absolute paths)
+    if v.startswith("/"):
+        raise ValueError("repo_path must be relative, not absolute")
+
+    # Normalize the path to catch tricks like 'safe/../../../etc'
+    # Use a dummy root to resolve against, then check if result stays inside
+    dummy_root = Path("/repo")
+    normalized = os.path.normpath(os.path.join(str(dummy_root), v))
+    normalized_path = Path(normalized)
+
+    # Check if the normalized path is still under dummy_root
+    # This catches paths like 'a/../../etc' which normalize to '/etc'
+    try:
+        normalized_path.relative_to(dummy_root)
+    except ValueError:
+        raise ValueError(
+            f"repo_path '{v}' escapes repository root after normalization"
+        )
+
+    return v
+
 
 class PluginSource(BaseModel):
     """Specification for a plugin to load.
@@ -127,17 +151,7 @@ class PluginSource(BaseModel):
     @classmethod
     def validate_repo_path(cls, v: str | None) -> str | None:
         """Validate repo_path is a safe relative path within the repository."""
-        if v is None:
-            return v
-        # Must be relative (no absolute paths)
-        if v.startswith("/"):
-            raise ValueError("repo_path must be relative, not absolute")
-        # No parent directory traversal
-        if ".." in Path(v).parts:
-            raise ValueError(
-                "repo_path cannot contain '..' (parent directory traversal)"
-            )
-        return v
+        return _validate_repo_path(v)
 
 
 class ResolvedPluginSource(BaseModel):

tests/sdk/plugin/test_marketplace_registry.py

@@ -12,6 +12,7 @@
     MarketplaceRegistration,
     MarketplaceRegistry,
     PluginNotFoundError,
+    PluginResolutionError,
 )
 
 
@@ -68,7 +69,7 @@ def test_repo_path_validation_rejects_absolute(self):
 
     def test_repo_path_validation_rejects_traversal(self):
         """Test that parent directory traversal is rejected."""
-        with pytest.raises(ValueError, match="cannot contain '..'"):
+        with pytest.raises(ValueError, match="escapes repository root"):
             MarketplaceRegistration(
                 name="test",
                 source="github:owner/repo",
@@ -319,3 +320,134 @@ def test_list_plugins_from_all(self, mock_marketplace_dir):
             plugins = registry.list_plugins()
 
             assert set(plugins) == {"plugin-a", "plugin-b"}
+
+
+class TestErrorAccumulation:
+    """Tests for error accumulation when marketplaces fail."""
+
+    def test_resolve_plugin_all_marketplaces_fail_shows_errors(self):
+        """Test that when all marketplaces fail, errors are included in exception."""
+        regs = [
+            MarketplaceRegistration(name="mp1", source="github:owner/repo1"),
+            MarketplaceRegistration(name="mp2", source="github:owner/repo2"),
+        ]
+        registry = MarketplaceRegistry(regs)
+
+        # Mock _fetch_marketplace to always fail
+        error1 = ConnectionError("Network unreachable for mp1")
+        error2 = TimeoutError("Timeout connecting to mp2")
+
+        def mock_fetch(reg):
+            if reg.name == "mp1":
+                raise error1
+            raise error2
+
+        with patch.object(registry, "_fetch_marketplace", side_effect=mock_fetch):
+            with pytest.raises(PluginNotFoundError) as exc_info:
+                registry.resolve_plugin("some-plugin")
+
+            # Error should mention all marketplace failures
+            assert exc_info.value.fetch_errors is not None
+            assert len(exc_info.value.fetch_errors) == 2
+            assert "mp1" in exc_info.value.fetch_errors
+            assert "mp2" in exc_info.value.fetch_errors
+            # Exception message should contain details
+            assert "All 2 marketplace(s) failed" in str(exc_info.value)
+            assert "Network unreachable" in str(exc_info.value)
+            assert "Timeout" in str(exc_info.value)
+
+    def test_resolve_plugin_partial_failures_dont_show_errors(
+        self, mock_marketplace_dir
+    ):
+        """Test that partial failures (some succeed) don't include fetch_errors."""
+        regs = [
+            MarketplaceRegistration(name="failing", source="github:owner/bad"),
+            MarketplaceRegistration(name="working", source=str(mock_marketplace_dir)),
+        ]
+        registry = MarketplaceRegistry(regs)
+
+        marketplace = Marketplace.load(mock_marketplace_dir)
+
+        def mock_fetch(reg):
+            if reg.name == "failing":
+                raise ConnectionError("Network error")
+            return (marketplace, mock_marketplace_dir)
+
+        with patch.object(registry, "_fetch_marketplace", side_effect=mock_fetch):
+            # Plugin not in the marketplace that succeeded
+            with pytest.raises(PluginNotFoundError) as exc_info:
+                registry.resolve_plugin("nonexistent-plugin")
+
+            # Since one marketplace was searched successfully, we get normal error
+            assert "not found in any registered marketplace" in str(exc_info.value)
+            # fetch_errors should be empty (not all failed)
+            assert not exc_info.value.fetch_errors
+
+    def test_list_plugins_all_marketplaces_fail_raises_error(self):
+        """Test that list_plugins raises error with details when all fail."""
+        regs = [
+            MarketplaceRegistration(name="mp1", source="github:owner/repo1"),
+            MarketplaceRegistration(name="mp2", source="github:owner/repo2"),
+        ]
+        registry = MarketplaceRegistry(regs)
+
+        def mock_fetch(reg):
+            raise ConnectionError(f"Failed to fetch {reg.name}")
+
+        with patch.object(registry, "_fetch_marketplace", side_effect=mock_fetch):
+            with pytest.raises(PluginResolutionError) as exc_info:
+                registry.list_plugins()
+
+            # Error message should show all failures
+            assert "All 2 marketplace(s) failed" in str(exc_info.value)
+            assert "mp1" in str(exc_info.value)
+            assert "mp2" in str(exc_info.value)
+
+
+class TestPathValidation:
+    """Tests for repo_path validation security."""
+
+    def test_repo_path_rejects_traversal_via_normalization(self):
+        """Test that paths like 'safe/../../../etc' are rejected."""
+        with pytest.raises(ValueError, match="escapes repository root"):
+            MarketplaceRegistration(
+                name="test",
+                source="github:owner/repo",
+                repo_path="safe/../../../etc/passwd",
+            )
+
+    def test_repo_path_allows_valid_nested_path(self):
+        """Test that valid nested paths are allowed."""
+        reg = MarketplaceRegistration(
+            name="test",
+            source="github:owner/repo",
+            repo_path="marketplaces/internal/plugins",
+        )
+        assert reg.repo_path == "marketplaces/internal/plugins"
+
+    def test_repo_path_allows_simple_path(self):
+        """Test that simple paths without any tricks work."""
+        reg = MarketplaceRegistration(
+            name="test",
+            source="github:owner/repo",
+            repo_path="plugins",
+        )
+        assert reg.repo_path == "plugins"
+
+    def test_repo_path_rejects_absolute_path(self):
+        """Test that absolute paths are rejected."""
+        with pytest.raises(ValueError, match="must be relative"):
+            MarketplaceRegistration(
+                name="test",
+                source="github:owner/repo",
+                repo_path="/etc/passwd",
+            )
+
+    def test_repo_path_rejects_simple_parent_traversal(self):
+        """Test that simple '..' traversal is rejected."""
+        with pytest.raises(ValueError, match="escapes repository root"):
+            MarketplaceRegistration(
+                name="test",
+                source="github:owner/repo",
+                repo_path="../outside",
+            )