From 2bfeff6de03b23aa9d0a0bb517264205c3ea31a5 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:52:11 -0400 Subject: [PATCH 01/12] feat(agent-profiles): tool catalog and a truthful materialize preview A client building an Agent Profile editor could not ask which tools a user may pick, or what a profile will actually launch with, so it had to hardcode both. - ToolDefinition declares `user_selectable`; the registry serves it via `GET /api/tools/catalog`. The catalog is sealed once the server has loaded its tools, so tools a single conversation registers (client tools, `tool_module_qualnames` imports) are never offered. - `resolve_tool_specs` is the one place a `tools` setting becomes specs, used by `create_agent` and by the profile resolver. Browser availability is an explicit resolver input rather than a post-launch injection, so `resolved_settings.tools` reports what the launch really builds. - Materialize accepts a draft profile body, so an editor can preview before the first save. - `server_info` advertises `tool_catalog_v1` and `agent_profile_draft_materialize_v1`. Closes #4958 Co-Authored-By: Claude Opus 5 (1M context) --- .../agent_server/agent_profiles_router.py | 91 +++++++++-------- .../openhands/agent_server/api.py | 6 ++ .../agent_server/conversation_service.py | 40 ++------ .../openhands/agent_server/profile_launch.py | 44 +++++++++ .../agent_server/server_details_router.py | 2 + .../openhands/agent_server/tool_router.py | 21 +++- .../openhands/sdk/profiles/agent_profile.py | 8 +- .../openhands/sdk/profiles/resolver.py | 28 +++++- openhands-sdk/openhands/sdk/settings/model.py | 18 ++-- openhands-sdk/openhands/sdk/tool/__init__.py | 8 ++ .../openhands/sdk/tool/builtins/finish.py | 4 +- .../sdk/tool/builtins/invoke_skill.py | 4 +- .../openhands/sdk/tool/builtins/switch_llm.py | 4 +- .../openhands/sdk/tool/builtins/think.py | 4 +- .../sdk/tool/builtins/vision_inspect.py | 2 + .../openhands/sdk/tool/client_tool.py | 4 +- openhands-sdk/openhands/sdk/tool/defaults.py | 43 +++++--- openhands-sdk/openhands/sdk/tool/registry.py | 54 +++++++++-- openhands-sdk/openhands/sdk/tool/tool.py | 3 + .../openhands/tools/gemini/edit/definition.py | 4 +- .../tools/gemini/list_directory/definition.py | 4 +- .../tools/gemini/read_file/definition.py | 4 +- .../tools/gemini/write_file/definition.py | 4 +- .../tools/planning_file_editor/definition.py | 4 +- .../openhands/tools/task/definition.py | 6 +- .../openhands/tools/workflow/definition.py | 4 +- .../docker_runtime/test_mediation.py | 2 +- .../test_agent_profile_conv_start.py | 97 +++++++------------ .../test_agent_profiles_router.py | 94 +++++++++++++++++- .../test_server_details_router.py | 6 ++ tests/agent_server/test_tool_router.py | 38 ++++++++ tests/sdk/profiles/test_resolver.py | 89 +++++++++++------ tests/sdk/tool/test_defaults.py | 16 +++ tests/sdk/tool/test_registry.py | 53 +++++++++- 34 files changed, 598 insertions(+), 215 deletions(-) create mode 100644 openhands-agent-server/openhands/agent_server/profile_launch.py diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index e83eac45d7..aba048e95b 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -6,9 +6,10 @@ pointer-only — unlike the LLM ``/activate`` it must **not** write ``agent_settings`` (the creation-time-only contract). -``POST /{name}/materialize`` performs a dry-run resolve of a profile's LLM and -MCP references and returns :class:`~openhands.sdk.profiles.AgentProfileDiagnostics` -(never raises on dangling refs — those appear in the body). +``POST /{name}/materialize`` performs a dry-run resolve of a stored profile, or +of a draft body, and returns +:class:`~openhands.sdk.profiles.AgentProfileDiagnostics` (never raises on +dangling refs — those appear in the body). """ import asyncio @@ -28,8 +29,8 @@ get_llm_profile_store, get_settings_store, ) +from openhands.agent_server.profile_launch import gather_profile_launch_inputs from openhands.agent_server.profiles_router import MAX_PROFILES, _has_api_key -from openhands.agent_server.skills_service import discover_profile_skills from openhands.sdk.llm import LLM from openhands.sdk.llm.llm_profile_store import ( ProfileLimitExceeded as LLMProfileLimitExceeded, @@ -37,6 +38,7 @@ from openhands.sdk.logger import get_logger from openhands.sdk.profiles import ( SEED_PROFILE_NAME, + AgentProfile, AgentProfileDiagnostics, AgentProfileStore, ProfileLimitExceeded, @@ -98,6 +100,16 @@ class ActivateAgentProfileResponse(BaseModel): agent_settings_applied: bool = False +class MaterializeAgentProfileRequest(BaseModel): + profile: AgentProfile | None = Field( + default=None, + description=( + "Draft profile to evaluate instead of the stored one. The path name " + "overrides the draft's name." + ), + ) + + class RenameAgentProfileRequest(BaseModel): new_name: str = Field( ..., @@ -488,23 +500,30 @@ def set_pointer(settings: PersistedSettings) -> PersistedSettings: response_model=AgentProfileDiagnostics, ) async def materialize_agent_profile( - request: Request, name: ProfileName + request: Request, + name: ProfileName, + body: MaterializeAgentProfileRequest | None = None, ) -> AgentProfileDiagnostics: - """Dry-run resolve a profile's LLM/MCP references; return a diagnostics report. + """Dry-run resolve a profile the way a launch would; return a diagnostics report. - Dangling LLM/MCP references are reported in the body (valid=False) rather - than raising — the only error status is 404 (unknown profile name). - resolved_settings is redacted (api_key_set booleans; no raw secrets). + Resolves the stored profile ``name``, or ``body.profile`` when given (so an + editor can preview before saving). Dangling LLM/MCP references are reported + in the body (valid=False) rather than raising — the only error statuses are + 404 (unknown stored profile) and 422 (invalid draft). resolved_settings is + redacted (api_key_set booleans; no raw secrets). """ - store = get_agent_profile_store() - try: - with store_errors(): - profile = store.load(name) - except FileNotFoundError: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Agent profile '{name}' not found", - ) + if body is not None and body.profile is not None: + profile = body.profile.model_copy(update={"name": name}) + else: + store = get_agent_profile_store() + try: + with store_errors(): + profile = store.load(name) + except FileNotFoundError: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Agent profile '{name}' not found", + ) # Still needed here (unlike the profile load above): resolve_agent_profile_ # dry_run uses it to decrypt the *referenced LLM profile's* own secret. @@ -513,35 +532,29 @@ async def materialize_agent_profile( settings = get_settings_store(config).load() or PersistedSettings() mcp_config = settings.agent_settings.mcp_config - # Discover skills off the event loop so the dry-run can report which skills - # (catalog minus ``disabled_skills``) resolve. Mirrors the launch rule in - # ``conversation_service._resolve_agent_from_profile`` so the preview matches - # a real launch: an ACP profile is only given a catalog where the CLI cannot - # read the user's own configuration (#4019). A discovery failure must not 500 - # the preview: pass ``available_skills=None`` and surface the failure as its - # own diagnostic below. - discovery_error: str | None = None - available_skills = None - if profile.agent_kind == "openhands" or ( - config.acp_skill_sourcing == "openhands_managed" - ): - try: - available_skills = await asyncio.to_thread(discover_profile_skills) - except Exception as exc: - available_skills = None - discovery_error = str(exc) - logger.warning("Skill discovery failed during materialize: %s", exc) + inputs = await asyncio.to_thread( + gather_profile_launch_inputs, profile, config.acp_skill_sourcing + ) + if inputs.skill_discovery_error is not None: + logger.warning( + "Skill discovery failed during materialize: %s", + inputs.skill_discovery_error, + ) llm_store = get_llm_profile_store() diagnostics = resolve_agent_profile_dry_run( profile, llm_store=llm_store, mcp_config=mcp_config, - available_skills=available_skills, + available_skills=inputs.available_skills, cipher=cipher, + browser_available=inputs.browser_available, ) - if discovery_error is not None: - diagnostics.errors.append(f"Skill discovery failed: {discovery_error}") + # Reported rather than raised: a launch would fail on it, the preview must not. + if inputs.skill_discovery_error is not None: + diagnostics.errors.append( + f"Skill discovery failed: {inputs.skill_discovery_error}" + ) diagnostics.valid = False diagnostics.resolved_settings = None return diagnostics diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index 9c54fcd5b3..3b44ae3363 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -95,6 +95,7 @@ from openhands.agent_server.vscode_service import get_vscode_service from openhands.agent_server.workspaces_router import workspaces_router from openhands.sdk.logger import DEBUG, get_logger +from openhands.sdk.tool import seal_tool_catalog from openhands.sdk.utils.redact import sanitize_dict from openhands.tools.terminal.constants import TMUX_SOCKET_NAME @@ -171,6 +172,11 @@ async def api_lifespan(api: FastAPI) -> AsyncIterator[None]: if not deferred: emit_server_started() + # Every tool this deployment offers is registered by now (presets at + # import, plus any ``--import-modules``). Later registrations belong to + # one conversation and vanish on restart, so they stay out of the catalog. + seal_tool_catalog() + vscode_service = get_vscode_service() tool_preload_service = get_tool_preload_service() diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index c510ce6b51..ffc8ec66ec 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -35,9 +35,9 @@ UpdateConversationRequest, ) from openhands.agent_server.persistence import FileSecretsStore +from openhands.agent_server.profile_launch import gather_profile_launch_inputs from openhands.agent_server.pub_sub import Subscriber from openhands.agent_server.server_details_router import update_last_execution_time -from openhands.agent_server.skills_service import discover_profile_skills from openhands.agent_server.telemetry import ( ConversationTelemetryContext, DiagnosticEventFactory, @@ -72,7 +72,6 @@ from openhands.sdk.git.utils import run_git_command, validate_git_repository from openhands.sdk.mcp.utils import MCPToolProvider from openhands.sdk.observability import OPERATION_METADATA_KEY, observe -from openhands.sdk.tool import BROWSER_TOOL_NAME, Tool, is_tool_usable from openhands.sdk.tool.client_tool import register_client_tools from openhands.sdk.utils.cipher import Cipher from openhands.sdk.workspace import LocalWorkspace @@ -402,22 +401,13 @@ def _resolve_agent_from_profile( f"Failed to load agent profile '{profile_name}': {exc}" ) from exc - # OpenHands profiles get the discovered catalog minus their ``disabled_skills`` - # deny-list. An ACP profile gets it only where the CLI cannot reach the user's - # own configuration (``openhands_managed``); under ``native`` it sources its - # own skills and OpenHands injects none (#4019). A genuine discovery failure - # fails the launch loudly rather than silently producing a zero-skill agent. - available_skills = None - wants_skills = profile.agent_kind == "openhands" or ( - acp_skill_sourcing == "openhands_managed" - ) - if wants_skills: - try: - available_skills = discover_profile_skills() - except Exception as exc: - raise ValueError( - f"Skill discovery failed for profile '{profile_name}': {exc}" - ) from exc + inputs = gather_profile_launch_inputs(profile, acp_skill_sourcing) + # Fail loudly rather than silently launching a zero-skill agent. + if inputs.skill_discovery_error is not None: + raise ValueError( + f"Skill discovery failed for profile '{profile_name}': " + f"{inputs.skill_discovery_error}" + ) from inputs.skill_discovery_error llm_store = get_llm_profile_store() try: @@ -425,8 +415,9 @@ def _resolve_agent_from_profile( profile, llm_store=llm_store, mcp_config=mcp_config, - available_skills=available_skills, + available_skills=inputs.available_skills, cipher=cipher, + browser_available=inputs.browser_available, ) except (TypeError, ValueError) as exc: raise ValueError(f"Profile '{profile_name}' failed to resolve: {exc}") from exc @@ -442,17 +433,6 @@ def _resolve_agent_from_profile( ) agent = settings_config.create_agent() - # Browser is deliberately absent from the deterministic SDK default - # (environment-dependent); this server knows its runtime, so it injects - # browser when usable. An explicit profile.tools list is authoritative. - if ( - profile.agent_kind == "openhands" - and profile.tools is None - and is_tool_usable(BROWSER_TOOL_NAME) - ): - agent = agent.model_copy( - update={"tools": [*agent.tools, Tool(name=BROWSER_TOOL_NAME)]} - ) launched = LaunchedAgentProfile( agent_profile_id=profile.id, diff --git a/openhands-agent-server/openhands/agent_server/profile_launch.py b/openhands-agent-server/openhands/agent_server/profile_launch.py new file mode 100644 index 0000000000..1fecf5c738 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/profile_launch.py @@ -0,0 +1,44 @@ +"""Deployment inputs an agent profile is resolved against. + +Shared by conversation launch and the materialize preview so both resolve a +profile against the same skill catalog and runtime capabilities. +""" + +from typing import NamedTuple + +from openhands.agent_server.config import ACPSkillSourcing +from openhands.agent_server.skills_service import discover_profile_skills +from openhands.sdk.profiles import ACPAgentProfile, OpenHandsAgentProfile +from openhands.sdk.skills import Skill +from openhands.sdk.tool import BROWSER_TOOL_NAME, is_tool_usable + + +class ProfileLaunchInputs(NamedTuple): + available_skills: list[Skill] | None + skill_discovery_error: Exception | None + browser_available: bool + + +def gather_profile_launch_inputs( + profile: OpenHandsAgentProfile | ACPAgentProfile, + acp_skill_sourcing: ACPSkillSourcing, +) -> ProfileLaunchInputs: + """Discover the skill catalog and probe this runtime for ``profile``. + + An ACP profile only gets the managed skill catalog where its CLI cannot read + the user's own configuration (#4019). + """ + available_skills = None + discovery_error = None + if profile.agent_kind == "openhands" or acp_skill_sourcing == "openhands_managed": + try: + available_skills = discover_profile_skills() + except Exception as exc: + discovery_error = exc + return ProfileLaunchInputs( + available_skills=available_skills, + skill_discovery_error=discovery_error, + browser_available=( + profile.agent_kind == "openhands" and is_tool_usable(BROWSER_TOOL_NAME) + ), + ) diff --git a/openhands-agent-server/openhands/agent_server/server_details_router.py b/openhands-agent-server/openhands/agent_server/server_details_router.py index 011b99f88d..70330ea387 100644 --- a/openhands-agent-server/openhands/agent_server/server_details_router.py +++ b/openhands-agent-server/openhands/agent_server/server_details_router.py @@ -67,6 +67,8 @@ class ServerInfo(BaseModel): "credential_binding_v1", "credential_binding_readiness_probe_v1", "credential_binding_activation_guard_v1", + "tool_catalog_v1", + "agent_profile_draft_materialize_v1", ] ) max_foreground_terminal_timeout_seconds: float | None = Field( diff --git a/openhands-agent-server/openhands/agent_server/tool_router.py b/openhands-agent-server/openhands/agent_server/tool_router.py index 0ab2b4a27f..86764ebce6 100644 --- a/openhands-agent-server/openhands/agent_server/tool_router.py +++ b/openhands-agent-server/openhands/agent_server/tool_router.py @@ -1,8 +1,13 @@ """Tool router for OpenHands SDK.""" from fastapi import APIRouter +from pydantic import BaseModel -from openhands.sdk.tool.registry import list_registered_tools +from openhands.sdk.tool.registry import ( + ToolCatalogEntry, + list_registered_tools, + list_tool_catalog, +) from openhands.tools.preset.default import ( register_builtins_agents, register_default_tools, @@ -24,3 +29,17 @@ async def list_available_tools() -> list[str]: """List all available tools.""" tools = list_registered_tools() return tools + + +class ToolCatalogResponse(BaseModel): + tools: list[ToolCatalogEntry] + + +@tool_router.get("/catalog") +async def get_tool_catalog() -> ToolCatalogResponse: + """List the tools this server offers for configuring an agent. + + Clients offer the ``user_selectable`` entries; ``usable`` says whether this + server's runtime can run the tool. + """ + return ToolCatalogResponse(tools=list_tool_catalog()) diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index 78f7e5dbaf..01de246a2b 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -162,15 +162,15 @@ class OpenHandsAgentProfile(AgentProfileBase): default="CodeActAgent", description="Agent class to build.", ) - # Same tri-state as the resolved settings' ``tools``: passed through - # verbatim by the resolver, so ``create_agent`` is the single defaulting - # point (#3978). Secret-free by construction (``Tool`` is name + params). + # Same tri-state as the settings' ``tools``, resolved by + # ``resolve_tool_specs``. Secret-free by construction (name + params). tools: list[Tool] | None = Field( default=None, description=( "Tool selection for the resolved agent. None (the default) = the " "server's standard tool set; [] = an explicitly bare agent; a " - "non-empty list is used exactly as given." + "non-empty list is used as given. enable_sub_agents adds the " + "sub-agent tool set in every case." ), ) diff --git a/openhands-sdk/openhands/sdk/profiles/resolver.py b/openhands-sdk/openhands/sdk/profiles/resolver.py index 4fb26297e2..f8e7b875b1 100644 --- a/openhands-sdk/openhands/sdk/profiles/resolver.py +++ b/openhands-sdk/openhands/sdk/profiles/resolver.py @@ -47,6 +47,7 @@ validate_agent_settings, ) from openhands.sdk.skills import Skill +from openhands.sdk.tool.defaults import resolve_tool_specs from openhands.sdk.utils.pydantic_secrets import REDACTED_SECRET_VALUE @@ -229,6 +230,8 @@ def _build_openhands_settings( llm: LLM, mcp_config: dict[str, MCPServer], filtered_skills: list[Skill], + *, + browser_available: bool, ) -> AgentSettingsConfig: """Compose the resolved ``OpenHandsAgentSettings`` from a profile + LLM. @@ -247,8 +250,11 @@ def _build_openhands_settings( "agent": profile.agent, "llm": llm, "mcp_config": mcp_config, - # Tri-state passthrough; create_agent materializes None. - "tools": profile.tools, + "tools": resolve_tool_specs( + profile.tools, + enable_sub_agents=profile.enable_sub_agents, + enable_browser=browser_available, + ), "agent_context": AgentContext( skills=filtered_skills, system_message_suffix=profile.system_message_suffix, @@ -315,6 +321,7 @@ def resolve_agent_profile( mcp_config: dict[str, MCPServer], available_skills: list[Skill] | None, cipher: Cipher | None = None, + browser_available: bool = False, ) -> AgentSettingsConfig: """Resolve a profile's references into a validated ``AgentSettingsConfig``. @@ -328,6 +335,8 @@ def resolve_agent_profile( deployment leaves skill sourcing to the CLI. Unlike the ``mcp_server_refs`` allow-list, the ``disabled_skills`` deny-list can never dangle, so this never raises for skills. ``cipher`` decrypts the referenced LLM profile. + ``browser_available`` adds the browser tool set to a default toolset; the + caller probes the runtime the agent will run on. Raises: ProfileNotFound: ``llm_profile_ref`` does not exist (OpenHands path). @@ -347,7 +356,13 @@ def resolve_agent_profile( raise ProfileNotFound( f"LLM profile {profile.llm_profile_ref!r} not found" ) from e - return _build_openhands_settings(profile, llm, filtered_mcp, filtered_skills) + return _build_openhands_settings( + profile, + llm, + filtered_mcp, + filtered_skills, + browser_available=browser_available, + ) return _build_acp_settings( profile, filtered_mcp, _apply_disabled_skills(available_skills, []) @@ -361,6 +376,7 @@ def resolve_agent_profile_dry_run( mcp_config: dict[str, MCPServer], available_skills: list[Skill] | None, cipher: Cipher | None = None, + browser_available: bool = False, ) -> AgentProfileDiagnostics: """Compute :class:`AgentProfileDiagnostics` without raising or side effects. @@ -442,7 +458,11 @@ def resolve_agent_profile_dry_run( "OpenHands profile marked valid without a resolved LLM" ) settings = _build_openhands_settings( - profile, llm, filtered_mcp, filtered_skills + profile, + llm, + filtered_mcp, + filtered_skills, + browser_available=browser_available, ) else: settings = _build_acp_settings(profile, filtered_mcp, filtered_skills) diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 5bac9b061e..4e19ddcd97 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1275,9 +1275,9 @@ class OpenHandsAgentSettings(AgentSettingsBase): default=None, description=( "Tools available to the agent. None (the default) resolves to the " - "standard exec set (see openhands.sdk.tool.defaults), plus the " - "sub-agent tool set when enable_sub_agents is set; [] is an " - "explicitly bare agent; a non-empty list is used exactly as given. " + "standard exec set (see openhands.sdk.tool.defaults); [] is an " + "explicitly bare agent; a non-empty list is used as given. " + "enable_sub_agents adds the sub-agent tool set in every case. " "Environment-dependent tools (browser) are injected by the serving " "layer, not the default." ), @@ -1398,15 +1398,9 @@ def create_agent(self) -> Agent: from openhands.sdk.agent import Agent from openhands.sdk.llm.auth.openai import create_subscription_llm_from_config from openhands.sdk.tool.builtins import BUILT_IN_TOOLS, SwitchLLMTool - from openhands.sdk.tool.defaults import default_tool_specs - - # Single defaulting point: None = the canonical default set (honoring - # enable_sub_agents); [] stays an explicitly bare agent. - tools = ( - self.tools - if self.tools is not None - else default_tool_specs(enable_sub_agents=self.enable_sub_agents) - ) + from openhands.sdk.tool.defaults import resolve_tool_specs + + tools = resolve_tool_specs(self.tools, enable_sub_agents=self.enable_sub_agents) include_default_tools = [tool.__name__ for tool in BUILT_IN_TOOLS] if self.enable_switch_llm_tool: diff --git a/openhands-sdk/openhands/sdk/tool/__init__.py b/openhands-sdk/openhands/sdk/tool/__init__.py index 6884a09c57..16ae5bbf27 100644 --- a/openhands-sdk/openhands/sdk/tool/__init__.py +++ b/openhands-sdk/openhands/sdk/tool/__init__.py @@ -16,12 +16,16 @@ DEFAULT_EXEC_TOOL_NAMES, SUB_AGENT_TOOL_NAME, default_tool_specs, + resolve_tool_specs, ) from openhands.sdk.tool.registry import ( + ToolCatalogEntry, is_tool_usable, list_registered_tools, + list_tool_catalog, register_tool, resolve_tool, + seal_tool_catalog, ) from openhands.sdk.tool.schema import ( Action, @@ -49,6 +53,7 @@ "DEFAULT_EXEC_TOOL_NAMES", "SUB_AGENT_TOOL_NAME", "default_tool_specs", + "resolve_tool_specs", "is_tool_usable", "ToolDefinition", "ToolAnnotations", @@ -63,4 +68,7 @@ "register_tool", "resolve_tool", "list_registered_tools", + "list_tool_catalog", + "seal_tool_catalog", + "ToolCatalogEntry", ] diff --git a/openhands-sdk/openhands/sdk/tool/builtins/finish.py b/openhands-sdk/openhands/sdk/tool/builtins/finish.py index 0bb12f849b..3e622d7504 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/finish.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/finish.py @@ -1,5 +1,5 @@ from collections.abc import Sequence -from typing import TYPE_CHECKING, Self +from typing import TYPE_CHECKING, ClassVar, Self from pydantic import Field from rich.text import Text @@ -69,6 +69,8 @@ def __call__( class FinishTool(ToolDefinition[FinishAction, FinishObservation]): """Tool for signaling the completion of a task or conversation.""" + user_selectable: ClassVar[bool] = False + @classmethod def create( cls, diff --git a/openhands-sdk/openhands/sdk/tool/builtins/invoke_skill.py b/openhands-sdk/openhands/sdk/tool/builtins/invoke_skill.py index fc39c735ee..9eabe38801 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/invoke_skill.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/invoke_skill.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING, Self +from typing import TYPE_CHECKING, ClassVar, Self from pydantic import Field from rich.text import Text @@ -163,6 +163,8 @@ def _append_skill_location_footer( class InvokeSkillTool(ToolDefinition[InvokeSkillAction, InvokeSkillObservation]): """Built-in tool for explicit invocation of progressive-disclosure skills.""" + user_selectable: ClassVar[bool] = False + def declared_resources(self, action: Action) -> DeclaredResources: # Rendering a skill may execute inline `!`cmd`` tokens, which can # touch arbitrary on-disk state. Keying on the skill name serializes diff --git a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py index 99e016f5f0..a00a55dde8 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py @@ -1,5 +1,5 @@ from collections.abc import Sequence -from typing import TYPE_CHECKING, Self +from typing import TYPE_CHECKING, ClassVar, Self from pydantic import Field from rich.text import Text @@ -151,6 +151,8 @@ def __call__( class SwitchLLMTool(ToolDefinition[SwitchLLMAction, SwitchLLMObservation]): """Tool for switching a conversation to a saved LLM profile.""" + user_selectable: ClassVar[bool] = False + @classmethod def create( cls, diff --git a/openhands-sdk/openhands/sdk/tool/builtins/think.py b/openhands-sdk/openhands/sdk/tool/builtins/think.py index ca641ce94d..b37cec850a 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/think.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/think.py @@ -1,5 +1,5 @@ from collections.abc import Sequence -from typing import TYPE_CHECKING, Self +from typing import TYPE_CHECKING, ClassVar, Self from pydantic import Field from rich.text import Text @@ -81,6 +81,8 @@ def __call__( class ThinkTool(ToolDefinition[ThinkAction, ThinkObservation]): """Tool for logging thoughts without making changes.""" + user_selectable: ClassVar[bool] = False + @classmethod def create( cls, diff --git a/openhands-sdk/openhands/sdk/tool/builtins/vision_inspect.py b/openhands-sdk/openhands/sdk/tool/builtins/vision_inspect.py index 3b59bfe3b3..136edf27be 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/vision_inspect.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/vision_inspect.py @@ -293,6 +293,8 @@ def __call__( class VisionInspectTool(ToolDefinition[VisionInspectAction, VisionInspectObservation]): """Tool for one-off image inspection through a saved vision profile.""" + user_selectable: ClassVar[bool] = False + name: ClassVar[str] = VISION_INSPECT_TOOL_NAME @classmethod diff --git a/openhands-sdk/openhands/sdk/tool/client_tool.py b/openhands-sdk/openhands/sdk/tool/client_tool.py index 633cdbaf70..d33498820c 100644 --- a/openhands-sdk/openhands/sdk/tool/client_tool.py +++ b/openhands-sdk/openhands/sdk/tool/client_tool.py @@ -12,7 +12,7 @@ import copy import threading from collections.abc import Sequence -from typing import TYPE_CHECKING, Any, Self +from typing import TYPE_CHECKING, Any, ClassVar, Self from pydantic import BaseModel, Field, field_validator @@ -184,6 +184,8 @@ class ClientTool(ToolDefinition[Action, ClientToolObservation]): over WebSocket for the client to handle. """ + user_selectable: ClassVar[bool] = False + client_tool_name: str = Field( description="Per-instance tool name from the ClientToolSpec.", ) diff --git a/openhands-sdk/openhands/sdk/tool/defaults.py b/openhands-sdk/openhands/sdk/tool/defaults.py index 58c43d63de..4faf826c42 100644 --- a/openhands-sdk/openhands/sdk/tool/defaults.py +++ b/openhands-sdk/openhands/sdk/tool/defaults.py @@ -11,6 +11,8 @@ lockstep with these names. """ +from collections.abc import Sequence + from openhands.sdk.tool.spec import Tool @@ -25,16 +27,37 @@ """Name of the browser tool set. Not part of the deterministic default: browser is an environment-dependent -capability, so the serving layer that knows its runtime injects it — the -agent-server appends it on profile launches when ``is_tool_usable`` says the -chromium stack is present, and the cloud conversation-builder does its own -injection. Clients (canvas) add it themselves on the settings launch path. +capability, so the serving layer that knows its runtime passes +``enable_browser`` when the chromium stack is present. Clients (canvas) add it +themselves on the settings launch path. """ SUB_AGENT_TOOL_NAME = "task_tool_set" """Name of the sub-agent delegation tool set, gated on ``enable_sub_agents``.""" +def resolve_tool_specs( + tools: Sequence[Tool] | None, + *, + enable_sub_agents: bool = False, + enable_browser: bool = False, +) -> list[Tool]: + """Resolve an agent's ``tools`` setting into the specs it is built with. + + ``None`` is the standard exec set, plus browser when ``enable_browser`` and + the sub-agent tool set when ``enable_sub_agents``; a list (``[]`` included) + is used as given. + """ + if tools is not None: + return list(tools) + resolved = [Tool(name=name) for name in DEFAULT_EXEC_TOOL_NAMES] + if enable_browser: + resolved.append(Tool(name=BROWSER_TOOL_NAME)) + if enable_sub_agents: + resolved.append(Tool(name=SUB_AGENT_TOOL_NAME)) + return resolved + + def default_tool_specs( *, enable_sub_agents: bool = False, @@ -44,12 +67,8 @@ def default_tool_specs( Deterministic: the same inputs yield the same specs on every runtime. Browser is off by default (see :data:`BROWSER_TOOL_NAME` — the serving - layer injects it where it can actually run); pass ``enable_browser=True`` - to include it explicitly. + layer enables it where it can actually run). """ - names = list(DEFAULT_EXEC_TOOL_NAMES) - if enable_browser: - names.append(BROWSER_TOOL_NAME) - if enable_sub_agents: - names.append(SUB_AGENT_TOOL_NAME) - return [Tool(name=name) for name in names] + return resolve_tool_specs( + None, enable_sub_agents=enable_sub_agents, enable_browser=enable_browser + ) diff --git a/openhands-sdk/openhands/sdk/tool/registry.py b/openhands-sdk/openhands/sdk/tool/registry.py index 021eb79389..a45b3a2814 100644 --- a/openhands-sdk/openhands/sdk/tool/registry.py +++ b/openhands-sdk/openhands/sdk/tool/registry.py @@ -3,6 +3,8 @@ from threading import RLock from typing import TYPE_CHECKING, Any +from pydantic import BaseModel + from openhands.sdk.logger import get_logger from openhands.sdk.tool.spec import Tool from openhands.sdk.tool.tool import ToolDefinition @@ -32,6 +34,16 @@ _REG: dict[str, Resolver] = {} _USABILITY_REG: dict[str, UsabilityChecker] = {} _MODULE_QUALNAMES: dict[str, str] = {} # Maps tool name to module qualname +_TOOL_CLASSES: dict[str, type[ToolDefinition]] = {} +_CATALOG_NAMES: set[str] | None = None + + +class ToolCatalogEntry(BaseModel): + """A registered tool as offered to clients configuring an agent.""" + + name: str + user_selectable: bool = True + usable: bool = True def _resolver_from_instance(name: str, tool: ToolDefinition) -> Resolver: @@ -129,12 +141,7 @@ def register_tool( ".executor, or (2) a ToolDefinition subclass with .create(**params)" ) - # Track the module qualname for this tool - module_qualname = None - if isinstance(factory, type): - module_qualname = factory.__module__ - elif isinstance(factory, ToolDefinition): - module_qualname = factory.__class__.__module__ + tool_class = factory if isinstance(factory, type) else factory.__class__ with _LOCK: # TODO: throw exception when registering duplicate name tools @@ -142,8 +149,8 @@ def register_tool( logger.warning(f"Duplicate tool name registerd {name}") _REG[name] = resolver _USABILITY_REG[name] = usability_checker - if module_qualname: - _MODULE_QUALNAMES[name] = module_qualname + _TOOL_CLASSES[name] = tool_class + _MODULE_QUALNAMES[name] = tool_class.__module__ def resolve_tool( @@ -202,6 +209,37 @@ def list_usable_tools() -> list[str]: ] +def seal_tool_catalog() -> None: + """Freeze the catalog to the tools registered so far. + + A server calls this once it has finished loading its tools. Registrations + after it — a conversation's client tools or dynamically imported modules — + vanish on restart, so they are never offered for configuring an agent. + """ + global _CATALOG_NAMES + with _LOCK: + _CATALOG_NAMES = set(_REG) + + +def list_tool_catalog() -> list[ToolCatalogEntry]: + """List the tools this process offers for configuring an agent.""" + with _LOCK: + names = [ + name for name in _REG if _CATALOG_NAMES is None or name in _CATALOG_NAMES + ] + tool_classes = dict(_TOOL_CLASSES) + usability_checkers = dict(_USABILITY_REG) + + return [ + ToolCatalogEntry( + name=name, + user_selectable=tool_classes[name].user_selectable, + usable=_check_tool_usable(name, usability_checkers.get(name, lambda: True)), + ) + for name in names + ] + + def get_tool_module_qualnames() -> dict[str, str]: """Get a mapping of tool names to their module qualnames. diff --git a/openhands-sdk/openhands/sdk/tool/tool.py b/openhands-sdk/openhands/sdk/tool/tool.py index 40ab0647e4..42a8d2c9fb 100644 --- a/openhands-sdk/openhands/sdk/tool/tool.py +++ b/openhands-sdk/openhands/sdk/tool/tool.py @@ -383,6 +383,9 @@ def create(cls, conv_state, **params): # Automatic tool naming - set by __init_subclass__ name: ClassVar[str] = "" + user_selectable: ClassVar[bool] = True + """Whether a user may pick this tool when configuring an agent's toolset.""" + def __init_subclass__(cls, **kwargs): """Automatically set name from class name when subclass is created.""" super().__init_subclass__(**kwargs) diff --git a/openhands-tools/openhands/tools/gemini/edit/definition.py b/openhands-tools/openhands/tools/gemini/edit/definition.py index 07403ab4f1..915971509a 100644 --- a/openhands-tools/openhands/tools/gemini/edit/definition.py +++ b/openhands-tools/openhands/tools/gemini/edit/definition.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from pydantic import Field @@ -96,6 +96,8 @@ def change_summary(self) -> str: class EditTool(ToolDefinition[EditAction, EditObservation]): """Tool for editing files via find/replace.""" + user_selectable: ClassVar[bool] = False + def declared_resources(self, action: Action) -> DeclaredResources: """Lock on the target file path so concurrent edits to the same file are serialized, while edits to different files run in parallel. diff --git a/openhands-tools/openhands/tools/gemini/list_directory/definition.py b/openhands-tools/openhands/tools/gemini/list_directory/definition.py index c6081b7440..576d005961 100644 --- a/openhands-tools/openhands/tools/gemini/list_directory/definition.py +++ b/openhands-tools/openhands/tools/gemini/list_directory/definition.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from datetime import datetime -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from pydantic import BaseModel, Field from rich.text import Text @@ -141,6 +141,8 @@ def _format_size(self, size: int) -> str: class ListDirectoryTool(ToolDefinition[ListDirectoryAction, ListDirectoryObservation]): """Tool for listing directory contents with metadata.""" + user_selectable: ClassVar[bool] = False + def declared_resources(self, action: Action) -> DeclaredResources: # noqa: ARG002 """Declare resource usage for parallel execution. diff --git a/openhands-tools/openhands/tools/gemini/read_file/definition.py b/openhands-tools/openhands/tools/gemini/read_file/definition.py index 8d62f70563..17b2ff5184 100644 --- a/openhands-tools/openhands/tools/gemini/read_file/definition.py +++ b/openhands-tools/openhands/tools/gemini/read_file/definition.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from pydantic import Field from rich.text import Text @@ -109,6 +109,8 @@ def visualize(self) -> Text: class ReadFileTool(ToolDefinition[ReadFileAction, ReadFileObservation]): """Tool for reading file contents with pagination support.""" + user_selectable: ClassVar[bool] = False + def declared_resources(self, action: Action) -> DeclaredResources: """Lock on the target file path so a read never sees partially-written content from a concurrent write. diff --git a/openhands-tools/openhands/tools/gemini/write_file/definition.py b/openhands-tools/openhands/tools/gemini/write_file/definition.py index 1f9f841eed..cfe6ccac57 100644 --- a/openhands-tools/openhands/tools/gemini/write_file/definition.py +++ b/openhands-tools/openhands/tools/gemini/write_file/definition.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from pydantic import Field @@ -69,6 +69,8 @@ def change_summary(self) -> str: class WriteFileTool(ToolDefinition[WriteFileAction, WriteFileObservation]): """Tool for writing complete file contents.""" + user_selectable: ClassVar[bool] = False + def declared_resources(self, action: Action) -> DeclaredResources: """Lock on the target file path so concurrent writes to the same file are serialized, while writes to different files run in parallel. diff --git a/openhands-tools/openhands/tools/planning_file_editor/definition.py b/openhands-tools/openhands/tools/planning_file_editor/definition.py index ab6aeba975..b36b1ecffe 100644 --- a/openhands-tools/openhands/tools/planning_file_editor/definition.py +++ b/openhands-tools/openhands/tools/planning_file_editor/definition.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar if TYPE_CHECKING: @@ -65,6 +65,8 @@ class PlanningFileEditorTool( ): """A planning file editor tool with read-all, edit-PLAN.md-only access.""" + user_selectable: ClassVar[bool] = False + @classmethod def create( cls, diff --git a/openhands-tools/openhands/tools/task/definition.py b/openhands-tools/openhands/tools/task/definition.py index f1a2d335f3..9b941d0241 100644 --- a/openhands-tools/openhands/tools/task/definition.py +++ b/openhands-tools/openhands/tools/task/definition.py @@ -10,7 +10,7 @@ """ from collections.abc import Sequence -from typing import TYPE_CHECKING, Final +from typing import TYPE_CHECKING, ClassVar, Final from pydantic import Field from pydantic.json_schema import SkipJsonSchema @@ -169,6 +169,8 @@ def to_llm_content(self) -> Sequence[TextContent | ImageContent]: class TaskTool(ToolDefinition[TaskAction, TaskObservation]): """Tool for launching (blocking) sub-agent tasks.""" + user_selectable: ClassVar[bool] = False + def declared_resources(self, action: Action) -> DeclaredResources: # noqa: ARG002 return DeclaredResources(keys=(), declared=True) @@ -213,6 +215,8 @@ class TaskToolSet(ToolDefinition[TaskAction, TaskObservation]): ) """ + user_selectable: ClassVar[bool] = False + @classmethod def create( cls, diff --git a/openhands-tools/openhands/tools/workflow/definition.py b/openhands-tools/openhands/tools/workflow/definition.py index f960ab66f1..89ba7b661d 100644 --- a/openhands-tools/openhands/tools/workflow/definition.py +++ b/openhands-tools/openhands/tools/workflow/definition.py @@ -3,7 +3,7 @@ from __future__ import annotations from collections.abc import Sequence -from typing import TYPE_CHECKING, Final, Literal +from typing import TYPE_CHECKING, ClassVar, Final, Literal from pydantic import Field @@ -142,6 +142,8 @@ class WorkflowTool(ToolDefinition[WorkflowAction, WorkflowObservation]): (e.g., in tests or extensions). """ + user_selectable: ClassVar[bool] = False + @classmethod def create( cls, diff --git a/tests/agent_server/docker_runtime/test_mediation.py b/tests/agent_server/docker_runtime/test_mediation.py index 614329a013..0e61db9d49 100644 --- a/tests/agent_server/docker_runtime/test_mediation.py +++ b/tests/agent_server/docker_runtime/test_mediation.py @@ -112,7 +112,7 @@ async def test_profile_uses_existing_resolver_and_secret_allowlist( ) get_agent_profile_store().save(profile) monkeypatch.setattr( - "openhands.agent_server.conversation_service.discover_profile_skills", + "openhands.agent_server.profile_launch.discover_profile_skills", lambda: [], ) monkeypatch.setattr( diff --git a/tests/agent_server/test_agent_profile_conv_start.py b/tests/agent_server/test_agent_profile_conv_start.py index 3d99f5b8d8..24609616d1 100644 --- a/tests/agent_server/test_agent_profile_conv_start.py +++ b/tests/agent_server/test_agent_profile_conv_start.py @@ -161,7 +161,7 @@ def test_agent_profile_id_present_in_request_payload(self): # Skill discovery is patched so OpenHands-profile resolves don't hit the network # (load_all_skills loads public skills from GitHub). conversation_service imports # discover_profile_skills directly, so patch it in that namespace. -_DISCOVER_PATH = "openhands.agent_server.conversation_service.discover_profile_skills" +_DISCOVER_PATH = "openhands.agent_server.profile_launch.discover_profile_skills" # The profile branch of start_conversation reads the persisted settings through a # local import too, so patch the package-level name it binds. _SETTINGS_STORE_PATH = "openhands.agent_server.persistence.get_settings_store" @@ -197,7 +197,7 @@ def test_openhands_profile_resolves_to_agent_and_stamps_launched(self): # injected iff the host has chromium (covered by the dedicated # injection tests below); this test is about resolution plumbing. patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=False, ), ): @@ -243,7 +243,7 @@ def test_openhands_profile_forces_llm_stream_true(self): patch(_LLM_STORE_PATH), patch(_RESOLVE_PATH, return_value=resolved_settings), patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=False, ), ): @@ -350,25 +350,25 @@ def test_acp_profile_gets_catalog_under_managed_sourcing(self): Disc.assert_called_once() assert MockResolve.call_args.kwargs["available_skills"] == catalog - def test_openhands_default_tools_get_browser_when_usable(self): - """A default-toolset (tools=None) OpenHands profile launch injects the - browser tool set when this server's runtime can run it — the - serving-layer counterpart of the SDK's deterministic default (#3978).""" + @pytest.mark.parametrize("usable", [True, False]) + def test_openhands_launch_passes_runtime_browser_availability(self, usable): + """This server probes its own runtime and hands the answer to the + resolver, which decides whether a default toolset gets the browser.""" from openhands.agent_server.conversation_service import ( _resolve_agent_from_profile, ) profile = _make_openhands_profile() - assert profile.tools is None agent = _make_agent() with ( patch(_STORE_PATH) as MockStore, patch(_LLM_STORE_PATH), patch(_RESOLVE_PATH) as MockResolve, + patch(_DISCOVER_PATH, return_value=[]), patch( - "openhands.agent_server.conversation_service.is_tool_usable", - return_value=True, + "openhands.agent_server.profile_launch.is_tool_usable", + return_value=usable, ) as MockUsable, ): store_inst = MockStore.return_value @@ -383,46 +383,16 @@ def test_openhands_default_tools_get_browser_when_usable(self): ) MockUsable.assert_called_once_with("browser_tool_set") - assert [tool.name for tool in result_agent.tools] == ["browser_tool_set"] - - def test_openhands_default_tools_skip_browser_when_unusable(self): - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - - profile = _make_openhands_profile() - agent = _make_agent() - - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch( - "openhands.agent_server.conversation_service.is_tool_usable", - return_value=False, - ), - ): - store_inst = MockStore.return_value - store_inst.name_for_id.return_value = profile.name - store_inst.load.return_value = profile - mock_config = MagicMock() - mock_config.create_agent.return_value = agent - MockResolve.return_value = mock_config - - result_agent, _, _ = _resolve_agent_from_profile( - profile.id, cipher=None, mcp_config={} - ) - + assert MockResolve.call_args.kwargs["browser_available"] is usable assert result_agent is agent - def test_openhands_explicit_tools_never_amended(self): - """An explicit profile tools list ([] included) is authoritative: the - serving layer must not inject browser on top of it.""" + def test_acp_profile_never_probes_browser(self): + """ACP agents own their tooling, so browser availability is never probed.""" from openhands.agent_server.conversation_service import ( _resolve_agent_from_profile, ) - profile = _make_openhands_profile().model_copy(update={"tools": []}) + profile = _make_acp_profile() agent = _make_agent() with ( @@ -430,7 +400,7 @@ def test_openhands_explicit_tools_never_amended(self): patch(_LLM_STORE_PATH), patch(_RESOLVE_PATH) as MockResolve, patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=True, ) as MockUsable, ): @@ -446,39 +416,44 @@ def test_openhands_explicit_tools_never_amended(self): ) MockUsable.assert_not_called() + assert MockResolve.call_args.kwargs["browser_available"] is False assert result_agent is agent - def test_acp_profile_never_gets_browser_injection(self): - """ACP agents own their tooling — the injection is OpenHands-only.""" + def test_launched_agent_uses_resolved_tools_unchanged(self, tmp_path): + """No tool is added after resolution: the launched agent's tools are + exactly what the resolver produced (the materialize preview's source).""" from openhands.agent_server.conversation_service import ( _resolve_agent_from_profile, ) + from openhands.sdk.llm.llm_profile_store import LLMProfileStore - profile = _make_acp_profile() - agent = _make_agent() + llm_store = LLMProfileStore(base_dir=tmp_path) + llm_store.save("default", LLM(model="gpt-4o"), include_secrets=True) + profile = _make_openhands_profile() with ( patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, + patch(_LLM_STORE_PATH, return_value=llm_store), + patch(_DISCOVER_PATH, return_value=[]), patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=True, - ) as MockUsable, + ), ): store_inst = MockStore.return_value store_inst.name_for_id.return_value = profile.name store_inst.load.return_value = profile - mock_config = MagicMock() - mock_config.create_agent.return_value = agent - MockResolve.return_value = mock_config result_agent, _, _ = _resolve_agent_from_profile( profile.id, cipher=None, mcp_config={} ) - MockUsable.assert_not_called() - assert result_agent is agent + assert [tool.name for tool in result_agent.tools] == [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + ] def test_openhands_default_profile_triggers_discovery(self): """An OpenHands profile always discovers the skill catalog (the deny-list @@ -646,7 +621,7 @@ async def capture_start(stored, **kwargs): # Pin the environment probe: browser injection is covered by its own # tests above and would otherwise vary with the host. patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=False, ), patch.object( @@ -1211,7 +1186,7 @@ def _resolve(self, profile): patch(_RESOLVE_PATH) as MockResolve, patch(_DISCOVER_PATH, return_value=[]), patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=False, ), ): @@ -1277,7 +1252,7 @@ async def test_start_conversation_drops_secrets_the_profile_disallows( patch(_RESOLVE_PATH) as MockResolve, patch(_DISCOVER_PATH, return_value=[]), patch( - "openhands.agent_server.conversation_service.is_tool_usable", + "openhands.agent_server.profile_launch.is_tool_usable", return_value=False, ), patch.object( diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 735be21376..ce93b9367e 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -944,7 +944,7 @@ def test_materialize_reports_disabled_and_resolved_skills( ) with patch( - "openhands.agent_server.agent_profiles_router.discover_profile_skills", + "openhands.agent_server.profile_launch.discover_profile_skills", return_value=[ Skill(name="alpha", content="x"), Skill(name="beta", content="y"), @@ -960,6 +960,98 @@ def test_materialize_reports_disabled_and_resolved_skills( assert body["resolved_settings"] is not None +_BROWSER_PROBE = "openhands.agent_server.profile_launch.is_tool_usable" +_DISCOVER = "openhands.agent_server.profile_launch.discover_profile_skills" + + +def test_materialize_reports_the_tools_a_launch_would_build( + client_with_llm_store, store, llm_store +): + llm_store.save("base-llm", LLM(model="gpt-4o"), include_secrets=True) + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="base-llm")) + + with ( + patch(_BROWSER_PROBE, return_value=True), + patch(_DISCOVER, return_value=[]), + ): + body = client_with_llm_store.post("/api/agent-profiles/p/materialize").json() + + assert [t["name"] for t in body["resolved_settings"]["tools"]] == [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + ] + + +def test_materialize_evaluates_a_draft_without_saving_it( + client_with_llm_store, store, llm_store +): + llm_store.save("base-llm", LLM(model="gpt-4o"), include_secrets=True) + draft = { + "name": "ignored", + "agent_kind": "openhands", + "llm_profile_ref": "base-llm", + "tools": [{"name": "glob"}], + "enable_switch_llm_tool": False, + } + + with ( + patch(_BROWSER_PROBE, return_value=True), + patch(_DISCOVER, return_value=[]), + ): + response = client_with_llm_store.post( + "/api/agent-profiles/draft/materialize", json={"profile": draft} + ) + + assert response.status_code == 200 + body = response.json() + assert body["valid"] is True + assert [t["name"] for t in body["resolved_settings"]["tools"]] == ["glob"] + assert store.list() == [] + + +def test_materialize_draft_takes_precedence_over_the_stored_profile( + client_with_llm_store, store, llm_store +): + llm_store.save("base-llm", LLM(model="gpt-4o"), include_secrets=True) + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="base-llm")) + + with ( + patch(_DISCOVER, return_value=[]), + patch(_BROWSER_PROBE, return_value=False), + ): + drafted = client_with_llm_store.post( + "/api/agent-profiles/p/materialize", + json={"profile": {"name": "p", "llm_profile_ref": "base-llm", "tools": []}}, + ).json() + stored = client_with_llm_store.post( + "/api/agent-profiles/p/materialize", json={} + ).json() + + assert drafted["resolved_settings"]["tools"] == [] + assert [t["name"] for t in stored["resolved_settings"]["tools"]] == [ + "terminal", + "file_editor", + "task_tracker", + ] + + +def test_materialize_invalid_draft_returns_422(client_with_llm_store): + response = client_with_llm_store.post( + "/api/agent-profiles/p/materialize", + json={ + "profile": { + "name": "p", + "llm_profile_ref": "base-llm", + "tools": "terminal", + } + }, + ) + + assert response.status_code == 422 + + def test_materialize_unknown_name_returns_404(client_with_llm_store): """Materializing an unknown profile name returns 404.""" response = client_with_llm_store.post("/api/agent-profiles/ghost/materialize") diff --git a/tests/agent_server/test_server_details_router.py b/tests/agent_server/test_server_details_router.py index df2732eef1..af70d14a8a 100644 --- a/tests/agent_server/test_server_details_router.py +++ b/tests/agent_server/test_server_details_router.py @@ -124,3 +124,9 @@ def test_server_info_advertises_profile_secret_enforcement(client): response = client.get("/server_info") assert response.status_code == 200 assert "profile_secret_scope_v1" in response.json()["capabilities"] + + +def test_server_info_advertises_tool_catalog_and_draft_materialize(client): + capabilities = client.get("/server_info").json()["capabilities"] + assert "tool_catalog_v1" in capabilities + assert "agent_profile_draft_materialize_v1" in capabilities diff --git a/tests/agent_server/test_tool_router.py b/tests/agent_server/test_tool_router.py index d9f25b37bf..42b252abed 100644 --- a/tests/agent_server/test_tool_router.py +++ b/tests/agent_server/test_tool_router.py @@ -26,3 +26,41 @@ def test_builtin_agents_registered_on_tool_router_import(): assert callable(factory.factory_func) _reset_registry_for_tests() + + +def test_catalog_offers_the_stock_tools_a_profile_may_pick(): + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from openhands.agent_server.tool_router import tool_router + + app = FastAPI() + app.include_router(tool_router, prefix="/api") + response = TestClient(app).get("/api/tools/catalog") + + assert response.status_code == 200 + entries = {entry["name"]: entry for entry in response.json()["tools"]} + selectable = {name for name, entry in entries.items() if entry["user_selectable"]} + assert { + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + "glob", + "grep", + "workflow_tool_set", + "ask_oracle", + } <= selectable + assert ( + not { + "task", + "task_tool_set", + "workflow", + "planning_file_editor", + "edit", + "read_file", + "write_file", + "list_directory", + } + & selectable + ) diff --git a/tests/sdk/profiles/test_resolver.py b/tests/sdk/profiles/test_resolver.py index 5282a53e82..fd029f1ed8 100644 --- a/tests/sdk/profiles/test_resolver.py +++ b/tests/sdk/profiles/test_resolver.py @@ -96,26 +96,28 @@ def test_openhands_resolves_to_settings_with_injected_llm( # MCP filtered to the referenced key. assert settings.mcp_config != {} assert list(settings.mcp_config.keys()) == ["fetch"] - # The profile's tools default (None) rides through so create_agent is the - # single defaulting point (#3967 / #3978); the built agent carries the - # standard exec set plus the sub-agent tool set (enable_sub_agents=True). - assert settings.tools is None agent = settings.create_agent() assert isinstance(agent, Agent) - agent_tool_names = [t.name for t in agent.tools] - assert {"terminal", "file_editor", "task_tracker"} <= set(agent_tool_names) - assert "task_tool_set" in agent_tool_names + assert [t.name for t in agent.tools] == [ + "terminal", + "file_editor", + "task_tracker", + "task_tool_set", + ] +@pytest.mark.parametrize( + ("browser_available", "expected"), + [ + (False, ["terminal", "file_editor", "task_tracker"]), + (True, ["terminal", "file_editor", "task_tracker", "browser_tool_set"]), + ], +) def test_openhands_resolves_default_exec_tools( - llm_store: LLMProfileStore, + llm_store: LLMProfileStore, browser_available: bool, expected: list[str] ) -> None: - """A profile with no explicit ``tools`` resolves to ``tools=None``, and - ``create_agent`` attaches the standard exec set (#3967) — otherwise the - agent has only the Finish/Think built-ins and no way to run shell commands - or edit files. The sub-agent tool set stays out when ``enable_sub_agents`` - is False (default); browser is a serving-layer injection, never part of - the deterministic default (see tests/sdk/tool/test_defaults.py).""" + """A profile with no explicit ``tools`` resolves to the standard exec set + (#3967), plus browser only where the caller's runtime can run it.""" profile = OpenHandsAgentProfile(name="oh", llm_profile_ref="default") assert profile.enable_sub_agents is False assert profile.tools is None @@ -126,27 +128,21 @@ def test_openhands_resolves_default_exec_tools( mcp_config={}, available_skills=None, cipher=None, + browser_available=browser_available, ) assert isinstance(settings, OpenHandsAgentSettings) - assert settings.tools is None - # The built agent carries the exec tools, not just the built-ins. - agent = settings.create_agent() - assert [t.name for t in agent.tools] == [ - "terminal", - "file_editor", - "task_tracker", - ] + assert [t.name for t in settings.create_agent().tools] == expected -def test_openhands_profile_tools_selection_is_passed_through( +def test_openhands_profile_tools_selection_is_used_as_given( llm_store: LLMProfileStore, ) -> None: - """An explicit profile ``tools`` list is authoritative: used exactly as - given ([] = deliberately bare), independent of ``enable_sub_agents``.""" + """An explicit profile ``tools`` list is used as given ([] = deliberately + bare) and never gets the browser.""" picked = OpenHandsAgentProfile( name="picked", llm_profile_ref="default", - tools=[Tool(name="terminal")], + tools=[Tool(name="terminal", params={"username": "dev"})], enable_sub_agents=True, ) settings = resolve_agent_profile( @@ -155,10 +151,12 @@ def test_openhands_profile_tools_selection_is_passed_through( mcp_config={}, available_skills=None, cipher=None, + browser_available=True, ) assert isinstance(settings, OpenHandsAgentSettings) - assert settings.tools == [Tool(name="terminal")] - assert [t.name for t in settings.create_agent().tools] == ["terminal"] + assert settings.create_agent().tools == [ + Tool(name="terminal", params={"username": "dev"}) + ] bare = OpenHandsAgentProfile(name="bare", llm_profile_ref="default", tools=[]) settings = resolve_agent_profile( @@ -167,9 +165,9 @@ def test_openhands_profile_tools_selection_is_passed_through( mcp_config={}, available_skills=None, cipher=None, + browser_available=True, ) assert isinstance(settings, OpenHandsAgentSettings) - assert settings.tools == [] assert settings.create_agent().tools == [] @@ -776,6 +774,39 @@ def test_dry_run_verdict_matches_real_resolve( ) +@pytest.mark.parametrize("browser_available", [True, False]) +@pytest.mark.parametrize("tools", [None, [], [Tool(name="glob")]]) +def test_dry_run_tools_match_the_launched_agent( + llm_store: LLMProfileStore, + browser_available: bool, + tools: list[Tool] | None, +) -> None: + """``resolved_settings.tools`` is what the launch actually builds — the + whole point of the preview (#4958).""" + profile = OpenHandsAgentProfile(name="oh", llm_profile_ref="default", tools=tools) + diag = resolve_agent_profile_dry_run( + profile, + llm_store=llm_store, + mcp_config={}, + available_skills=None, + cipher=None, + browser_available=browser_available, + ) + agent = resolve_agent_profile( + profile, + llm_store=llm_store, + mcp_config={}, + available_skills=None, + cipher=None, + browser_available=browser_available, + ).create_agent() + + assert diag.resolved_settings is not None + assert [t["name"] for t in diag.resolved_settings["tools"]] == [ + t.name for t in agent.tools + ] + + def test_dry_run_acp_reports_credential_channels_by_role( llm_store: LLMProfileStore, ) -> None: diff --git a/tests/sdk/tool/test_defaults.py b/tests/sdk/tool/test_defaults.py index 92ade18a1c..b2834fd56b 100644 --- a/tests/sdk/tool/test_defaults.py +++ b/tests/sdk/tool/test_defaults.py @@ -8,7 +8,9 @@ DEFAULT_EXEC_TOOL_NAMES, SUB_AGENT_TOOL_NAME, default_tool_specs, + resolve_tool_specs, ) +from openhands.sdk.tool.spec import Tool def _names(**kwargs) -> list[str]: @@ -37,6 +39,20 @@ def test_explicit_browser_appends_before_sub_agents() -> None: ] +def test_resolve_unset_tools_is_the_default_set() -> None: + assert [t.name for t in resolve_tool_specs(None, enable_browser=True)] == [ + *DEFAULT_EXEC_TOOL_NAMES, + BROWSER_TOOL_NAME, + ] + + +def test_resolve_configured_tools_is_used_as_given() -> None: + """An explicit list is authoritative — browser is never added on top.""" + assert resolve_tool_specs([], enable_browser=True) == [] + spec = Tool(name="terminal", params={"username": "dev"}) + assert resolve_tool_specs([spec], enable_browser=True) == [spec] + + def test_is_tool_usable_contract(monkeypatch: pytest.MonkeyPatch) -> None: assert registry.is_tool_usable("definitely-not-registered") is False monkeypatch.setitem(registry._REG, "probe", lambda params, conv: []) diff --git a/tests/sdk/tool/test_registry.py b/tests/sdk/tool/test_registry.py index 07fcb5a1cf..78bd014545 100644 --- a/tests/sdk/tool/test_registry.py +++ b/tests/sdk/tool/test_registry.py @@ -7,7 +7,14 @@ from openhands.sdk.conversation.state import ConversationState from openhands.sdk.llm.message import ImageContent, TextContent from openhands.sdk.tool import ToolDefinition -from openhands.sdk.tool.registry import list_usable_tools, resolve_tool +from openhands.sdk.tool.client_tool import ClientToolSpec, register_client_tools +from openhands.sdk.tool.registry import ( + list_registered_tools, + list_tool_catalog, + list_usable_tools, + resolve_tool, + seal_tool_catalog, +) from openhands.sdk.tool.schema import Action, Observation from openhands.sdk.tool.spec import Tool from openhands.sdk.tool.tool import ToolExecutor @@ -89,6 +96,14 @@ def is_usable(cls) -> bool: return False +class _InternalHelloTool(_SimpleHelloTool): + user_selectable = False + + +def _catalog() -> dict[str, dict]: + return {entry.name: entry.model_dump() for entry in list_tool_catalog()} + + def _hello_tool_factory(conv_state=None, **params) -> list[ToolDefinition]: return list(_SimpleHelloTool.create(conv_state, **params)) @@ -152,3 +167,39 @@ def test_register_tool_type_uses_create_params(): observation = tool(_HelloAction(name="Alice")) assert isinstance(observation, _HelloObservation) assert observation.message == "Howdy, Alice?" + + +def test_catalog_reports_selectability_and_usability(): + register_tool("catalog_plain", _SimpleHelloTool) + register_tool("catalog_internal", _InternalHelloTool) + register_tool("catalog_unusable", _UnavailableHelloTool) + + catalog = _catalog() + + assert catalog["catalog_plain"] == { + "name": "catalog_plain", + "user_selectable": True, + "usable": True, + } + assert catalog["catalog_internal"]["user_selectable"] is False + assert catalog["catalog_unusable"]["usable"] is False + + +def test_sealed_catalog_ignores_later_registrations(monkeypatch): + """Tools a conversation registers vanish on restart, so a server seals the + catalog once its own tools are loaded.""" + from openhands.sdk.tool import registry + + register_tool("catalog_at_startup", _SimpleHelloTool) + monkeypatch.setattr(registry, "_CATALOG_NAMES", None) + seal_tool_catalog() + + register_tool("catalog_after_seal", _SimpleHelloTool) + register_client_tools( + [ClientToolSpec(name="catalog_client_tool", description="client side")] + ) + + assert "catalog_at_startup" in _catalog() + assert "catalog_after_seal" in list_registered_tools() + assert "catalog_after_seal" not in _catalog() + assert "catalog_client_tool" not in _catalog() From 875d2b8414ba10ff7e4b347c77c4eb37a409170d Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:28:53 -0400 Subject: [PATCH 02/12] chore: address PR review feedback (#5151) Drop the stale field descriptions that promised enable_sub_agents would apply to an explicit tools list; that change lives in #5158. Co-Authored-By: Claude Opus 5 (1M context) --- openhands-sdk/openhands/sdk/profiles/agent_profile.py | 3 +-- openhands-sdk/openhands/sdk/settings/model.py | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index 01de246a2b..cf99bd4ab0 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -169,8 +169,7 @@ class OpenHandsAgentProfile(AgentProfileBase): description=( "Tool selection for the resolved agent. None (the default) = the " "server's standard tool set; [] = an explicitly bare agent; a " - "non-empty list is used as given. enable_sub_agents adds the " - "sub-agent tool set in every case." + "non-empty list is used exactly as given." ), ) diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 4e19ddcd97..499620ce0a 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1275,9 +1275,9 @@ class OpenHandsAgentSettings(AgentSettingsBase): default=None, description=( "Tools available to the agent. None (the default) resolves to the " - "standard exec set (see openhands.sdk.tool.defaults); [] is an " - "explicitly bare agent; a non-empty list is used as given. " - "enable_sub_agents adds the sub-agent tool set in every case. " + "standard exec set (see openhands.sdk.tool.defaults), plus the " + "sub-agent tool set when enable_sub_agents is set; [] is an " + "explicitly bare agent; a non-empty list is used exactly as given. " "Environment-dependent tools (browser) are injected by the serving " "layer, not the default." ), From 2695be607abe9985a2c8445a41eaca6d1d92aae2 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 19:42:17 -0400 Subject: [PATCH 03/12] refactor(profiles): select delegation and LLM switching in tools `enable_sub_agents` and `enable_switch_llm_tool` were separate controls over which tools an agent gets, so the same question had two answers and a tool catalog could not describe either. Both are retired from `OpenHandsAgentProfile`; `task_tool_set` and `SwitchLLMTool` become user-selectable catalog entries picked in `tools` like any other tool. Schema v3 migrates stored profiles: a profile with delegation on has its `tools` pinned to the standard set plus `task_tool_set`, since "the standard set plus delegation" is no longer expressible. Browser is part of that pin because it resolves to nothing where the runtime cannot run it. `enable_switch_llm_tool` defaulted on, so folding it would pin a list on nearly every profile; it is dropped instead and `switch_llm` must be selected again. Both switches remain on `AgentSettingsConfig` for the legacy settings launch path. Co-Authored-By: Claude Opus 5 (1M context) --- .../openhands/sdk/profiles/agent_profile.py | 64 +++++++++++++---- .../openhands/sdk/profiles/resolver.py | 7 +- openhands-sdk/openhands/sdk/profiles/seed.py | 11 +-- openhands-sdk/openhands/sdk/settings/model.py | 13 +++- .../openhands/sdk/tool/builtins/switch_llm.py | 2 +- openhands-sdk/openhands/sdk/tool/defaults.py | 21 +++--- openhands-sdk/openhands/sdk/tool/registry.py | 18 ++++- .../openhands/tools/task/definition.py | 2 +- .../test_agent_profiles_router.py | 14 +++- tests/agent_server/test_tool_router.py | 3 +- .../test_check_persisted_settings_compat.py | 2 +- .../v3/agent_profile_default.json | 16 +++++ tests/sdk/profiles/test_agent_profile.py | 70 +++++++++++++++++-- tests/sdk/profiles/test_resolver.py | 44 +----------- 14 files changed, 197 insertions(+), 90 deletions(-) create mode 100644 tests/sdk/persisted_settings_baselines/v3/agent_profile_default.json diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index cf99bd4ab0..f2c0babf0c 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -9,7 +9,7 @@ from __future__ import annotations -from collections.abc import Callable, Mapping +from collections.abc import Callable, Mapping, Sequence from typing import Annotated, Any, Literal from uuid import UUID, uuid4 @@ -31,9 +31,14 @@ VerificationSettings, ) from openhands.sdk.tool import Tool +from openhands.sdk.tool.defaults import ( + BROWSER_TOOL_NAME, + DEFAULT_EXEC_TOOL_NAMES, + SUB_AGENT_TOOL_NAME, +) -AGENT_PROFILE_SCHEMA_VERSION = 2 +AGENT_PROFILE_SCHEMA_VERSION = 3 class ProfileVerificationSettings(BaseModel): @@ -197,18 +202,6 @@ class OpenHandsAgentProfile(AgentProfileBase): default_factory=ProfileVerificationSettings, description="Critic/verification policy (secret-free; no critic_api_key).", ) - enable_sub_agents: bool = Field( - default=False, - description="Enable sub-agent delegation via TaskToolSet.", - ) - enable_switch_llm_tool: bool = Field( - default=True, - description=( - "Enable the built-in switch_llm tool for switching between saved " - "LLM profiles. Defaults True to match the global agent settings " - "default (AgentSettingsConfig.enable_switch_llm_tool)." - ), - ) tool_concurrency_limit: int = Field( default=1, ge=1, @@ -364,8 +357,51 @@ def _migrate_v1_to_v2(payload: dict[str, Any]) -> dict[str, Any]: return migrated +def fold_sub_agents_into_tools( + tools: Sequence[dict[str, Any] | Tool] | None, + *, + enable_sub_agents: bool, +) -> list[Tool] | None: + """Express a legacy ``enable_sub_agents`` switch as a ``tools`` selection.""" + if not enable_sub_agents: + return None if tools is None else [_as_tool(tool) for tool in tools] + # "The standard set plus delegation" is not expressible without the switch, + # so an unset list has to be pinned. Browser is part of that set because it + # resolves to nothing where the runtime cannot run it. + entries = ( + [_as_tool(tool) for tool in tools] + if tools is not None + else [Tool(name=name) for name in (*DEFAULT_EXEC_TOOL_NAMES, BROWSER_TOOL_NAME)] + ) + if all(entry.name != SUB_AGENT_TOOL_NAME for entry in entries): + entries.append(Tool(name=SUB_AGENT_TOOL_NAME)) + return entries + + +def _as_tool(tool: dict[str, Any] | Tool) -> Tool: + return tool if isinstance(tool, Tool) else Tool.model_validate(tool) + + +def _migrate_v2_to_v3(payload: dict[str, Any]) -> dict[str, Any]: + """Fold the retired tool switches into ``tools``.""" + migrated = dict(payload) + sub_agents = migrated.pop("enable_sub_agents", False) is True + # Dropped rather than folded: it defaulted on, so honouring it would pin a + # list on nearly every profile. + migrated.pop("enable_switch_llm_tool", None) + if sub_agents and migrated.get("agent_kind", "openhands") == "openhands": + stored = migrated.get("tools") + migrated["tools"] = fold_sub_agents_into_tools( + stored if isinstance(stored, list) else None, + enable_sub_agents=True, + ) + migrated["schema_version"] = 3 + return migrated + + _AGENT_PROFILE_MIGRATIONS: dict[int, PersistedProfileMigrator] = { 1: _migrate_v1_to_v2, + 2: _migrate_v2_to_v3, } diff --git a/openhands-sdk/openhands/sdk/profiles/resolver.py b/openhands-sdk/openhands/sdk/profiles/resolver.py index f8e7b875b1..e8d8c4dc74 100644 --- a/openhands-sdk/openhands/sdk/profiles/resolver.py +++ b/openhands-sdk/openhands/sdk/profiles/resolver.py @@ -252,7 +252,6 @@ def _build_openhands_settings( "mcp_config": mcp_config, "tools": resolve_tool_specs( profile.tools, - enable_sub_agents=profile.enable_sub_agents, enable_browser=browser_available, ), "agent_context": AgentContext( @@ -263,8 +262,10 @@ def _build_openhands_settings( ), "condenser": profile.condenser, "verification": profile.verification.model_dump(), - "enable_sub_agents": profile.enable_sub_agents, - "enable_switch_llm_tool": profile.enable_switch_llm_tool, + # Pinned off so the settings defaults cannot re-add a tool the + # profile's ``tools`` did not ask for. + "enable_sub_agents": False, + "enable_switch_llm_tool": False, "tool_concurrency_limit": profile.tool_concurrency_limit, } return validate_agent_settings(payload) diff --git a/openhands-sdk/openhands/sdk/profiles/seed.py b/openhands-sdk/openhands/sdk/profiles/seed.py index e9cce1f13a..1fb1f11c49 100644 --- a/openhands-sdk/openhands/sdk/profiles/seed.py +++ b/openhands-sdk/openhands/sdk/profiles/seed.py @@ -14,6 +14,7 @@ ACPAgentProfile, OpenHandsAgentProfile, build_profile_verification, + fold_sub_agents_into_tools, ) @@ -63,8 +64,12 @@ def build_seed_profile( name=name, llm_profile_ref=active_llm_profile or SEED_PROFILE_NAME, agent=agent_settings.agent, - # Verbatim: preserves explicit toolsets; None stays "server default". - tools=agent_settings.tools, + # Verbatim, except that a legacy ``enable_sub_agents`` switch has to be + # said as a tool selection now (see fold_sub_agents_into_tools). + tools=fold_sub_agents_into_tools( + agent_settings.tools, + enable_sub_agents=agent_settings.enable_sub_agents, + ), # Deny-list defaults to [] — the seeded default profile launches with all # discovered skills, matching the "all skills by default" model. No names # are frozen, so nothing can dangle at launch (the freeze-by-name seed @@ -73,8 +78,6 @@ def build_seed_profile( system_message_suffix=context.system_message_suffix, condenser=agent_settings.condenser, verification=build_profile_verification(agent_settings.verification), - enable_sub_agents=agent_settings.enable_sub_agents, - enable_switch_llm_tool=agent_settings.enable_switch_llm_tool, tool_concurrency_limit=agent_settings.tool_concurrency_limit, mcp_server_refs=None, ) diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 499620ce0a..ce3a5f5ce7 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1398,9 +1398,18 @@ def create_agent(self) -> Agent: from openhands.sdk.agent import Agent from openhands.sdk.llm.auth.openai import create_subscription_llm_from_config from openhands.sdk.tool.builtins import BUILT_IN_TOOLS, SwitchLLMTool - from openhands.sdk.tool.defaults import resolve_tool_specs + from openhands.sdk.tool.defaults import ( + SUB_AGENT_TOOL_NAME, + resolve_tool_specs, + ) - tools = resolve_tool_specs(self.tools, enable_sub_agents=self.enable_sub_agents) + # Legacy switches of this settings model: the tools they add are + # otherwise selected in ``tools``. + tools = resolve_tool_specs(self.tools) + if self.enable_sub_agents and all( + tool.name != SUB_AGENT_TOOL_NAME for tool in tools + ): + tools = [*tools, Tool(name=SUB_AGENT_TOOL_NAME)] include_default_tools = [tool.__name__ for tool in BUILT_IN_TOOLS] if self.enable_switch_llm_tool: diff --git a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py index a00a55dde8..f8708ef123 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py @@ -151,7 +151,7 @@ def __call__( class SwitchLLMTool(ToolDefinition[SwitchLLMAction, SwitchLLMObservation]): """Tool for switching a conversation to a saved LLM profile.""" - user_selectable: ClassVar[bool] = False + user_selectable: ClassVar[bool] = True @classmethod def create( diff --git a/openhands-sdk/openhands/sdk/tool/defaults.py b/openhands-sdk/openhands/sdk/tool/defaults.py index 4faf826c42..a49d373575 100644 --- a/openhands-sdk/openhands/sdk/tool/defaults.py +++ b/openhands-sdk/openhands/sdk/tool/defaults.py @@ -33,28 +33,24 @@ """ SUB_AGENT_TOOL_NAME = "task_tool_set" -"""Name of the sub-agent delegation tool set, gated on ``enable_sub_agents``.""" +"""Name of the sub-agent delegation tool set, selected like any other tool.""" def resolve_tool_specs( tools: Sequence[Tool] | None, *, - enable_sub_agents: bool = False, enable_browser: bool = False, ) -> list[Tool]: """Resolve an agent's ``tools`` setting into the specs it is built with. - ``None`` is the standard exec set, plus browser when ``enable_browser`` and - the sub-agent tool set when ``enable_sub_agents``; a list (``[]`` included) - is used as given. + ``None`` is the standard exec set, plus browser when ``enable_browser``; a + list (``[]`` included) is used as given. """ if tools is not None: return list(tools) resolved = [Tool(name=name) for name in DEFAULT_EXEC_TOOL_NAMES] if enable_browser: resolved.append(Tool(name=BROWSER_TOOL_NAME)) - if enable_sub_agents: - resolved.append(Tool(name=SUB_AGENT_TOOL_NAME)) return resolved @@ -65,10 +61,15 @@ def default_tool_specs( ) -> list[Tool]: """Default tool specs for an OpenHands agent whose settings carry no tools. + ``enable_sub_agents`` is retained for the legacy ``agent_settings`` path, + where the switch still exists; agent profiles select the sub-agent tool set + in ``tools`` instead. + Deterministic: the same inputs yield the same specs on every runtime. Browser is off by default (see :data:`BROWSER_TOOL_NAME` — the serving layer enables it where it can actually run). """ - return resolve_tool_specs( - None, enable_sub_agents=enable_sub_agents, enable_browser=enable_browser - ) + specs = resolve_tool_specs(None, enable_browser=enable_browser) + if enable_sub_agents: + specs.append(Tool(name=SUB_AGENT_TOOL_NAME)) + return specs diff --git a/openhands-sdk/openhands/sdk/tool/registry.py b/openhands-sdk/openhands/sdk/tool/registry.py index a45b3a2814..a26250a310 100644 --- a/openhands-sdk/openhands/sdk/tool/registry.py +++ b/openhands-sdk/openhands/sdk/tool/registry.py @@ -222,7 +222,14 @@ def seal_tool_catalog() -> None: def list_tool_catalog() -> list[ToolCatalogEntry]: - """List the tools this process offers for configuring an agent.""" + """List the tools this process offers for configuring an agent. + + Includes the built-ins a user may select: they are resolved by class name + rather than through the registry, but a profile stores them in ``tools`` + like any other pick. + """ + from openhands.sdk.tool.builtins import BUILT_IN_TOOL_CLASSES + with _LOCK: names = [ name for name in _REG if _CATALOG_NAMES is None or name in _CATALOG_NAMES @@ -230,7 +237,7 @@ def list_tool_catalog() -> list[ToolCatalogEntry]: tool_classes = dict(_TOOL_CLASSES) usability_checkers = dict(_USABILITY_REG) - return [ + entries = [ ToolCatalogEntry( name=name, user_selectable=tool_classes[name].user_selectable, @@ -238,6 +245,13 @@ def list_tool_catalog() -> list[ToolCatalogEntry]: ) for name in names ] + listed = {entry.name for entry in entries} + entries.extend( + ToolCatalogEntry(name=class_name, user_selectable=True, usable=True) + for class_name, tool_class in BUILT_IN_TOOL_CLASSES.items() + if tool_class.user_selectable and class_name not in listed + ) + return entries def get_tool_module_qualnames() -> dict[str, str]: diff --git a/openhands-tools/openhands/tools/task/definition.py b/openhands-tools/openhands/tools/task/definition.py index 9b941d0241..28eaab1c60 100644 --- a/openhands-tools/openhands/tools/task/definition.py +++ b/openhands-tools/openhands/tools/task/definition.py @@ -215,7 +215,7 @@ class TaskToolSet(ToolDefinition[TaskAction, TaskObservation]): ) """ - user_selectable: ClassVar[bool] = False + user_selectable: ClassVar[bool] = True @classmethod def create( diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index ce93b9367e..f9dc0fc749 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -728,8 +728,17 @@ def test_seed_preserves_openhands_fields(client): client.get("/api/agent-profiles") # triggers the seed prof = client.get("/api/agent-profiles/default").json()["profile"] - assert prof["enable_sub_agents"] is True - assert prof["enable_switch_llm_tool"] is False + # The retired switches are said as a tool selection now: delegation was on, + # so the seed pins the list it was launching with. + assert [tool["name"] for tool in prof["tools"]] == [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + "task_tool_set", + ] + assert "enable_sub_agents" not in prof + assert "enable_switch_llm_tool" not in prof assert prof["tool_concurrency_limit"] == 3 assert prof["system_message_suffix"] == "be terse" # The seed disables nothing — the default profile launches with all @@ -993,7 +1002,6 @@ def test_materialize_evaluates_a_draft_without_saving_it( "agent_kind": "openhands", "llm_profile_ref": "base-llm", "tools": [{"name": "glob"}], - "enable_switch_llm_tool": False, } with ( diff --git a/tests/agent_server/test_tool_router.py b/tests/agent_server/test_tool_router.py index 42b252abed..db0be5951e 100644 --- a/tests/agent_server/test_tool_router.py +++ b/tests/agent_server/test_tool_router.py @@ -50,11 +50,12 @@ def test_catalog_offers_the_stock_tools_a_profile_may_pick(): "grep", "workflow_tool_set", "ask_oracle", + "task_tool_set", + "SwitchLLMTool", } <= selectable assert ( not { "task", - "task_tool_set", "workflow", "planning_file_editor", "edit", diff --git a/tests/cross/test_check_persisted_settings_compat.py b/tests/cross/test_check_persisted_settings_compat.py index ec4df7e448..345e65f826 100644 --- a/tests/cross/test_check_persisted_settings_compat.py +++ b/tests/cross/test_check_persisted_settings_compat.py @@ -138,7 +138,7 @@ def test_collect_fixture_cases_and_validate_current_repo_fixtures() -> None: assert versions_by_surface == { "agent_settings": {1, 2, 3, 4, 5, 6}, - "agent_profile": {1, 2}, + "agent_profile": {1, 2, 3}, "conversation_settings": {1}, "persisted_settings": {1, 2, 3}, } diff --git a/tests/sdk/persisted_settings_baselines/v3/agent_profile_default.json b/tests/sdk/persisted_settings_baselines/v3/agent_profile_default.json new file mode 100644 index 0000000000..1001ac21c9 --- /dev/null +++ b/tests/sdk/persisted_settings_baselines/v3/agent_profile_default.json @@ -0,0 +1,16 @@ +{ + "schema_version": 3, + "name": "default", + "llm_profile_ref": "default", + "revision": 0, + "disabled_skills": [], + "tools": [ + { "name": "terminal", "params": {} }, + { "name": "task_tool_set", "params": {} } + ], + "__expected__": { + "name": "default", + "llm_profile_ref": "default", + "disabled_skills": [] + } +} diff --git a/tests/sdk/profiles/test_agent_profile.py b/tests/sdk/profiles/test_agent_profile.py index 2e3107244e..26eebd1b90 100644 --- a/tests/sdk/profiles/test_agent_profile.py +++ b/tests/sdk/profiles/test_agent_profile.py @@ -39,8 +39,6 @@ def test_openhands_profile_round_trips() -> None: mcp_server_refs=["fetch"], disabled_skills=["pdf-tools"], system_message_suffix="be terse", - enable_sub_agents=True, - enable_switch_llm_tool=False, tool_concurrency_limit=4, ) reloaded = validate_agent_profile(profile.model_dump(mode="json")) @@ -53,7 +51,6 @@ def test_openhands_profile_round_trips() -> None: assert reloaded.revision == 3 assert reloaded.mcp_server_refs == ["fetch"] assert reloaded.disabled_skills == ["pdf-tools"] - assert reloaded.enable_switch_llm_tool is False assert reloaded.tool_concurrency_limit == 4 @@ -63,13 +60,11 @@ def test_openhands_profile_new_field_defaults() -> None: discovered skills" (#4017). Skills are selected by exclusion, never an allow-list of names that could dangle.""" profile = OpenHandsAgentProfile(name="oh", llm_profile_ref="default") - assert profile.enable_switch_llm_tool is True assert profile.disabled_skills == [] reloaded = validate_agent_profile( {"agent_kind": "openhands", "name": "oh", "llm_profile_ref": "default"} ) assert isinstance(reloaded, OpenHandsAgentProfile) - assert reloaded.enable_switch_llm_tool is True assert reloaded.disabled_skills == [] @@ -351,6 +346,71 @@ def test_v1_explicit_empty_tools_remain_empty(payload: dict[str, object]) -> Non assert profile.tools == [] +def test_v2_sub_agents_switch_pins_the_standard_set_plus_delegation() -> None: + profile = validate_agent_profile( + { + "schema_version": 2, + "name": "default", + "llm_profile_ref": "default", + "revision": 0, + "enable_sub_agents": True, + } + ) + assert isinstance(profile, OpenHandsAgentProfile) + assert profile.schema_version == AGENT_PROFILE_SCHEMA_VERSION + assert [tool.name for tool in profile.tools or []] == [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + "task_tool_set", + ] + + +def test_v2_sub_agents_switch_appends_to_an_explicit_list() -> None: + profile = validate_agent_profile( + { + "schema_version": 2, + "name": "default", + "llm_profile_ref": "default", + "revision": 0, + "tools": [{"name": "glob", "params": {}}], + "enable_sub_agents": True, + } + ) + assert isinstance(profile, OpenHandsAgentProfile) + assert [tool.name for tool in profile.tools or []] == ["glob", "task_tool_set"] + + +@pytest.mark.parametrize("switch_llm", [True, False]) +def test_v2_switch_llm_flag_is_dropped_without_pinning_tools(switch_llm: bool) -> None: + profile = validate_agent_profile( + { + "schema_version": 2, + "name": "default", + "llm_profile_ref": "default", + "revision": 0, + "enable_switch_llm_tool": switch_llm, + } + ) + assert isinstance(profile, OpenHandsAgentProfile) + assert profile.tools is None + assert not hasattr(profile, "enable_switch_llm_tool") + + +def test_v2_sub_agents_switch_leaves_acp_profiles_alone() -> None: + profile = validate_agent_profile( + { + "schema_version": 2, + "agent_kind": "acp", + "name": "acp", + "enable_sub_agents": True, + } + ) + assert profile.schema_version == AGENT_PROFILE_SCHEMA_VERSION + assert not hasattr(profile, "tools") + + def test_rejects_newer_schema_version() -> None: with pytest.raises(ValueError, match="newer than supported"): validate_agent_profile( diff --git a/tests/sdk/profiles/test_resolver.py b/tests/sdk/profiles/test_resolver.py index fd029f1ed8..2668487d83 100644 --- a/tests/sdk/profiles/test_resolver.py +++ b/tests/sdk/profiles/test_resolver.py @@ -72,7 +72,6 @@ def test_openhands_resolves_to_settings_with_injected_llm( llm_profile_ref="default", agent="CodeActAgent", system_message_suffix="be terse", - enable_sub_agents=True, tool_concurrency_limit=3, mcp_server_refs=["fetch"], ) @@ -86,7 +85,6 @@ def test_openhands_resolves_to_settings_with_injected_llm( assert isinstance(settings, OpenHandsAgentSettings) assert settings.agent == "CodeActAgent" - assert settings.enable_sub_agents is True assert settings.tool_concurrency_limit == 3 assert settings.agent_context is not None assert settings.agent_context.system_message_suffix == "be terse" @@ -98,11 +96,11 @@ def test_openhands_resolves_to_settings_with_injected_llm( assert list(settings.mcp_config.keys()) == ["fetch"] agent = settings.create_agent() assert isinstance(agent, Agent) + # Delegation is a tool the profile selects, not a switch on the side. assert [t.name for t in agent.tools] == [ "terminal", "file_editor", "task_tracker", - "task_tool_set", ] @@ -119,7 +117,6 @@ def test_openhands_resolves_default_exec_tools( """A profile with no explicit ``tools`` resolves to the standard exec set (#3967), plus browser only where the caller's runtime can run it.""" profile = OpenHandsAgentProfile(name="oh", llm_profile_ref="default") - assert profile.enable_sub_agents is False assert profile.tools is None settings = resolve_agent_profile( @@ -143,7 +140,6 @@ def test_openhands_profile_tools_selection_is_used_as_given( name="picked", llm_profile_ref="default", tools=[Tool(name="terminal", params={"username": "dev"})], - enable_sub_agents=True, ) settings = resolve_agent_profile( picked, @@ -226,44 +222,6 @@ def test_missing_llm_ref_raises_profile_not_found( ) -# --------------------------------------------------------------------------- # -# enable_switch_llm_tool (#3856) -# --------------------------------------------------------------------------- # - - -def test_enable_switch_llm_tool_defaults_true_threads_through( - llm_store: LLMProfileStore, mcp_config: dict[str, MCPServer] -) -> None: - profile = OpenHandsAgentProfile(name="oh", llm_profile_ref="default") - settings = resolve_agent_profile( - profile, - llm_store=llm_store, - mcp_config=mcp_config, - available_skills=None, - cipher=None, - ) - assert isinstance(settings, OpenHandsAgentSettings) - # Defaults True to match the global agent settings default. - assert settings.enable_switch_llm_tool is True - - -def test_enable_switch_llm_tool_false_threads_through( - llm_store: LLMProfileStore, mcp_config: dict[str, MCPServer] -) -> None: - profile = OpenHandsAgentProfile( - name="oh", llm_profile_ref="default", enable_switch_llm_tool=False - ) - settings = resolve_agent_profile( - profile, - llm_store=llm_store, - mcp_config=mcp_config, - available_skills=None, - cipher=None, - ) - assert isinstance(settings, OpenHandsAgentSettings) - assert settings.enable_switch_llm_tool is False - - # --------------------------------------------------------------------------- # # disabled_skills deny-list over discovered skills (#4017) # --------------------------------------------------------------------------- # From 681d891d45ea5d93d0eed3ae0cb2f25f5f00c4f8 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 21:01:49 -0400 Subject: [PATCH 04/12] fix(tool): derive a built-in's catalog usability from its class The built-in branch of the catalog hardcoded usable=True instead of asking the class, so a selectable built-in with a runtime-conditional is_usable() would be offered on runtimes that cannot run it. Co-Authored-By: Claude Opus 5 (1M context) --- openhands-sdk/openhands/sdk/tool/registry.py | 6 +++++- tests/sdk/tool/test_registry.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/tool/registry.py b/openhands-sdk/openhands/sdk/tool/registry.py index a26250a310..2d78bd19e2 100644 --- a/openhands-sdk/openhands/sdk/tool/registry.py +++ b/openhands-sdk/openhands/sdk/tool/registry.py @@ -247,7 +247,11 @@ def list_tool_catalog() -> list[ToolCatalogEntry]: ] listed = {entry.name for entry in entries} entries.extend( - ToolCatalogEntry(name=class_name, user_selectable=True, usable=True) + ToolCatalogEntry( + name=class_name, + user_selectable=True, + usable=_check_tool_usable(class_name, _usability_from_subclass(tool_class)), + ) for class_name, tool_class in BUILT_IN_TOOL_CLASSES.items() if tool_class.user_selectable and class_name not in listed ) diff --git a/tests/sdk/tool/test_registry.py b/tests/sdk/tool/test_registry.py index 78bd014545..f15f1b116e 100644 --- a/tests/sdk/tool/test_registry.py +++ b/tests/sdk/tool/test_registry.py @@ -185,6 +185,22 @@ def test_catalog_reports_selectability_and_usability(): assert catalog["catalog_unusable"]["usable"] is False +def test_catalog_reports_a_selectable_builtin_as_unusable(monkeypatch): + """A built-in is listed by class name, so its usability comes from the class.""" + from openhands.sdk.tool import builtins, registry + + monkeypatch.setitem( + builtins.BUILT_IN_TOOL_CLASSES, "UnusableBuiltin", _UnavailableHelloTool + ) + monkeypatch.setattr(registry, "_CATALOG_NAMES", None) + + assert _catalog()["UnusableBuiltin"] == { + "name": "UnusableBuiltin", + "user_selectable": True, + "usable": False, + } + + def test_sealed_catalog_ignores_later_registrations(monkeypatch): """Tools a conversation registers vanish on restart, so a server seals the catalog once its own tools are loaded.""" From 9c9a63cd200c4f8e4ee9c91480a57a6f244711a5 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Sat, 19 Sep 2026 09:20:35 +0100 Subject: [PATCH 05/12] feat(tool): give each catalog entry a blurb and a consistent name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the picker needs from the catalog. A `catalog_description` ClassVar carries one line per tool, written for a user rather than the model — `description` is the LLM's prompt and runs to paragraphs. It lives on the class so the catalog can read it without instantiating anything, and so a tool owns its own blurb. Built-ins were offered under their class name (`SwitchLLMTool`) while every other tool used its snake_case name (`terminal`, `task_tool_set`). They now use `ToolDefinition.name`, which is already snake_case, and `resolve_tool` accepts that name so a stored pick resolves. Co-Authored-By: Claude Opus 5 (1M context) --- .../openhands/sdk/tool/builtins/switch_llm.py | 4 ++ openhands-sdk/openhands/sdk/tool/registry.py | 20 ++++++--- openhands-sdk/openhands/sdk/tool/tool.py | 7 +++ .../openhands/tools/ask_oracle/definition.py | 6 ++- .../openhands/tools/browser_use/definition.py | 4 ++ .../openhands/tools/file_editor/definition.py | 4 +- .../openhands/tools/glob/definition.py | 4 +- .../openhands/tools/grep/definition.py | 6 ++- .../openhands/tools/task/definition.py | 4 ++ .../tools/task_tracker/definition.py | 6 ++- .../openhands/tools/terminal/definition.py | 6 ++- .../openhands/tools/workflow/definition.py | 2 + tests/agent_server/test_tool_router.py | 6 ++- tests/sdk/tool/test_registry.py | 43 ++++++++++++++++++- 14 files changed, 108 insertions(+), 14 deletions(-) diff --git a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py index f8708ef123..b14297ad0b 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py @@ -151,6 +151,10 @@ def __call__( class SwitchLLMTool(ToolDefinition[SwitchLLMAction, SwitchLLMObservation]): """Tool for switching a conversation to a saved LLM profile.""" + catalog_description: ClassVar[str] = ( + "Let the agent switch the conversation to another saved LLM profile." + ) + user_selectable: ClassVar[bool] = True @classmethod diff --git a/openhands-sdk/openhands/sdk/tool/registry.py b/openhands-sdk/openhands/sdk/tool/registry.py index 2d78bd19e2..f9216f051b 100644 --- a/openhands-sdk/openhands/sdk/tool/registry.py +++ b/openhands-sdk/openhands/sdk/tool/registry.py @@ -44,6 +44,7 @@ class ToolCatalogEntry(BaseModel): name: str user_selectable: bool = True usable: bool = True + description: str = "" def _resolver_from_instance(name: str, tool: ToolDefinition) -> Resolver: @@ -162,7 +163,10 @@ def resolve_tool( if resolver is None: from openhands.sdk.tool.builtins import BUILT_IN_TOOL_CLASSES - tool_class = BUILT_IN_TOOL_CLASSES.get(tool_spec.name) + tool_class = BUILT_IN_TOOL_CLASSES.get(tool_spec.name) or next( + (c for c in BUILT_IN_TOOL_CLASSES.values() if c.name == tool_spec.name), + None, + ) if tool_class is None: raise KeyError(f"ToolDefinition '{tool_spec.name}' is not registered") resolver = _resolver_from_subclass(tool_spec.name, tool_class) @@ -242,18 +246,24 @@ def list_tool_catalog() -> list[ToolCatalogEntry]: name=name, user_selectable=tool_classes[name].user_selectable, usable=_check_tool_usable(name, usability_checkers.get(name, lambda: True)), + description=tool_classes[name].catalog_description, ) for name in names ] + # Built-ins are keyed by class name, but a profile stores the same snake_case + # tool name as every other pick. listed = {entry.name for entry in entries} entries.extend( ToolCatalogEntry( - name=class_name, + name=tool_class.name, user_selectable=True, - usable=_check_tool_usable(class_name, _usability_from_subclass(tool_class)), + usable=_check_tool_usable( + tool_class.name, _usability_from_subclass(tool_class) + ), + description=tool_class.catalog_description, ) - for class_name, tool_class in BUILT_IN_TOOL_CLASSES.items() - if tool_class.user_selectable and class_name not in listed + for tool_class in BUILT_IN_TOOL_CLASSES.values() + if tool_class.user_selectable and tool_class.name not in listed ) return entries diff --git a/openhands-sdk/openhands/sdk/tool/tool.py b/openhands-sdk/openhands/sdk/tool/tool.py index 42a8d2c9fb..19fcc229a5 100644 --- a/openhands-sdk/openhands/sdk/tool/tool.py +++ b/openhands-sdk/openhands/sdk/tool/tool.py @@ -386,6 +386,13 @@ def create(cls, conv_state, **params): user_selectable: ClassVar[bool] = True """Whether a user may pick this tool when configuring an agent's toolset.""" + catalog_description: ClassVar[str] = "" + """One line telling a user what this tool lets the agent do. + + For the tool catalog, not the model: ``description`` is the prompt the LLM + reads. Empty means the catalog offers no blurb. + """ + def __init_subclass__(cls, **kwargs): """Automatically set name from class name when subclass is created.""" super().__init_subclass__(**kwargs) diff --git a/openhands-tools/openhands/tools/ask_oracle/definition.py b/openhands-tools/openhands/tools/ask_oracle/definition.py index c6a472e641..18b2883cc1 100644 --- a/openhands-tools/openhands/tools/ask_oracle/definition.py +++ b/openhands-tools/openhands/tools/ask_oracle/definition.py @@ -1,7 +1,7 @@ """Action, observation, and tool definitions for the ask_oracle tool.""" from collections.abc import Sequence -from typing import TYPE_CHECKING, Final, Self +from typing import TYPE_CHECKING, ClassVar, Final, Self from pydantic import Field from rich.text import Text @@ -82,6 +82,10 @@ def visualize(self) -> Text: class AskOracleTool(ToolDefinition[AskOracleAction, AskOracleObservation]): """Tool for consulting the Oracle (a saved LLM profile named "oracle").""" + catalog_description: ClassVar[str] = ( + "Ask a stronger model for a second opinion on hard problems." + ) + @classmethod def create( cls, diff --git a/openhands-tools/openhands/tools/browser_use/definition.py b/openhands-tools/openhands/tools/browser_use/definition.py index 405e362d67..c2db630993 100644 --- a/openhands-tools/openhands/tools/browser_use/definition.py +++ b/openhands-tools/openhands/tools/browser_use/definition.py @@ -783,6 +783,10 @@ class BrowserToolSet(ToolDefinition[BrowserAction, BrowserObservation]): when created and automatically installs it if missing. """ + catalog_description: ClassVar[str] = ( + "Browse the web: navigate pages, click, type and read content." + ) + # Shared executor: reuse a single Chromium/CDP instance across parent # and subagents to avoid CDP port conflicts in sandbox containers. _shared_executor: ClassVar["BrowserToolExecutor | None"] = None diff --git a/openhands-tools/openhands/tools/file_editor/definition.py b/openhands-tools/openhands/tools/file_editor/definition.py index 82ec8e6a08..58abf6fc67 100644 --- a/openhands-tools/openhands/tools/file_editor/definition.py +++ b/openhands-tools/openhands/tools/file_editor/definition.py @@ -2,7 +2,7 @@ from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING, Literal +from typing import TYPE_CHECKING, ClassVar, Literal from pydantic import Field, PrivateAttr @@ -193,6 +193,8 @@ def _has_meaningful_diff(self) -> bool: class FileEditorTool(ToolDefinition[FileEditorAction, FileEditorObservation]): """A ToolDefinition subclass that automatically initializes a FileEditorExecutor.""" + catalog_description: ClassVar[str] = "View, create and edit files." + def declared_resources(self, action: Action) -> DeclaredResources: """Declare file resources accessed by this action. diff --git a/openhands-tools/openhands/tools/glob/definition.py b/openhands-tools/openhands/tools/glob/definition.py index 9bfa0627b4..a607cf23c5 100644 --- a/openhands-tools/openhands/tools/glob/definition.py +++ b/openhands-tools/openhands/tools/glob/definition.py @@ -2,7 +2,7 @@ import os from collections.abc import Sequence -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from pydantic import Field @@ -65,6 +65,8 @@ class GlobObservation(Observation): class GlobTool(ToolDefinition[GlobAction, GlobObservation]): """A ToolDefinition subclass that automatically initializes a GlobExecutor.""" + catalog_description: ClassVar[str] = "Find files by name pattern, such as **/*.ts." + def declared_resources(self, action: Action) -> DeclaredResources: """Declare resource usage based on the active backend. diff --git a/openhands-tools/openhands/tools/grep/definition.py b/openhands-tools/openhands/tools/grep/definition.py index a910966dd9..942009cd76 100644 --- a/openhands-tools/openhands/tools/grep/definition.py +++ b/openhands-tools/openhands/tools/grep/definition.py @@ -2,7 +2,7 @@ import os from collections.abc import Sequence -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from pydantic import Field @@ -67,6 +67,10 @@ class GrepObservation(Observation): class GrepTool(ToolDefinition[GrepAction, GrepObservation]): """A ToolDefinition subclass that automatically initializes a GrepExecutor.""" + catalog_description: ClassVar[str] = ( + "Search file contents with regular expressions." + ) + def declared_resources(self, action: Action) -> DeclaredResources: """Declare resource usage for parallel execution. diff --git a/openhands-tools/openhands/tools/task/definition.py b/openhands-tools/openhands/tools/task/definition.py index 28eaab1c60..ece273ea82 100644 --- a/openhands-tools/openhands/tools/task/definition.py +++ b/openhands-tools/openhands/tools/task/definition.py @@ -215,6 +215,10 @@ class TaskToolSet(ToolDefinition[TaskAction, TaskObservation]): ) """ + catalog_description: ClassVar[str] = ( + "Delegate a self-contained sub-task to a separate agent." + ) + user_selectable: ClassVar[bool] = True @classmethod diff --git a/openhands-tools/openhands/tools/task_tracker/definition.py b/openhands-tools/openhands/tools/task_tracker/definition.py index 966d136a5a..0dd0e42677 100644 --- a/openhands-tools/openhands/tools/task_tracker/definition.py +++ b/openhands-tools/openhands/tools/task_tracker/definition.py @@ -1,7 +1,7 @@ import json from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING, Literal +from typing import TYPE_CHECKING, ClassVar, Literal from pydantic import BaseModel, Field, ValidationError @@ -403,6 +403,10 @@ def _save_tasks(self) -> None: class TaskTrackerTool(ToolDefinition[TaskTrackerAction, TaskTrackerObservation]): """A ToolDefinition subclass that automatically initializes a TaskTrackerExecutor.""" # noqa: E501 + catalog_description: ClassVar[str] = ( + "Keep a running task list to organise multi-step work." + ) + @classmethod def create(cls, conv_state: "ConversationState") -> Sequence["TaskTrackerTool"]: """Initialize TaskTrackerTool with a TaskTrackerExecutor. diff --git a/openhands-tools/openhands/tools/terminal/definition.py b/openhands-tools/openhands/tools/terminal/definition.py index 4d8d7f4623..c0508864e8 100644 --- a/openhands-tools/openhands/tools/terminal/definition.py +++ b/openhands-tools/openhands/tools/terminal/definition.py @@ -3,7 +3,7 @@ import os import platform from collections.abc import Mapping, Sequence -from typing import TYPE_CHECKING, Literal +from typing import TYPE_CHECKING, ClassVar, Literal from pydantic import Field @@ -268,6 +268,10 @@ def visualize(self) -> Text: class TerminalTool(ToolDefinition[TerminalAction, TerminalObservation]): """A ToolDefinition subclass that automatically initializes a TerminalExecutor with auto-detection.""" # noqa: E501 + catalog_description: ClassVar[str] = ( + "Run shell commands in a persistent terminal session." + ) + def declared_resources(self, action: Action) -> DeclaredResources: # noqa: ARG002 # When using the tmux backend, TmuxPanePool handles concurrency # internally via pane-level isolation — opt out of framework diff --git a/openhands-tools/openhands/tools/workflow/definition.py b/openhands-tools/openhands/tools/workflow/definition.py index 89ba7b661d..6614ed7810 100644 --- a/openhands-tools/openhands/tools/workflow/definition.py +++ b/openhands-tools/openhands/tools/workflow/definition.py @@ -173,6 +173,8 @@ def create( class WorkflowToolSet(ToolDefinition[WorkflowAction, WorkflowObservation]): """Tool set that creates the dynamic workflow tool.""" + catalog_description: ClassVar[str] = "Run the workflows defined for this project." + @classmethod def create( cls, diff --git a/tests/agent_server/test_tool_router.py b/tests/agent_server/test_tool_router.py index db0be5951e..0b9cd13ac6 100644 --- a/tests/agent_server/test_tool_router.py +++ b/tests/agent_server/test_tool_router.py @@ -51,8 +51,12 @@ def test_catalog_offers_the_stock_tools_a_profile_may_pick(): "workflow_tool_set", "ask_oracle", "task_tool_set", - "SwitchLLMTool", + "switch_llm", } <= selectable + assert "SwitchLLMTool" not in entries, ( + "built-ins are offered under their snake_case tool name" + ) + assert entries["terminal"]["description"], "catalog carries a per-tool blurb" assert ( not { "task", diff --git a/tests/sdk/tool/test_registry.py b/tests/sdk/tool/test_registry.py index f15f1b116e..06e10cf6bf 100644 --- a/tests/sdk/tool/test_registry.py +++ b/tests/sdk/tool/test_registry.py @@ -96,6 +96,10 @@ def is_usable(cls) -> bool: return False +class _DescribedHelloTool(_SimpleHelloTool): + catalog_description = "Say hello, briefly." + + class _InternalHelloTool(_SimpleHelloTool): user_selectable = False @@ -180,11 +184,45 @@ def test_catalog_reports_selectability_and_usability(): "name": "catalog_plain", "user_selectable": True, "usable": True, + "description": "", } assert catalog["catalog_internal"]["user_selectable"] is False assert catalog["catalog_unusable"]["usable"] is False +def test_catalog_offers_a_builtin_under_its_snake_case_name(monkeypatch): + """A built-in is keyed by class name internally but offered like any tool.""" + from openhands.sdk.tool import builtins, registry + + monkeypatch.setattr(registry, "_CATALOG_NAMES", None) + catalog = _catalog() + + assert "switch_llm" in catalog + assert "SwitchLLMTool" not in catalog + assert builtins.SwitchLLMTool.name == "switch_llm" + + +def test_builtin_resolves_under_its_snake_case_name(): + from openhands.sdk.tool import builtins + + resolved = resolve_tool(Tool(name="switch_llm"), _create_mock_conv_state()) + + assert [t.name for t in resolved] == ["switch_llm"] + assert isinstance(resolved[0], builtins.SwitchLLMTool) + + +def test_catalog_carries_the_class_blurb(monkeypatch): + register_tool("catalog_described", _DescribedHelloTool) + + assert _catalog()["catalog_described"]["description"] == "Say hello, briefly." + + +def test_catalog_description_defaults_to_empty(): + register_tool("catalog_undescribed", _SimpleHelloTool) + + assert _catalog()["catalog_undescribed"]["description"] == "" + + def test_catalog_reports_a_selectable_builtin_as_unusable(monkeypatch): """A built-in is listed by class name, so its usability comes from the class.""" from openhands.sdk.tool import builtins, registry @@ -194,10 +232,11 @@ def test_catalog_reports_a_selectable_builtin_as_unusable(monkeypatch): ) monkeypatch.setattr(registry, "_CATALOG_NAMES", None) - assert _catalog()["UnusableBuiltin"] == { - "name": "UnusableBuiltin", + assert _catalog()[_UnavailableHelloTool.name] == { + "name": _UnavailableHelloTool.name, "user_selectable": True, "usable": False, + "description": "", } From c8a50d05d09e5d330752294f7a967dfddb88de1a Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:08:10 +0100 Subject: [PATCH 06/12] fix(tool): pin the browser-degradation invariant, look built-ins up by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Schema v3 pins `browser_tool_set` into a migrated list even on hosts with no chromium, and nothing downstream re-checks usability — that is only safe because `BrowserToolSet.create` degrades to no tools. Test it, so the migration cannot turn into a crash on such a host. Also resolve built-ins through a name-keyed dict rather than scanning, and fix a typo in the duplicate-registration warning. Co-Authored-By: Claude Opus 5 (1M context) --- .../openhands/sdk/tool/builtins/__init__.py | 7 ++++ openhands-sdk/openhands/sdk/tool/registry.py | 14 ++++--- .../tools/browser_use/test_browser_toolset.py | 37 +++++++++++++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/openhands-sdk/openhands/sdk/tool/builtins/__init__.py b/openhands-sdk/openhands/sdk/tool/builtins/__init__.py index 6e88318483..640a12000b 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/__init__.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/__init__.py @@ -53,9 +53,16 @@ VisionInspectTool.__name__: VisionInspectTool, } +# The same classes keyed by the snake_case tool name a profile stores, so +# resolving a stored pick is a lookup rather than a scan. +BUILT_IN_TOOL_CLASSES_BY_TOOL_NAME = { + tool.name: tool for tool in BUILT_IN_TOOL_CLASSES.values() +} + __all__ = [ "BUILT_IN_TOOLS", "BUILT_IN_TOOL_CLASSES", + "BUILT_IN_TOOL_CLASSES_BY_TOOL_NAME", "FinishTool", "FinishAction", "FinishObservation", diff --git a/openhands-sdk/openhands/sdk/tool/registry.py b/openhands-sdk/openhands/sdk/tool/registry.py index f9216f051b..f2408e2b94 100644 --- a/openhands-sdk/openhands/sdk/tool/registry.py +++ b/openhands-sdk/openhands/sdk/tool/registry.py @@ -147,7 +147,7 @@ def register_tool( with _LOCK: # TODO: throw exception when registering duplicate name tools if name in _REG: - logger.warning(f"Duplicate tool name registerd {name}") + logger.warning(f"Duplicate tool name registered: {name}") _REG[name] = resolver _USABILITY_REG[name] = usability_checker _TOOL_CLASSES[name] = tool_class @@ -161,12 +161,14 @@ def resolve_tool( resolver = _REG.get(tool_spec.name) if resolver is None: - from openhands.sdk.tool.builtins import BUILT_IN_TOOL_CLASSES - - tool_class = BUILT_IN_TOOL_CLASSES.get(tool_spec.name) or next( - (c for c in BUILT_IN_TOOL_CLASSES.values() if c.name == tool_spec.name), - None, + from openhands.sdk.tool.builtins import ( + BUILT_IN_TOOL_CLASSES, + BUILT_IN_TOOL_CLASSES_BY_TOOL_NAME, ) + + tool_class = BUILT_IN_TOOL_CLASSES.get( + tool_spec.name + ) or BUILT_IN_TOOL_CLASSES_BY_TOOL_NAME.get(tool_spec.name) if tool_class is None: raise KeyError(f"ToolDefinition '{tool_spec.name}' is not registered") resolver = _resolver_from_subclass(tool_spec.name, tool_class) diff --git a/tests/tools/browser_use/test_browser_toolset.py b/tests/tools/browser_use/test_browser_toolset.py index c244fe26fe..df71ebce5a 100644 --- a/tests/tools/browser_use/test_browser_toolset.py +++ b/tests/tools/browser_use/test_browser_toolset.py @@ -472,3 +472,40 @@ def test_resolve_tool_survives_browser_executor_failure(): resolved = resolve_tool(Tool(name=BrowserToolSet.name), conv_state) assert list(resolved) == [] + + +def test_migrated_profile_with_pinned_browser_resolves_on_browserless_runtime(): + """A v3 profile carrying `browser_tool_set` is harmless without a browser. + + Schema v3 pins the standard set — browser included — into `tools` when it + folds away `enable_sub_agents`, and `resolve_tool_specs` then uses an + explicit list verbatim. Nothing downstream re-checks usability, so the + pinned entry is only safe because `BrowserToolSet.create` degrades to no + tools. Pin that, or the migration turns into a crash on such a host. + """ + from openhands.sdk.profiles.agent_profile import fold_sub_agents_into_tools + from openhands.sdk.tool.defaults import resolve_tool_specs + from openhands.sdk.tool.registry import resolve_tool + + migrated = fold_sub_agents_into_tools(None, enable_sub_agents=True) + assert migrated is not None + specs = resolve_tool_specs(migrated) + assert [spec.name for spec in specs] == [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + "task_tool_set", + ] + + with tempfile.TemporaryDirectory() as temp_dir: + conv_state = _create_test_conv_state(temp_dir) + with patch.object( + BrowserToolSet, + "_get_or_create_shared_executor", + side_effect=RuntimeError("no chromium on this host"), + ): + browser_spec = next(s for s in specs if s.name == "browser_tool_set") + resolved = resolve_tool(browser_spec, conv_state) + + assert list(resolved) == [] From 933b46b59ffddbe1c34e2d50872819b26b22413d Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:34:13 +0100 Subject: [PATCH 07/12] feat(profiles): keep switch_llm in the default set so nothing loses it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `enable_switch_llm_tool` defaulted to true, so dropping it cost every existing profile the tool. Making `switch_llm` part of the set an unset `tools` resolves to removes that loss entirely, and the migration only has to pin a list where the stored config differed from that default: | stored v2 | migrated v3 `tools` | |----------------------------|--------------------------------| | defaults | unset (still tracks the default) | | switch off | standard set minus switch_llm | | sub-agents on | standard set + both | | explicit list | list + switch_llm | | explicit list, switch off | list, unchanged | The agent rejects duplicate tool names, so `switch_llm` now has exactly one delivery channel: `tools`. `create_agent` appends it for the legacy flag instead of routing it through `include_default_tools`, which keeps the two launch paths agreeing on the same toolset. `default_tool_specs` stays in lockstep with the openhands-tools preset, which does not carry this SDK built-in; only `resolve_tool_specs` — the settings/profile defaulting point — adds it. Co-Authored-By: Claude Opus 5 (1M context) --- .../openhands/sdk/profiles/agent_profile.py | 41 ++++++++++------- openhands-sdk/openhands/sdk/profiles/seed.py | 7 +-- openhands-sdk/openhands/sdk/settings/model.py | 18 ++++---- openhands-sdk/openhands/sdk/tool/defaults.py | 25 ++++++++--- .../test_agent_profiles_router.py | 2 + tests/sdk/profiles/test_agent_profile.py | 45 ++++++++++++++++--- tests/sdk/profiles/test_resolver.py | 14 +++++- .../profiles/test_store_protocol_hoists.py | 8 +++- tests/sdk/tool/test_defaults.py | 13 ++++++ tests/sdk/tool/test_switch_llm.py | 4 +- .../tools/browser_use/test_browser_toolset.py | 7 ++- 11 files changed, 139 insertions(+), 45 deletions(-) diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index f2c0babf0c..2bff60f03c 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -35,6 +35,7 @@ BROWSER_TOOL_NAME, DEFAULT_EXEC_TOOL_NAMES, SUB_AGENT_TOOL_NAME, + SWITCH_LLM_TOOL_NAME, ) @@ -357,24 +358,35 @@ def _migrate_v1_to_v2(payload: dict[str, Any]) -> dict[str, Any]: return migrated -def fold_sub_agents_into_tools( +def fold_tool_switches_into_tools( tools: Sequence[dict[str, Any] | Tool] | None, *, enable_sub_agents: bool, + enable_switch_llm_tool: bool, ) -> list[Tool] | None: - """Express a legacy ``enable_sub_agents`` switch as a ``tools`` selection.""" - if not enable_sub_agents: - return None if tools is None else [_as_tool(tool) for tool in tools] - # "The standard set plus delegation" is not expressible without the switch, - # so an unset list has to be pinned. Browser is part of that set because it - # resolves to nothing where the runtime cannot run it. + """Express the legacy tool switches as a ``tools`` selection. + + Behaviour-preserving: the switches are folded into the list only where the + result would otherwise differ from the standard set, so a profile that ran + on the defaults keeps an unset ``tools`` and stays free to follow future + changes to that set. + """ + if tools is None and not enable_sub_agents and enable_switch_llm_tool: + return None + # Anything else has to be pinned: "the standard set plus/minus one tool" is + # not expressible. Browser is part of that set because it resolves to + # nothing where the runtime cannot run it. entries = ( [_as_tool(tool) for tool in tools] if tools is not None else [Tool(name=name) for name in (*DEFAULT_EXEC_TOOL_NAMES, BROWSER_TOOL_NAME)] ) - if all(entry.name != SUB_AGENT_TOOL_NAME for entry in entries): - entries.append(Tool(name=SUB_AGENT_TOOL_NAME)) + for enabled, name in ( + (enable_sub_agents, SUB_AGENT_TOOL_NAME), + (enable_switch_llm_tool, SWITCH_LLM_TOOL_NAME), + ): + if enabled and all(entry.name != name for entry in entries): + entries.append(Tool(name=name)) return entries @@ -386,14 +398,13 @@ def _migrate_v2_to_v3(payload: dict[str, Any]) -> dict[str, Any]: """Fold the retired tool switches into ``tools``.""" migrated = dict(payload) sub_agents = migrated.pop("enable_sub_agents", False) is True - # Dropped rather than folded: it defaulted on, so honouring it would pin a - # list on nearly every profile. - migrated.pop("enable_switch_llm_tool", None) - if sub_agents and migrated.get("agent_kind", "openhands") == "openhands": + switch_llm = migrated.pop("enable_switch_llm_tool", True) is not False + if migrated.get("agent_kind", "openhands") == "openhands": stored = migrated.get("tools") - migrated["tools"] = fold_sub_agents_into_tools( + migrated["tools"] = fold_tool_switches_into_tools( stored if isinstance(stored, list) else None, - enable_sub_agents=True, + enable_sub_agents=sub_agents, + enable_switch_llm_tool=switch_llm, ) migrated["schema_version"] = 3 return migrated diff --git a/openhands-sdk/openhands/sdk/profiles/seed.py b/openhands-sdk/openhands/sdk/profiles/seed.py index 1fb1f11c49..cab40222be 100644 --- a/openhands-sdk/openhands/sdk/profiles/seed.py +++ b/openhands-sdk/openhands/sdk/profiles/seed.py @@ -14,7 +14,7 @@ ACPAgentProfile, OpenHandsAgentProfile, build_profile_verification, - fold_sub_agents_into_tools, + fold_tool_switches_into_tools, ) @@ -65,10 +65,11 @@ def build_seed_profile( llm_profile_ref=active_llm_profile or SEED_PROFILE_NAME, agent=agent_settings.agent, # Verbatim, except that a legacy ``enable_sub_agents`` switch has to be - # said as a tool selection now (see fold_sub_agents_into_tools). - tools=fold_sub_agents_into_tools( + # said as a tool selection now (see fold_tool_switches_into_tools). + tools=fold_tool_switches_into_tools( agent_settings.tools, enable_sub_agents=agent_settings.enable_sub_agents, + enable_switch_llm_tool=agent_settings.enable_switch_llm_tool, ), # Deny-list defaults to [] — the seeded default profile launches with all # discovered skills, matching the "all skills by default" model. No names diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index ce3a5f5ce7..84f453332e 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1397,23 +1397,25 @@ def create_agent(self) -> Agent: """ from openhands.sdk.agent import Agent from openhands.sdk.llm.auth.openai import create_subscription_llm_from_config - from openhands.sdk.tool.builtins import BUILT_IN_TOOLS, SwitchLLMTool + from openhands.sdk.tool.builtins import BUILT_IN_TOOLS from openhands.sdk.tool.defaults import ( SUB_AGENT_TOOL_NAME, + SWITCH_LLM_TOOL_NAME, resolve_tool_specs, ) - # Legacy switches of this settings model: the tools they add are - # otherwise selected in ``tools``. + # Legacy switches of this settings model. Both add a tool that ``tools`` + # can also select, and the agent rejects a duplicate name, so each is + # added only when the selection does not already carry it. tools = resolve_tool_specs(self.tools) - if self.enable_sub_agents and all( - tool.name != SUB_AGENT_TOOL_NAME for tool in tools + for flag, name in ( + (self.enable_sub_agents, SUB_AGENT_TOOL_NAME), + (self.enable_switch_llm_tool, SWITCH_LLM_TOOL_NAME), ): - tools = [*tools, Tool(name=SUB_AGENT_TOOL_NAME)] + if flag and all(tool.name != name for tool in tools): + tools = [*tools, Tool(name=name)] include_default_tools = [tool.__name__ for tool in BUILT_IN_TOOLS] - if self.enable_switch_llm_tool: - include_default_tools.append(SwitchLLMTool.__name__) llm = create_subscription_llm_from_config(self.llm) condenser = self.build_condenser(llm) diff --git a/openhands-sdk/openhands/sdk/tool/defaults.py b/openhands-sdk/openhands/sdk/tool/defaults.py index a49d373575..d1ab67ccfc 100644 --- a/openhands-sdk/openhands/sdk/tool/defaults.py +++ b/openhands-sdk/openhands/sdk/tool/defaults.py @@ -35,6 +35,9 @@ SUB_AGENT_TOOL_NAME = "task_tool_set" """Name of the sub-agent delegation tool set, selected like any other tool.""" +SWITCH_LLM_TOOL_NAME = "switch_llm" +"""Name of the built-in LLM-switching tool, selected like any other tool.""" + def resolve_tool_specs( tools: Sequence[Tool] | None, @@ -43,15 +46,25 @@ def resolve_tool_specs( ) -> list[Tool]: """Resolve an agent's ``tools`` setting into the specs it is built with. - ``None`` is the standard exec set, plus browser when ``enable_browser``; a - list (``[]`` included) is used as given. + ``None`` is the standard exec set, plus browser when ``enable_browser``, + plus LLM switching; a list (``[]`` included) is used as given. """ if tools is not None: return list(tools) - resolved = [Tool(name=name) for name in DEFAULT_EXEC_TOOL_NAMES] + # ``switch_llm`` is an SDK built-in rather than part of the openhands-tools + # preset, so it joins here and not in :func:`default_tool_specs`, which + # stays in lockstep with ``get_default_tools``. + return [ + *_preset_specs(enable_browser=enable_browser), + Tool(name=SWITCH_LLM_TOOL_NAME), + ] + + +def _preset_specs(*, enable_browser: bool) -> list[Tool]: + specs = [Tool(name=name) for name in DEFAULT_EXEC_TOOL_NAMES] if enable_browser: - resolved.append(Tool(name=BROWSER_TOOL_NAME)) - return resolved + specs.append(Tool(name=BROWSER_TOOL_NAME)) + return specs def default_tool_specs( @@ -69,7 +82,7 @@ def default_tool_specs( Browser is off by default (see :data:`BROWSER_TOOL_NAME` — the serving layer enables it where it can actually run). """ - specs = resolve_tool_specs(None, enable_browser=enable_browser) + specs = _preset_specs(enable_browser=enable_browser) if enable_sub_agents: specs.append(Tool(name=SUB_AGENT_TOOL_NAME)) return specs diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index f9dc0fc749..cb73b9d8e0 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -990,6 +990,7 @@ def test_materialize_reports_the_tools_a_launch_would_build( "file_editor", "task_tracker", "browser_tool_set", + "switch_llm", ] @@ -1042,6 +1043,7 @@ def test_materialize_draft_takes_precedence_over_the_stored_profile( "terminal", "file_editor", "task_tracker", + "switch_llm", ] diff --git a/tests/sdk/profiles/test_agent_profile.py b/tests/sdk/profiles/test_agent_profile.py index 26eebd1b90..055b47a760 100644 --- a/tests/sdk/profiles/test_agent_profile.py +++ b/tests/sdk/profiles/test_agent_profile.py @@ -333,7 +333,15 @@ def test_v1_profile_migrates_legacy_embedded_skills(skills: list[object]) -> Non {"name": "bare", "revision": 0}, ], ) -def test_v1_explicit_empty_tools_remain_empty(payload: dict[str, object]) -> None: +def test_v1_explicit_empty_tools_keep_only_switch_llm( + payload: dict[str, object], +) -> None: + """An explicitly bare agent stays bare of exec tools. + + It does not come out of the migration as ``[]`` though: ``switch_llm`` was + attached by the default-on switch regardless of ``tools``, so preserving + behaviour means saying so in the list. + """ profile = validate_agent_profile( { "schema_version": 1, @@ -343,7 +351,7 @@ def test_v1_explicit_empty_tools_remain_empty(payload: dict[str, object]) -> Non } ) assert isinstance(profile, OpenHandsAgentProfile) - assert profile.tools == [] + assert [tool.name for tool in profile.tools or []] == ["switch_llm"] def test_v2_sub_agents_switch_pins_the_standard_set_plus_delegation() -> None: @@ -364,6 +372,7 @@ def test_v2_sub_agents_switch_pins_the_standard_set_plus_delegation() -> None: "task_tracker", "browser_tool_set", "task_tool_set", + "switch_llm", ] @@ -379,18 +388,22 @@ def test_v2_sub_agents_switch_appends_to_an_explicit_list() -> None: } ) assert isinstance(profile, OpenHandsAgentProfile) - assert [tool.name for tool in profile.tools or []] == ["glob", "task_tool_set"] + assert [tool.name for tool in profile.tools or []] == [ + "glob", + "task_tool_set", + "switch_llm", + ] -@pytest.mark.parametrize("switch_llm", [True, False]) -def test_v2_switch_llm_flag_is_dropped_without_pinning_tools(switch_llm: bool) -> None: +def test_v2_default_switch_llm_needs_no_pinned_list() -> None: + """The default set now carries switch_llm, so nothing has to be frozen.""" profile = validate_agent_profile( { "schema_version": 2, "name": "default", "llm_profile_ref": "default", "revision": 0, - "enable_switch_llm_tool": switch_llm, + "enable_switch_llm_tool": True, } ) assert isinstance(profile, OpenHandsAgentProfile) @@ -398,6 +411,26 @@ def test_v2_switch_llm_flag_is_dropped_without_pinning_tools(switch_llm: bool) - assert not hasattr(profile, "enable_switch_llm_tool") +def test_v2_switch_llm_turned_off_pins_a_list_without_it() -> None: + """Off is not the default, so it has to be said explicitly.""" + profile = validate_agent_profile( + { + "schema_version": 2, + "name": "default", + "llm_profile_ref": "default", + "revision": 0, + "enable_switch_llm_tool": False, + } + ) + assert isinstance(profile, OpenHandsAgentProfile) + assert [tool.name for tool in profile.tools or []] == [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + ] + + def test_v2_sub_agents_switch_leaves_acp_profiles_alone() -> None: profile = validate_agent_profile( { diff --git a/tests/sdk/profiles/test_resolver.py b/tests/sdk/profiles/test_resolver.py index 2668487d83..1cfe548ddb 100644 --- a/tests/sdk/profiles/test_resolver.py +++ b/tests/sdk/profiles/test_resolver.py @@ -101,14 +101,24 @@ def test_openhands_resolves_to_settings_with_injected_llm( "terminal", "file_editor", "task_tracker", + "switch_llm", ] @pytest.mark.parametrize( ("browser_available", "expected"), [ - (False, ["terminal", "file_editor", "task_tracker"]), - (True, ["terminal", "file_editor", "task_tracker", "browser_tool_set"]), + (False, ["terminal", "file_editor", "task_tracker", "switch_llm"]), + ( + True, + [ + "terminal", + "file_editor", + "task_tracker", + "browser_tool_set", + "switch_llm", + ], + ), ], ) def test_openhands_resolves_default_exec_tools( diff --git a/tests/sdk/profiles/test_store_protocol_hoists.py b/tests/sdk/profiles/test_store_protocol_hoists.py index c9edae22bf..9135acdd05 100644 --- a/tests/sdk/profiles/test_store_protocol_hoists.py +++ b/tests/sdk/profiles/test_store_protocol_hoists.py @@ -292,7 +292,13 @@ def test_build_seed_profile_copies_explicit_tools(): profile = build_seed_profile(settings, active_llm_profile="my-llm") assert isinstance(profile, OpenHandsAgentProfile) assert profile.tools is not None - assert [t.name for t in profile.tools] == ["terminal", "browser_use"] + # `switch_llm` joins because the settings' default-on switch was giving it + # to this agent already; the explicit toolset itself is copied verbatim. + assert [t.name for t in profile.tools] == [ + "terminal", + "browser_use", + "switch_llm", + ] def test_build_seed_profile_acp_branch(): diff --git a/tests/sdk/tool/test_defaults.py b/tests/sdk/tool/test_defaults.py index b2834fd56b..096701daee 100644 --- a/tests/sdk/tool/test_defaults.py +++ b/tests/sdk/tool/test_defaults.py @@ -7,6 +7,7 @@ BROWSER_TOOL_NAME, DEFAULT_EXEC_TOOL_NAMES, SUB_AGENT_TOOL_NAME, + SWITCH_LLM_TOOL_NAME, default_tool_specs, resolve_tool_specs, ) @@ -43,6 +44,18 @@ def test_resolve_unset_tools_is_the_default_set() -> None: assert [t.name for t in resolve_tool_specs(None, enable_browser=True)] == [ *DEFAULT_EXEC_TOOL_NAMES, BROWSER_TOOL_NAME, + SWITCH_LLM_TOOL_NAME, + ] + + +def test_preset_default_specs_exclude_the_sdk_builtin() -> None: + """``default_tool_specs`` stays in lockstep with the openhands-tools preset. + + ``switch_llm`` is an SDK built-in, so it belongs to the settings/profile + default (``resolve_tool_specs``) and not to the preset constructor. + """ + assert SWITCH_LLM_TOOL_NAME not in [ + t.name for t in default_tool_specs(enable_browser=True) ] diff --git a/tests/sdk/tool/test_switch_llm.py b/tests/sdk/tool/test_switch_llm.py index 097e37c825..ff66d0ac87 100644 --- a/tests/sdk/tool/test_switch_llm.py +++ b/tests/sdk/tool/test_switch_llm.py @@ -61,7 +61,7 @@ def test_agent_settings_includes_switch_llm_tool_when_profiles_exist(profile_sto llm=_make_llm("default-model", "default"), tools=[] ).create_agent() - assert "SwitchLLMTool" in agent.include_default_tools + assert any(tool.name == "switch_llm" for tool in agent.tools) conversation = LocalConversation(agent=agent, workspace=Path.cwd()) conversation._ensure_agent_ready() @@ -87,7 +87,7 @@ def test_agent_settings_includes_switch_llm_tool_without_profiles(empty_profile_ llm=_make_llm("default-model", "default"), tools=[] ).create_agent() - assert "SwitchLLMTool" in agent.include_default_tools + assert any(tool.name == "switch_llm" for tool in agent.tools) conversation = LocalConversation(agent=agent, workspace=Path.cwd()) conversation._ensure_agent_ready() diff --git a/tests/tools/browser_use/test_browser_toolset.py b/tests/tools/browser_use/test_browser_toolset.py index df71ebce5a..7e39eca0eb 100644 --- a/tests/tools/browser_use/test_browser_toolset.py +++ b/tests/tools/browser_use/test_browser_toolset.py @@ -483,11 +483,13 @@ def test_migrated_profile_with_pinned_browser_resolves_on_browserless_runtime(): pinned entry is only safe because `BrowserToolSet.create` degrades to no tools. Pin that, or the migration turns into a crash on such a host. """ - from openhands.sdk.profiles.agent_profile import fold_sub_agents_into_tools + from openhands.sdk.profiles.agent_profile import fold_tool_switches_into_tools from openhands.sdk.tool.defaults import resolve_tool_specs from openhands.sdk.tool.registry import resolve_tool - migrated = fold_sub_agents_into_tools(None, enable_sub_agents=True) + migrated = fold_tool_switches_into_tools( + None, enable_sub_agents=True, enable_switch_llm_tool=True + ) assert migrated is not None specs = resolve_tool_specs(migrated) assert [spec.name for spec in specs] == [ @@ -496,6 +498,7 @@ def test_migrated_profile_with_pinned_browser_resolves_on_browserless_runtime(): "task_tracker", "browser_tool_set", "task_tool_set", + "switch_llm", ] with tempfile.TemporaryDirectory() as temp_dir: From fd3c3eb1e815704d22fa82a3e7e69cba595faafd Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:43:51 +0100 Subject: [PATCH 08/12] fix(agent): make include_default_tools idempotent against the tools list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A built-in can be named in both channels, and both mean "give the agent this tool" — so the second mention is a no-op, not a duplicate-name error. This is load-bearing for OpenHands Cloud. Enterprise attaches SwitchLLMTool through `include_default_tools` after building the agent, while the settings flag now puts `switch_llm` in `tools`; without this, every cloud conversation with two or more saved LLM profiles raises `Duplicate tool names found: {'switch_llm'}`. A genuine duplicate inside `tools` is still rejected. Co-Authored-By: Claude Opus 5 (1M context) --- openhands-sdk/openhands/sdk/agent/base.py | 11 ++++++++- tests/sdk/tool/test_switch_llm.py | 28 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/agent/base.py b/openhands-sdk/openhands/sdk/agent/base.py index 6f95eb6520..c5f9c00b6d 100644 --- a/openhands-sdk/openhands/sdk/agent/base.py +++ b/openhands-sdk/openhands/sdk/agent/base.py @@ -599,6 +599,10 @@ def _initialize( VisionInspectTool.__name__, ) + # A built-in can also be named in ``tools`` — both channels mean "give + # the agent this tool", so the second mention is a no-op rather than a + # duplicate-name error. + selected_names = {tool.name for tool in tools} for tool_name in default_tool_names: tool_class = BUILT_IN_TOOL_CLASSES.get(tool_name) if tool_class is None: @@ -606,7 +610,12 @@ def _initialize( f"Unknown built-in tool class: '{tool_name}'. " f"Expected one of: {list(BUILT_IN_TOOL_CLASSES.keys())}" ) - tool_instances = tool_class.create(state) + tool_instances = [ + tool + for tool in tool_class.create(state) + if tool.name not in selected_names + ] + selected_names.update(tool.name for tool in tool_instances) tools.extend(tool_instances) # Check tool types diff --git a/tests/sdk/tool/test_switch_llm.py b/tests/sdk/tool/test_switch_llm.py index ff66d0ac87..7261c8166a 100644 --- a/tests/sdk/tool/test_switch_llm.py +++ b/tests/sdk/tool/test_switch_llm.py @@ -156,3 +156,31 @@ def _raise_permission_error(profile_name: str) -> None: assert "Cannot read fast" in observation.text assert conversation.agent.llm.model == "default-model" assert conversation.state.agent.llm.model == "default-model" + + +def test_include_default_tools_is_idempotent_against_the_tools_list(profile_store): + """Naming a built-in in both channels must not fail the conversation. + + Cloud attaches `SwitchLLMTool` through `include_default_tools` after + building the agent, while the settings flag now puts `switch_llm` in + `tools`. Both say "give the agent this tool", so the second one is a + no-op rather than a duplicate-name error. + """ + agent = OpenHandsAgentSettings( + llm=_make_llm("default-model", "default"), tools=[] + ).create_agent() + assert any(tool.name == "switch_llm" for tool in agent.tools) + + agent = agent.model_copy( + update={ + "include_default_tools": [ + *agent.include_default_tools, + SwitchLLMTool.__name__, + ] + } + ) + + conversation = LocalConversation(agent=agent, workspace=Path.cwd()) + conversation._ensure_agent_ready() + + assert "switch_llm" in agent.tools_map From fcdac9e9c16b8de38b044611c49e0b2f8eec840d Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Sat, 19 Sep 2026 14:02:13 +0100 Subject: [PATCH 09/12] fix(tests): follow switch_llm into the default set; read built-in selectability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six expectations still described the old default set — `create_agent` and the profile resolver both hand back `switch_llm` now, and it arrives through `tools` rather than `include_default_tools`. `list_tool_catalog` also reads `user_selectable` off the built-in class instead of hardcoding True, so the flag has one source on both branches. Co-Authored-By: Claude Opus 5 (1M context) --- openhands-sdk/openhands/sdk/tool/registry.py | 2 +- .../test_agent_profile_conv_start.py | 1 + tests/agent_server/test_conversation_router.py | 4 +++- tests/sdk/test_settings.py | 16 +++++++++++++--- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/openhands-sdk/openhands/sdk/tool/registry.py b/openhands-sdk/openhands/sdk/tool/registry.py index f2408e2b94..2cf1ac077c 100644 --- a/openhands-sdk/openhands/sdk/tool/registry.py +++ b/openhands-sdk/openhands/sdk/tool/registry.py @@ -258,7 +258,7 @@ def list_tool_catalog() -> list[ToolCatalogEntry]: entries.extend( ToolCatalogEntry( name=tool_class.name, - user_selectable=True, + user_selectable=tool_class.user_selectable, usable=_check_tool_usable( tool_class.name, _usability_from_subclass(tool_class) ), diff --git a/tests/agent_server/test_agent_profile_conv_start.py b/tests/agent_server/test_agent_profile_conv_start.py index 24609616d1..e251ca588b 100644 --- a/tests/agent_server/test_agent_profile_conv_start.py +++ b/tests/agent_server/test_agent_profile_conv_start.py @@ -453,6 +453,7 @@ def test_launched_agent_uses_resolved_tools_unchanged(self, tmp_path): "file_editor", "task_tracker", "browser_tool_set", + "switch_llm", ] def test_openhands_default_profile_triggers_discovery(self): diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index 61fbfc69f1..51add50b7e 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -698,12 +698,14 @@ def test_start_conversation_agent_settings_uses_sdk_default_tools( assert response.status_code == 201 request = mock_conversation_service.start_conversation.call_args.args[0] - assert "SwitchLLMTool" in request.agent.include_default_tools + # `switch_llm` is delivered through `tools` now, not include_default_tools. + assert any(tool.name == "switch_llm" for tool in request.agent.tools) assert {tool.name for tool in request.agent.tools} == { "terminal", "file_editor", "task_tracker", "browser_tool_set", + "switch_llm", } finally: client.app.dependency_overrides.clear() diff --git a/tests/sdk/test_settings.py b/tests/sdk/test_settings.py index 2acaf2520a..c7cc5207c2 100644 --- a/tests/sdk/test_settings.py +++ b/tests/sdk/test_settings.py @@ -962,7 +962,9 @@ def test_llm_create_agent_uses_settings_llm_and_tools() -> None: agent = settings.create_agent() assert isinstance(agent, Agent) assert agent.llm is llm - assert agent.tools == tools + # `switch_llm` joins from the default-on settings switch; the explicit + # selection itself is used verbatim. + assert agent.tools == [*tools, Tool(name="switch_llm")] def test_llm_create_agent_defaults_tool_concurrency_limit_to_one() -> None: @@ -977,7 +979,12 @@ def test_create_agent_defaults_tools_when_none() -> None: settings = OpenHandsAgentSettings(llm=LLM(model="test-model")) assert settings.tools is None agent = settings.create_agent() - assert [t.name for t in agent.tools] == ["terminal", "file_editor", "task_tracker"] + assert [t.name for t in agent.tools] == [ + "terminal", + "file_editor", + "task_tracker", + "switch_llm", + ] def test_create_agent_default_tools_honor_enable_sub_agents() -> None: @@ -985,10 +992,12 @@ def test_create_agent_default_tools_honor_enable_sub_agents() -> None: llm=LLM(model="test-model"), enable_sub_agents=True ) agent = settings.create_agent() + # `switch_llm` comes from the default set, so the sub-agent set lands after it. assert [t.name for t in agent.tools] == [ "terminal", "file_editor", "task_tracker", + "switch_llm", "task_tool_set", ] @@ -998,7 +1007,8 @@ def test_create_agent_empty_tools_stays_bare() -> None: compatibility — [] predates the None default and keeps its old meaning).""" settings = OpenHandsAgentSettings(llm=LLM(model="test-model"), tools=[]) agent = settings.create_agent() - assert agent.tools == [] + # Bare of exec tools; `switch_llm` still comes from the default-on switch. + assert [t.name for t in agent.tools] == ["switch_llm"] def test_tool_concurrency_limit_defaults_to_one_when_omitted_from_payload() -> None: From ec301c8545d0ad20b836682f1d04c9fac26c72fd Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Sat, 19 Sep 2026 14:06:21 +0100 Subject: [PATCH 10/12] fix(settings): keep enable_sub_agents off an explicit tools list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The switch only ever fed the default set — an explicit `tools` (`[]` included) was used exactly as given. Folding both switches through one loop quietly extended its reach, contradicting the field's own description and making the behaviour change that #5157 deliberately left out of this PR. `enable_switch_llm_tool` keeps its full reach, because it attached its tool to every agent regardless of `tools`. Both appends still skip a tool the selection already names, so picking `switch_llm` from the catalog cannot trip the duplicate-name guard. Co-Authored-By: Claude Opus 5 (1M context) --- openhands-sdk/openhands/sdk/settings/model.py | 11 ++++--- tests/sdk/test_settings.py | 30 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 84f453332e..70b1486ae3 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1404,12 +1404,15 @@ def create_agent(self) -> Agent: resolve_tool_specs, ) - # Legacy switches of this settings model. Both add a tool that ``tools`` - # can also select, and the agent rejects a duplicate name, so each is - # added only when the selection does not already carry it. + # Legacy switches of this settings model, each kept to the reach it + # always had: ``enable_sub_agents`` only ever fed the default set, so an + # explicit ``tools`` (``[]`` included) stays exactly as given, while + # ``enable_switch_llm_tool`` attached its tool to every agent. Both + # tools can also be selected in ``tools``, and the agent rejects a + # duplicate name, so neither is added twice. tools = resolve_tool_specs(self.tools) for flag, name in ( - (self.enable_sub_agents, SUB_AGENT_TOOL_NAME), + (self.enable_sub_agents and self.tools is None, SUB_AGENT_TOOL_NAME), (self.enable_switch_llm_tool, SWITCH_LLM_TOOL_NAME), ): if flag and all(tool.name != name for tool in tools): diff --git a/tests/sdk/test_settings.py b/tests/sdk/test_settings.py index c7cc5207c2..494a2495d7 100644 --- a/tests/sdk/test_settings.py +++ b/tests/sdk/test_settings.py @@ -1002,6 +1002,36 @@ def test_create_agent_default_tools_honor_enable_sub_agents() -> None: ] +def test_enable_sub_agents_does_not_reach_an_explicit_tools_list() -> None: + """The switch only ever fed the default set. + + An explicit ``tools`` is used exactly as given, so turning sub-agents on + must not append to it — honouring the switch against an explicit list is + its own behaviour change (#5157), deliberately not made here. + """ + explicit = OpenHandsAgentSettings( + llm=LLM(model="test-model"), + tools=[Tool(name="terminal")], + enable_sub_agents=True, + ).create_agent() + assert "task_tool_set" not in [t.name for t in explicit.tools] + + bare = OpenHandsAgentSettings( + llm=LLM(model="test-model"), tools=[], enable_sub_agents=True + ).create_agent() + assert "task_tool_set" not in [t.name for t in bare.tools] + + +def test_explicitly_selected_switch_llm_is_not_added_twice() -> None: + """The catalog offers `switch_llm` as a pick; the default-on switch must + not then deliver it a second time and trip the duplicate-name guard.""" + agent = OpenHandsAgentSettings( + llm=LLM(model="test-model"), tools=[Tool(name="switch_llm")] + ).create_agent() + + assert [t.name for t in agent.tools] == ["switch_llm"] + + def test_create_agent_empty_tools_stays_bare() -> None: """tools=[] is an explicit choice: no default injection (persisted-payload compatibility — [] predates the None default and keeps its old meaning).""" From b331faa5569bf4e30e0b53dd2ffecea36dd60a42 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:31:46 +0000 Subject: [PATCH 11/12] fix(tool): honour switch_llm off, and treat its class name as the same tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes opened by putting `switch_llm` in the default set, both found by @rajshah4 testing this against the Canvas PR: - `enable_switch_llm_tool=false` with `tools` unset still got the tool: the default injected it and nothing took it back out. `resolve_tool_specs` now takes `enable_switch_llm`, so the off switch is honoured at the one defaulting point. - `resolve_tool` accepts a built-in under its class name as well as its tool name, so a stored `SwitchLLMTool` and an appended `switch_llm` are the same tool twice — a duplicate-name crash at agent init. Both append sites now compare canonical names. Co-Authored-By: Claude Opus 5 (1M context) --- .../openhands/sdk/profiles/agent_profile.py | 4 ++- openhands-sdk/openhands/sdk/settings/model.py | 8 ++++-- openhands-sdk/openhands/sdk/tool/defaults.py | 27 ++++++++++++++----- tests/sdk/profiles/test_agent_profile.py | 15 +++++++++++ tests/sdk/test_settings.py | 23 ++++++++++++++++ 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index 2bff60f03c..0686f69ea5 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -36,6 +36,7 @@ DEFAULT_EXEC_TOOL_NAMES, SUB_AGENT_TOOL_NAME, SWITCH_LLM_TOOL_NAME, + canonical_tool_name, ) @@ -385,7 +386,8 @@ def fold_tool_switches_into_tools( (enable_sub_agents, SUB_AGENT_TOOL_NAME), (enable_switch_llm_tool, SWITCH_LLM_TOOL_NAME), ): - if enabled and all(entry.name != name for entry in entries): + selected = {canonical_tool_name(entry.name) for entry in entries} + if enabled and name not in selected: entries.append(Tool(name=name)) return entries diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 70b1486ae3..cd076439fa 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1401,6 +1401,7 @@ def create_agent(self) -> Agent: from openhands.sdk.tool.defaults import ( SUB_AGENT_TOOL_NAME, SWITCH_LLM_TOOL_NAME, + canonical_tool_name, resolve_tool_specs, ) @@ -1410,12 +1411,15 @@ def create_agent(self) -> Agent: # ``enable_switch_llm_tool`` attached its tool to every agent. Both # tools can also be selected in ``tools``, and the agent rejects a # duplicate name, so neither is added twice. - tools = resolve_tool_specs(self.tools) + tools = resolve_tool_specs( + self.tools, enable_switch_llm=self.enable_switch_llm_tool + ) for flag, name in ( (self.enable_sub_agents and self.tools is None, SUB_AGENT_TOOL_NAME), (self.enable_switch_llm_tool, SWITCH_LLM_TOOL_NAME), ): - if flag and all(tool.name != name for tool in tools): + selected = {canonical_tool_name(tool.name) for tool in tools} + if flag and name not in selected: tools = [*tools, Tool(name=name)] include_default_tools = [tool.__name__ for tool in BUILT_IN_TOOLS] diff --git a/openhands-sdk/openhands/sdk/tool/defaults.py b/openhands-sdk/openhands/sdk/tool/defaults.py index d1ab67ccfc..84d518b60e 100644 --- a/openhands-sdk/openhands/sdk/tool/defaults.py +++ b/openhands-sdk/openhands/sdk/tool/defaults.py @@ -43,21 +43,36 @@ def resolve_tool_specs( tools: Sequence[Tool] | None, *, enable_browser: bool = False, + enable_switch_llm: bool = True, ) -> list[Tool]: """Resolve an agent's ``tools`` setting into the specs it is built with. - ``None`` is the standard exec set, plus browser when ``enable_browser``, - plus LLM switching; a list (``[]`` included) is used as given. + ``None`` is the standard exec set, plus browser when ``enable_browser`` and + LLM switching when ``enable_switch_llm``; a list (``[]`` included) is used + as given. """ if tools is not None: return list(tools) # ``switch_llm`` is an SDK built-in rather than part of the openhands-tools # preset, so it joins here and not in :func:`default_tool_specs`, which # stays in lockstep with ``get_default_tools``. - return [ - *_preset_specs(enable_browser=enable_browser), - Tool(name=SWITCH_LLM_TOOL_NAME), - ] + resolved = _preset_specs(enable_browser=enable_browser) + if enable_switch_llm: + resolved.append(Tool(name=SWITCH_LLM_TOOL_NAME)) + return resolved + + +def canonical_tool_name(name: str) -> str: + """The runtime name a spec resolves to, collapsing built-in class aliases. + + ``resolve_tool`` accepts a built-in under its class name as well as its + tool name, so ``SwitchLLMTool`` and ``switch_llm`` select the same tool and + must not both be added. + """ + from openhands.sdk.tool.builtins import BUILT_IN_TOOL_CLASSES + + tool_class = BUILT_IN_TOOL_CLASSES.get(name) + return tool_class.name if tool_class is not None else name def _preset_specs(*, enable_browser: bool) -> list[Tool]: diff --git a/tests/sdk/profiles/test_agent_profile.py b/tests/sdk/profiles/test_agent_profile.py index 055b47a760..e56d304249 100644 --- a/tests/sdk/profiles/test_agent_profile.py +++ b/tests/sdk/profiles/test_agent_profile.py @@ -411,6 +411,21 @@ def test_v2_default_switch_llm_needs_no_pinned_list() -> None: assert not hasattr(profile, "enable_switch_llm_tool") +def test_v2_fold_recognises_the_switch_llm_class_alias() -> None: + """A stored list naming the built-in by class name already has the tool.""" + profile = validate_agent_profile( + { + "schema_version": 2, + "name": "default", + "llm_profile_ref": "default", + "revision": 0, + "tools": [{"name": "SwitchLLMTool", "params": {}}], + } + ) + assert isinstance(profile, OpenHandsAgentProfile) + assert [tool.name for tool in profile.tools or []] == ["SwitchLLMTool"] + + def test_v2_switch_llm_turned_off_pins_a_list_without_it() -> None: """Off is not the default, so it has to be said explicitly.""" profile = validate_agent_profile( diff --git a/tests/sdk/test_settings.py b/tests/sdk/test_settings.py index 494a2495d7..31a7e6de37 100644 --- a/tests/sdk/test_settings.py +++ b/tests/sdk/test_settings.py @@ -1002,6 +1002,29 @@ def test_create_agent_default_tools_honor_enable_sub_agents() -> None: ] +def test_switch_llm_turned_off_is_honoured_for_the_default_set() -> None: + """The default set carries `switch_llm`, so the off switch has to remove it. + + Reported by @rajshah4: an agent_settings with the flag off and `tools` + unset was still getting the tool. + """ + agent = OpenHandsAgentSettings( + llm=LLM(model="test-model"), enable_switch_llm_tool=False + ).create_agent() + + assert "switch_llm" not in [t.name for t in agent.tools] + + +def test_switch_llm_class_name_alias_is_not_duplicated() -> None: + """`resolve_tool` takes a built-in under either name, so both select one + tool — adding the second spelling would trip the duplicate-name guard.""" + agent = OpenHandsAgentSettings( + llm=LLM(model="test-model"), tools=[Tool(name="SwitchLLMTool")] + ).create_agent() + + assert [t.name for t in agent.tools] == ["SwitchLLMTool"] + + def test_enable_sub_agents_does_not_reach_an_explicit_tools_list() -> None: """The switch only ever fed the default set. From cc0f2493d5f8cf89ceb8768f7a81f942d547d873 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:49:02 +0000 Subject: [PATCH 12/12] fix(profiles): keep enable_sub_agents off an explicit list in the migration too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ec301c8` restored the switch's original reach in `create_agent` but left the migration folding it into any stored list. A v2 profile with `enable_sub_agents: true` and `tools: [glob]` launched with `[glob]`, yet migrated to `[glob, task_tool_set]` — handing the agent delegation it never had, which is both a break of the behaviour-preserving claim and the change deferred to #5157. `test_v2_sub_agents_switch_appends_to_an_explicit_list` pinned that wrong behaviour and is replaced. Co-Authored-By: Claude Opus 5 (1M context) --- .../openhands/sdk/profiles/agent_profile.py | 5 ++++- tests/sdk/profiles/test_agent_profile.py | 14 ++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index 0686f69ea5..7f0a40da8a 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -383,7 +383,10 @@ def fold_tool_switches_into_tools( else [Tool(name=name) for name in (*DEFAULT_EXEC_TOOL_NAMES, BROWSER_TOOL_NAME)] ) for enabled, name in ( - (enable_sub_agents, SUB_AGENT_TOOL_NAME), + # Each switch keeps the reach it had: `enable_sub_agents` only ever fed + # the default set, so an explicit list is used as given, while + # `enable_switch_llm_tool` attached its tool to every agent. + (enable_sub_agents and tools is None, SUB_AGENT_TOOL_NAME), (enable_switch_llm_tool, SWITCH_LLM_TOOL_NAME), ): selected = {canonical_tool_name(entry.name) for entry in entries} diff --git a/tests/sdk/profiles/test_agent_profile.py b/tests/sdk/profiles/test_agent_profile.py index e56d304249..90a27b4ff0 100644 --- a/tests/sdk/profiles/test_agent_profile.py +++ b/tests/sdk/profiles/test_agent_profile.py @@ -376,7 +376,13 @@ def test_v2_sub_agents_switch_pins_the_standard_set_plus_delegation() -> None: ] -def test_v2_sub_agents_switch_appends_to_an_explicit_list() -> None: +def test_v2_sub_agents_switch_does_not_reach_an_explicit_list() -> None: + """The switch only ever fed the default set. + + A v2 profile with an explicit ``tools`` launched with exactly that list, so + folding delegation into it here would hand the agent a capability it never + had — the behaviour change deferred to #5157. + """ profile = validate_agent_profile( { "schema_version": 2, @@ -388,11 +394,7 @@ def test_v2_sub_agents_switch_appends_to_an_explicit_list() -> None: } ) assert isinstance(profile, OpenHandsAgentProfile) - assert [tool.name for tool in profile.tools or []] == [ - "glob", - "task_tool_set", - "switch_llm", - ] + assert [tool.name for tool in profile.tools or []] == ["glob", "switch_llm"] def test_v2_default_switch_llm_needs_no_pinned_list() -> None: