feat(agent-profiles): select every tool from one server catalog

[simonrosenberg]

HUMAN:

I filed #4958 after the canvas profile editor forced us to hardcode a tool allow-list and a copy of the SDK defaults. This is the SDK half: the server answers what is pickable and what a profile resolves to. It also retires enable_sub_agents and enable_switch_llm_tool from the profile — a profile should have exactly one place that says which tools the agent gets, and that is tools. Deliberately kept small otherwise: no per-tool provenance, no server-provided descriptions.

---

AGENT:

Why

An Agent Profile editor has to answer two questions about tools, and the SDK could answer neither (#4958):

1. Which tools may a user pick? /api/tools/ and /server_info.usable_tools return the process's tool registry — presets no product path builds an agent from, and it grows as conversations register their own tools. feat(agent-profiles): expose custom instructions and tool selection in the editor OpenHands#17235 has to carry a hand-curated allow-list.
2. What will this profile actually launch with? materialize passed tools straight through, so a profile that leaves it unset reported null while the launch resolved four tools. The browser was appended after resolution and only on the launch path, so the preview could never show it.

Underneath both sat a third problem: tools was not actually the answer to "which tools?". enable_sub_agents and enable_switch_llm_tool were two more controls over the same thing, invisible to any catalog and silently ignored once tools was an explicit list.

Summary

• user_selectable on ToolDefinition, served by GET /api/tools/catalog along with usable. Marked False on the built-ins that are always attached, ClientTool, the single-task task, planning_file_editor, the low-level workflow, and the Gemini file family. A third-party tool that declares nothing stays selectable.
• Every entry carries a one-line blurb from a catalog_description ClassVar on the tool class, so a picker can say what task_tool_set does instead of showing a bare name. It lives on the class (readable without instantiating) and is written for a user — description is the model's prompt and runs to paragraphs.
• Built-ins are offered under their snake_case name. They were listed by class name (SwitchLLMTool) while everything else used terminal/task_tool_set; they now use ToolDefinition.name, which is already snake_case, and resolve_tool accepts it so a stored pick resolves.
• The catalog is sealed once the server has loaded its tools (presets plus --import-modules). Anything registered later belongs to one conversation and vanishes on restart, so client tools and tool_module_qualnames imports are never offered.
• tools is the only tool control on a profile. enable_sub_agents and enable_switch_llm_tool are gone from OpenHandsAgentProfile; task_tool_set and switch_llm are ordinary selectable catalog entries. Both switches stay on AgentSettingsConfig, which the legacy settings launch path still honours.
• resolve_tool_specs is the single defaulting point — used by create_agent and by the profile resolver — and browser availability is an explicit resolver input instead of a post-launch injection. resolved_settings.tools therefore reports what the launch really builds, on both paths.
• 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, so clients feature-detect instead of guessing version numbers ([Agent Profile] The profile model isn't self-describing — clients hardcode version numbers to guess which fields a server accepts #4964).

⚠️ Ships in lockstep with OpenHands/OpenHands#17516

AgentProfileBase is extra="forbid", and the profile save is a whole-profile overwrite. A canvas that still sends enable_sub_agents / enable_switch_llm_tool gets the entire save rejected with 422. The migration does not cover this: it runs only while schema_version < 3, so an old client saving an already-migrated v3 profile re-adds the retired keys at v3 and is refused.

Deploy the two together. An old client against an old server, or a new client against a new server, are both fine; only old-client-against-new-server breaks, and it breaks loudly rather than silently.

Verified against OpenHands Cloud

Enterprise is pinned to openhands-sdk==1.46.0, so its whole unit suite was run against this branch by installing it editable over the pin:

• main SDK: 5746 passed, 2 failed
• this branch: 5746 passed, 2 failed — byte-identical failure sets

Both failures are the 1.46 → 1.49 version bump (agent_settings schema 5 → 6, and a sandbox-injector default), reproduced on plain main; neither is from this PR.

That run also caught a real break, now fixed in fd3c3eb: cloud attaches SwitchLLMTool through include_default_tools after create_agent, while the settings flag now puts switch_llm in tools. Agent rejects duplicate tool names, so every cloud conversation with two or more saved LLM profiles would have raised Duplicate tool names found: {'switch_llm'}. include_default_tools is now idempotent against tools — naming a built-in in both channels is a no-op, while a genuine duplicate inside tools is still rejected.

One pre-existing gap this PR does not fix: live_status_app_conversation_service.py overwrites tools unconditionally when building a cloud launch, so a profile's tool selection never reaches a cloud conversation. The feature is inert on cloud until that reads the resolved tools. Tracked separately.

Migration (profile schema v2 → v3), behaviour-preserving

switch_llm is part of the set an unset tools resolves to, so a profile that ran on the defaults keeps tools: null and stays free to follow future changes to that set. The migration pins a list only where the stored config differed from the default:

stored v2	migrated v3 tools	why
defaults	null (unchanged)	the default set already carries switch_llm
enable_switch_llm_tool: false	[terminal, file_editor, task_tracker, browser_tool_set]	opting out is not the default, so it has to be said
sub-agents on, tools unset	[…standard…, task_tool_set, switch_llm]	"standard plus delegation" is not expressible
sub-agents on, tools: [glob]	[glob, task_tool_set, switch_llm]	already pinned, so the appends are free
tools: [glob]	[glob, switch_llm]	it was getting switch_llm from the default-on switch
tools: []	[switch_llm]	bare of exec tools, but it had switch_llm
ACP profile	untouched	no tools field

browser_tool_set rides along in a pinned list even on a host without chromium. That is safe because BrowserToolSet.create degrades to no tools, which test_migrated_profile_with_pinned_browser_resolves_on_browserless_runtime now pins so the degradation cannot be removed silently.

Because Agent rejects duplicate tool names, switch_llm has exactly one delivery channel — tools. create_agent appends it for the legacy enable_switch_llm_tool flag rather than routing it through include_default_tools, so the legacy agent_settings path and the profile path agree on the same toolset for the same conceptual config.

REST API contract changes

Compared with base OpenAPI 004c674a96d7 for public /api/** paths.

--- base public OpenAPI
+++ head public OpenAPI
@@ -77,0 +78 @@
+operation GET /api/tools/catalog operationId=get_tool_catalog_api_tools_catalog_get
@@ -333,0 +335 @@
+requestBody POST /api/agent-profiles/{name}/materialize application/json required=false schema=anyOf=[MaterializeAgentProfileRequest,type="null"]
@@ -529,0 +532 @@
+response GET /api/tools/catalog 200 application/json schema=ToolCatalogResponse
@@ -752,0 +756,15 @@
+schema ACPAgentProfile property acp_args optional schema=anyOf=[type="array" items=type="string",type="null"]
+schema ACPAgentProfile property acp_command optional schema=anyOf=[type="string",type="null"]
+schema ACPAgentProfile property acp_model optional schema=anyOf=[type="string",type="null"]
+schema ACPAgentProfile property acp_prompt_timeout optional schema=type="number" default=1800.0 exclusiveMinimum=0.0
+schema ACPAgentProfile property acp_server optional schema=type="string" enum=["claude-code","codex","gemini-cli","kimi-code","pi","opencode","custom"] default="claude-code"
+schema ACPAgentProfile property acp_session_mode optional schema=anyOf=[type="string",type="null"]
+schema ACPAgentProfile property acp_startup_timeout optional schema=type="number" default=90.0 exclusiveMinimum=0.0
+schema ACPAgentProfile property agent_kind optional schema=type="string" const="acp" default="acp"
+schema ACPAgentProfile property id optional schema=type="string" format="uuid"
+schema ACPAgentProfile property mcp_server_refs optional schema=anyOf=[type="array" items=type="string",type="null"]
+schema ACPAgentProfile property name required schema=type="string" minLength=1
+schema ACPAgentProfile property revision optional schema=type="integer" default=0 minimum=0.0
+schema ACPAgentProfile property schema_version optional schema=type="integer" default=3 minimum=1.0
+schema ACPAgentProfile property secret_refs optional schema=anyOf=[type="array" items=type="string",type="null"]
+schema ACPAgentProfile type="object" additionalProperties=false
@@ -1883,0 +1902,9 @@
+schema LLMSummarizingCondenserSettings property condenser_kind optional schema=type="string" const="llm_summarizing" default="llm_summarizing"
+schema LLMSummarizingCondenserSettings property enabled optional schema=type="boolean" default=true
+schema LLMSummarizingCondenserSettings property hard_context_reset_context_scaling optional schema=type="number" default=0.8 exclusiveMinimum=0.0 exclusiveMaximum=1.0
+schema LLMSummarizingCondenserSettings property hard_context_reset_max_retries optional schema=type="integer" default=5 exclusiveMinimum=0.0
+schema LLMSummarizingCondenserSettings property keep_first optional schema=type="integer" default=2 minimum=0.0
+schema LLMSummarizingCondenserSettings property max_size optional schema=type="integer" default=240 minimum=20.0
+schema LLMSummarizingCondenserSettings property max_tokens optional schema=anyOf=[type="integer" exclusiveMinimum=0.0,type="null"]
+schema LLMSummarizingCondenserSettings property minimum_progress optional schema=type="number" default=0.1 exclusiveMinimum=0.0 exclusiveMaximum=1.0
+schema LLMSummarizingCondenserSettings type="object"
@@ -2126,0 +2154,2 @@
+schema MaterializeAgentProfileRequest property profile optional schema=anyOf=[oneOf=[OpenHandsAgentProfile,ACPAgentProfile],type="null"]
+schema MaterializeAgentProfileRequest type="object"
@@ -2170,0 +2200,3 @@
+schema NoOpCondenserSettings property condenser_kind optional schema=type="string" const="no_op" default="no_op"
+schema NoOpCondenserSettings property enabled optional schema=type="boolean" default=true
+schema NoOpCondenserSettings type="object"
@@ -2182,0 +2215,16 @@
+schema OpenHandsAgentProfile property agent optional schema=type="string" default="CodeActAgent"
+schema OpenHandsAgentProfile property agent_kind optional schema=type="string" const="openhands" default="openhands"
+schema OpenHandsAgentProfile property condenser optional schema=oneOf=[LLMSummarizingCondenserSettings,NoOpCondenserSettings]
+schema OpenHandsAgentProfile property disabled_skills optional schema=type="array" items=type="string"
+schema OpenHandsAgentProfile property id optional schema=type="string" format="uuid"
+schema OpenHandsAgentProfile property llm_profile_ref required schema=type="string" minLength=1
+schema OpenHandsAgentProfile property mcp_server_refs optional schema=anyOf=[type="array" items=type="string",type="null"]
+schema OpenHandsAgentProfile property name required schema=type="string" minLength=1
+schema OpenHandsAgentProfile property revision optional schema=type="integer" default=0 minimum=0.0
+schema OpenHandsAgentProfile property schema_version optional schema=type="integer" default=3 minimum=1.0
+schema OpenHandsAgentProfile property secret_refs optional schema=anyOf=[type="array" items=type="string",type="null"]
+schema OpenHandsAgentProfile property system_message_suffix optional schema=anyOf=[type="string",type="null"]
+schema OpenHandsAgentProfile property tool_concurrency_limit optional schema=type="integer" default=1 minimum=1.0
+schema OpenHandsAgentProfile property tools optional schema=anyOf=[type="array" items=Tool-Input,type="null"]
+schema OpenHandsAgentProfile property verification optional schema=ProfileVerificationSettings
+schema OpenHandsAgentProfile type="object" additionalProperties=false
@@ -2289,0 +2338,8 @@
+schema ProfileVerificationSettings property critic_enabled optional schema=type="boolean" default=false
+schema ProfileVerificationSettings property critic_mode optional schema=type="string" enum=["finish_and_message","all_actions"] default="finish_and_message"
+schema ProfileVerificationSettings property critic_model_name optional schema=anyOf=[type="string",type="null"]
+schema ProfileVerificationSettings property critic_server_url optional schema=anyOf=[type="string",type="null"]
+schema ProfileVerificationSettings property critic_threshold optional schema=type="number" default=0.6 minimum=0.0 maximum=1.0
+schema ProfileVerificationSettings property enable_iterative_refinement optional schema=type="boolean" default=false
+schema ProfileVerificationSettings property max_refinement_iterations optional schema=type="integer" default=3 minimum=1.0
+schema ProfileVerificationSettings type="object"
@@ -2757,0 +2814,7 @@
+schema ToolCatalogEntry property description optional schema=type="string" default=""
+schema ToolCatalogEntry property name required schema=type="string"
+schema ToolCatalogEntry property usable optional schema=type="boolean" default=true
+schema ToolCatalogEntry property user_selectable optional schema=type="boolean" default=true
+schema ToolCatalogEntry type="object"
+schema ToolCatalogResponse property tools required schema=type="array" items=ToolCatalogEntry
+schema ToolCatalogResponse type="object"

Issue Number

Closes #4958
Closes #5157

How to Test

Automated

• CI on this head is green apart from one unrelated pre-existing failure: test_model_features.py::test_reasoning_effort_support[openrouter/moonshotai/kimi-k2.5-False], which fails identically on main — the test file is unchanged here and this PR touches no LLM code (verified by running it against main's openhands/sdk/llm/). Everything else passes, including agent-server-tests, cross-tests, Persisted settings, Check OpenAPI Schema, REST API (OpenAPI) and check-docstrings.
• Locally, uv run pytest -n 8 tests/sdk tests/agent_server tests/cross → 9013 passed. The handful of parallel failures on this laptop are contention, not the branch: each run fails a different set, all in files this PR does not touch, and every one of them passes when its file is run serially (test_truncate needs a short tmp path, the live-server tests need a short tmux socket path, and the OpenAPI/restore clusters fail only under load).
• pre-commit run --from-ref HEAD~1 --to-ref HEAD (ruff, pycodestyle, pyright, import rules, tool-registration) passes.
• Worth reading: test_dry_run_tools_match_the_launched_agent asserts the preview equals what resolve_agent_profile(...).create_agent() builds, across tools ∈ {None, [], [glob]} × browser on/off; test_sealed_catalog_ignores_later_registrations covers the seal; the four test_v2_* cases in tests/sdk/profiles/test_agent_profile.py pin every row of the migration table above; tests/sdk/persisted_settings_baselines/v3/ adds the required v3 fixture.

End to end. Agent Canvas on an agent-server built from this branch (OH_AGENT_SERVER_LOCAL_PATH=<worktree> npm run dev:minimal), with the canvas e2e mock LLM, which records every completion request.

1. Catalog. GET /api/tools/catalog → selectable are terminal, file_editor, task_tracker, glob, grep, browser_tool_set, workflow_tool_set, ask_oracle, task_tool_set, switch_llm. /server_info advertises both capability flags.

2. The seal holds. Launching from the canvas home page (which sends client_tools: canvas_ui_control, launch_child_conversation) and starting one API conversation with tool_module_qualnames for apply_patch and tom_consult left /api/tools/ reporting 23 names while the catalog still reported its fixed set, none of them the newly registered ones.

3. Preview vs. what the LLM received. For each stored profile: materialize, then launch with agent_profile_id, then compare with the tools array of the mock LLM request.

   stored tools	resolved_settings.tools	LLM received
   [glob, grep]	glob, grep	glob, grep + built-ins (finish, think, invoke_skill) + MCP fetch
   []	(empty)	built-ins + MCP fetch only
   [terminal, glob]	terminal, glob	terminal, glob + built-ins

   Every registry tool the model was given appears in the preview, and nothing in the preview was missing from the launch. The extras are the built-ins the agent always attaches and MCP tools, which materialize reports separately as resolved_mcp_config_keys.

4. Draft preview. POST /api/agent-profiles/code-explorer/materialize with {"profile": {"name": "code-explorer", "llm_profile_ref": "mock-llm", "tools": [{"name":"glob"},{"name":"grep"}]}} → valid: true, resolved_settings.tools = [glob, grep], and GET /api/agent-profiles/code-explorer → 404 (nothing saved). A draft without name, or with an invalid field → 422. No body, or {}, materializes the stored profile.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

• Breaking, deliberately. Profiles lose two fields and agents whose profile had enable_switch_llm_tool on lose switch_llm until it is selected. The alternative — folding a default-true switch into tools — would pin an explicit list on nearly every stored profile, freezing them out of future default changes. See the migration section.
• Pinned browser is safe by a non-obvious invariant, now tested. A migrated list carries browser_tool_set even on a host with no chromium, and nothing downstream re-checks usability — it is harmless only because BrowserToolSet.create degrades to no tools. test_migrated_profile_with_pinned_browser_resolves_on_browserless_runtime pins that, so the migration cannot become a crash if the degradation is ever removed.
• resolve_agent_profile now returns a concrete list where it used to pass tools: None through, so any consumer reading resolved_settings.tools is None as "server default" sees a list instead. See the cross-repo sweep below.
• Draft materialize is a slightly wider oracle. An authenticated caller can now point it at any llm_profile_ref and trigger a dry-run resolve. Output stays redacted, so this is not secret exposure — noting the widened surface, not a defect.
• Scope cuts, on purpose. An earlier revision also reported per-tool provenance (default / runtime / builtin / …) and a description per tool. Both were dropped: the provenance needed two models, an eight-value literal and pulling the built-in rule out of Agent._initialize, and a picker renders fine without either. Easy follow-ups if a UI wants them.
• Supersedes enable_sub_agents is ignored when tools is an explicit list #5157/fix(settings): apply enable_sub_agents to an explicit tools list #5158. That issue asked for enable_sub_agents to be honoured alongside an explicit tools list; retiring the switch answers it instead, and removes the conflict rather than defining one.
• resolve_agent_profile output change. It now returns the concrete tool list rather than passing tools: None through. The stored tri-state still lives on the profile, which clients already have.
• Gemini file tools are marked not selectable. [Agent Profile] Clients can't ask what tools an agent will get, or which tools are pickable #4958 left this as a product decision; no product path builds an agent from them.
• workflow_tool_set stays selectable, as [Agent Profile] Clients can't ask what tools an agent will get, or which tools are pickable #4958 proposed, but it runs sub-agent tasks, so it is a delegation path in the same sense as [Agent Profile] Sub-agent delegation bypasses a profile's tool and MCP restrictions #4953.
• Canvas side is feat(agent-profiles): pick every tool from the server's catalog OpenHands#17516, which consumes this catalog, drops the two toggles from the profile editor, and marks its own canvas_ui tool non-selectable.
• Docker runtime, unchanged here. Its mediation still resolves profiles on the host, so browser availability is probed on the host rather than inside the runtime container — a single-launch-path concern tracked in [Agent Profile] The default profile and named profiles build different agents — collapse launch into one pipeline #5141.

🤖 Generated with Claude Code

---

🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)

• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant	Architectures	Base Image	Docs / Tags
java	amd64, arm64	eclipse-temurin:17-jdk	Link
python-slim	amd64, arm64	python-node-runtime	Link
python-minimal	amd64, arm64	python-node-runtime	Link
python	amd64, arm64	python-node-runtime	Link
golang	amd64, arm64	golang:1.21-bookworm	Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:ec301c8-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-ec301c8-python \
  ghcr.io/openhands/agent-server:ec301c8-python

All tags pushed for this build

ghcr.io/openhands/agent-server:ec301c8-golang-amd64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-golang-amd64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-golang-amd64
ghcr.io/openhands/agent-server:ec301c8-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:ec301c8-golang-arm64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-golang-arm64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-golang-arm64
ghcr.io/openhands/agent-server:ec301c8-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:ec301c8-java-amd64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-java-amd64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-java-amd64
ghcr.io/openhands/agent-server:ec301c8-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:ec301c8-java-arm64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-java-arm64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-java-arm64
ghcr.io/openhands/agent-server:ec301c8-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:ec301c8-python-amd64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-amd64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-amd64
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-amd64
ghcr.io/openhands/agent-server:ec301c8-python-arm64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-arm64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-arm64
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-arm64
ghcr.io/openhands/agent-server:ec301c8-python-minimal-amd64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-minimal-amd64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-minimal-amd64
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-minimal-amd64
ghcr.io/openhands/agent-server:ec301c8-python-minimal-arm64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-minimal-arm64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-minimal-arm64
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-minimal-arm64
ghcr.io/openhands/agent-server:ec301c8-python-slim-amd64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-slim-amd64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-slim-amd64
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-slim-amd64
ghcr.io/openhands/agent-server:ec301c8-python-slim-arm64
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-slim-arm64
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-slim-arm64
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-slim-arm64
ghcr.io/openhands/agent-server:ec301c8-golang
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-golang
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-golang
ghcr.io/openhands/agent-server:ec301c8-golang_tag_1.21-bookworm
ghcr.io/openhands/agent-server:ec301c8-java
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-java
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-java
ghcr.io/openhands/agent-server:ec301c8-eclipse-temurin_tag_17-jdk
ghcr.io/openhands/agent-server:ec301c8-python-minimal
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-minimal
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-minimal
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-minimal
ghcr.io/openhands/agent-server:ec301c8-python-slim
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python-slim
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python-slim
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime-slim
ghcr.io/openhands/agent-server:ec301c8-python
ghcr.io/openhands/agent-server:ec301c8545d0ad20b836682f1d04c9fac26c72fd-python
ghcr.io/openhands/agent-server:agent-profile-tool-catalog-python
ghcr.io/openhands/agent-server:ec301c8-python-node-runtime

About Multi-Architecture Support

• Each variant tag (e.g., ec301c8-python) is a multi-arch manifest supporting both amd64 and arm64
• Docker automatically pulls the correct architecture for your platform
• Individual architecture tags (e.g., ec301c8-python-amd64) are also available if needed

Jev-Fast-Audit

⚡ Jev fast audit · estimates · 0.64s · commit ec301c8
Strongest signal: Contract regression · 71% estimated likelihood.
Evidence: F008H004 · openhands-sdk/openhands/sdk/profiles/agent_profile.py:203–208.
Coverage: ⚠️ reduced context — partial coverage; 21/129 hunks, 17/52 files (context budget: 21, file budget: 35, hunk budget: 108).

All estimates and evidence

Estimate	Likelihood / value	Direct evidence
SQL injection	3.0%	No direct hunk selected
Command injection	7.0%	No direct hunk selected
Weakened authentication	11.0%	No direct hunk selected
Weakened authorization	28.0%	F001H004 · openhands-agent-server/openhands/agent_server/agent_profiles_router.py:500–529
Contract regression	71.0%	F008H004 · openhands-sdk/openhands/sdk/profiles/agent_profile.py:203–208
Data loss	21.0%	F008H004 · openhands-sdk/openhands/sdk/profiles/agent_profile.py:203–208
Sensitive data disclosure	7.0%	No direct hunk selected
Unexpected data transfer	4.0%	No direct hunk selected
Credential misuse	9.0%	No direct hunk selected
Untrusted instruction authority	5.0%	No direct hunk selected
Package source redirection	4.0%	No direct hunk selected
Unverified remote execution	3.0%	No direct hunk selected
Privileged environment access	4.0%	No direct hunk selected
Security assessment bypass	9.0%	No direct hunk selected
Prohibited workload	2.0%	No direct hunk selected
Primary concern	Contract regression; confidence 79.0%	F008H004 · openhands-sdk/openhands/sdk/profiles/agent_profile.py:203–208

[github-actions]

REST API breakage checks (OpenAPI) — ✅ PASSED

Result: ✅ PASSED

Action log

[github-actions]

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-agent-server/openhands/agent_server
agent_profiles_router.py	213	15	93%	177, 181, 185, 204, 207, 230, 343, 347, 440, 480–481, 539, 555, 558–559
api.py	309	26	92%	139, 141–146, 148, 150, 152, 202, 216, 222, 278, 283, 293–295, 325, 331, 335, 356–357, 608, 611, 617
conversation_service.py	1247	131	89%	201–202, 211, 238–239, 243–244, 249, 395–396, 399–400, 407, 423, 579–580, 641, 744, 751–752, 853, 939, 990–991, 998, 1030–1031, 1047, 1078, 1082, 1094, 1114, 1126–1129, 1135–1136, 1145, 1147, 1213, 1223, 1248, 1254–1255, 1259–1260, 1268, 1295, 1301, 1395, 1401, 1406, 1412, 1420–1421, 1430–1433, 1442, 1454, 1462, 1508, 1514–1515, 1518–1520, 1547, 1599, 1648–1649, 1653, 1743–1744, 1815, 1870–1872, 1874–1875, 1878–1879, 1916, 1990–1991, 2023, 2026, 2033–2035, 2038–2039, 2043–2045, 2048–2049, 2053–2055, 2058–2059, 2088, 2097, 2140, 2150–2152, 2212, 2215, 2242, 2252, 2257–2260, 2274, 2285, 2297–2298, 2330, 2425, 2482, 2540, 2555–2556, 2934, 2987, 2990
profile_launch.py	19	2	89%	36–37
server_details_router.py	61	2	97%	27–28
openhands-sdk/openhands/sdk/agent
base.py	372	42	89%	96–97, 248, 315, 321, 519, 609, 624, 632–633, 675, 677–678, 782–783, 793–794, 819–825, 834, 838, 841, 916, 919, 931–933, 938, 940, 943, 950, 954, 963, 977, 980, 996, 1042
openhands-sdk/openhands/sdk/profiles
agent_profile.py	126	7	94%	311, 322, 325, 445, 451, 456, 484
resolver.py	128	3	98%	151, 205, 458
openhands-sdk/openhands/sdk/settings
model.py	815	69	92%	313, 331, 540, 557, 567–570, 573, 586, 590, 596, 606, 612, 617, 733, 736–737, 743–746, 751, 755, 770, 773–775, 823, 828–829, 834, 855, 867, 916, 925, 972, 1160, 1185, 1187, 1189, 1191, 1193, 1195, 1197, 1199, 1201, 1203, 1477, 1479, 1781, 1801, 1938, 2067, 2106, 2261–2263, 2265, 2351, 2361, 2363, 2368, 2386, 2399, 2401, 2403, 2405, 2412
openhands-sdk/openhands/sdk/tool
client_tool.py	126	8	94%	235, 281, 300–302, 319, 350–351
registry.py	124	7	94%	52, 72–73, 84, 97, 131, 173
tool.py	353	28	92%	94–95, 97, 107, 161, 163, 187, 195–198, 327, 441, 488, 561–562, 584–585, 611, 636, 640, 667–668, 741, 862, 919, 931–932
openhands-sdk/openhands/sdk/tool/builtins
finish.py	28	1	96%	93
invoke_skill.py	88	2	98%	79, 146
switch_llm.py	65	11	83%	34–40, 62, 104, 121, 167
think.py	34	2	94%	41, 105
vision_inspect.py	122	22	82%	55, 83, 103, 106–107, 109–110, 113–114, 116, 127, 143, 156, 168, 175, 188, 204–205, 217, 259, 307, 311
openhands-tools/openhands/tools/ask_oracle
definition.py	40	19	53%	47–53, 61–63, 65–69, 95–96, 99, 101
openhands-tools/openhands/tools/browser_use
definition.py	200	72	64%	54–57, 72–74, 76–79, 82–84, 86–88, 90–92, 96, 99–100, 103–105, 116–117, 120–121, 124–125, 130–131, 133, 180, 230, 277, 321, 368, 412, 450, 488, 531, 573, 614, 662, 712, 758, 804–806, 820–822, 824–826, 828, 830, 834–836, 844–846, 849, 854, 857–858, 874–875
openhands-tools/openhands/tools/file_editor
definition.py	73	10	86%	103–104, 107, 129, 132, 135, 142, 144, 146, 148
openhands-tools/openhands/tools/gemini/edit
definition.py	37	12	68%	65, 105–108, 111–112, 124, 126, 128–129, 135
openhands-tools/openhands/tools/gemini/list_directory
definition.py	66	35	47%	65, 67–70, 72–74, 76–77, 80–82, 84–86, 89, 91, 94, 98, 100–101, 106, 110–115, 153, 165, 167, 171–172, 178
openhands-tools/openhands/tools/gemini/read_file
definition.py	49	23	53%	68, 70–73, 75–76, 78–80, 88–89, 119–122, 125–126, 138, 140, 142–143, 149
openhands-tools/openhands/tools/gemini/write_file
definition.py	34	12	65%	47, 78–81, 84–85, 97, 99, 101–102, 108
openhands-tools/openhands/tools/glob
definition.py	33	13	61%	77–78, 80, 82–84, 99, 101–103, 106, 109, 116
openhands-tools/openhands/tools/grep
definition.py	32	10	69%	81–83, 98, 100–102, 105, 108, 115
openhands-tools/openhands/tools/planning_file_editor
definition.py	38	21	45%	87, 91, 94–95, 98–99, 102–103, 105, 108, 113, 116, 119–120, 122, 125–127, 130, 136, 143
openhands-tools/openhands/tools/task
definition.py	63	26	59%	73, 79–81, 83–85, 87–88, 96, 101–102, 105, 107, 175, 183, 242, 244, 246–247, 251, 256–257, 259–260, 266
openhands-tools/openhands/tools/task_tracker
definition.py	134	86	36%	57, 60–62, 64–65, 68–69, 71, 87, 89–91, 93, 95–96, 99, 102–104, 106–107, 110–116, 118–120, 123, 125–128, 130, 133, 136–137, 139–140, 142–143, 145, 174, 176, 178–180, 187, 189–190, 195–196, 202, 214–215, 217–219, 223–224, 226–229, 231, 236, 242–246, 250, 254–255, 257–258, 260, 262–266
openhands-tools/openhands/tools/terminal
definition.py	127	59	54%	68, 72, 76–78, 82, 119, 122, 125–126, 128, 131–133, 135–137, 139–141, 143, 171, 179, 206, 208–210, 213, 215, 217–219, 221, 225–226, 229–231, 233–234, 237–240, 244–246, 251, 255–260, 262–263, 265, 284, 323
openhands-tools/openhands/tools/workflow
definition.py	26	4	85%	154, 156, 183, 185
TOTAL	45137	8652	81%

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 6dfd5b70064dc61126bfcad690e992fee92d92f3
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/74630076-9e6f-4f75-81be-102e5dde0a9c

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 2bfeff6de03b23aa9d0a0bb517264205c3ea31a5
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/0a789d23-b812-44fb-be8a-d00fce432d19

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

Solid, well-structured change. The single resolution path (resolve_tool_specs -> _profile_tool_specs, shared by launch and materialize) is a real simplification -- it removes the post-launch browser append and the duplicated defaulting logic in Agent._initialize, and the test_dry_run_tools_match_the_launched_agent matrix (16 cases) genuinely pins the preview-to-launch invariant. The conversation-scoped registry context var cleanly solves the catalog leakage problem without touching existing call sites, and the draft materialize body typing is correct (path name overrides the draft's name, invalid draft -> 422).

No material bugs found at the line level. Two things for a human maintainer:

Eval risk -- flagging for a human decision (not approving)

This PR ships a deliberate agent-behavior change: enable_sub_agents now adds task_tool_set to an explicit tools list, where previously the flag was silently ignored once tools was set. The author calls this out and it's the right call (a toggle that does nothing is worse than a consistent one), but it does change the toolset an agent launches with for any profile/settings that combines an explicit tools list with enable_sub_agents=True. That plausibly affects benchmark/eval performance, so per the repo's review policy I'm leaving a COMMENT rather than an approval.

Concretely: the integration-test label is not on this PR, so Run Integration Tests and Run Examples Scripts are both skipped for the head SHA, and there is no eval-monitor link in the PR description or comments. Before merging I'd want either:

• a lightweight eval run posted to https://openhands-eval-monitor.vercel.app with a human confirmation, or
• the integration-test label added so the integration/examples workflows actually exercise the changed tool-resolution path, or
• an explicit maintainer decision that the task_tool_set addition is low-risk and no eval is needed.

If a maintainer confirms the behavior change is acceptable, the rest of the PR is in good shape and I'd approve.

Minor observations (non-blocking)

• resolve_builtin_tools can auto-attach InvokeSkillTool/VisionInspectTool that are not in include_default_tools. test_dry_run_tools_match_the_launched_agent asserts reported == agent.tools + agent.include_default_tools, which only holds when neither auto-attaches (no skills, vision not triggered). It passes in CI today, but the assertion's premise is fragile -- if the default LLM fixture ever becomes non-vision with a vision profile available, it will start failing. Not a blocker; just a heads-up.
• gather_profile_launch_inputs probes is_tool_usable(BROWSER_TOOL_NAME) on the host for both launch and materialize. The PR notes Docker-runtime mediation still resolves on the host (browser probed outside the container) and defers that to #5141 -- acknowledged, not a regression here.

Risk assessment: MEDIUM -- clean implementation and good test coverage, but the enable_sub_agents toolset change touches agent behavior without eval evidence. Recommend human maintainer review + eval/integration-test confirmation before merge.

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

The unification of launch and preview through gather_profile_launch_inputs + resolve_tool_specs is a clean design - one resolution path for both, with the browser probe and skill discovery shared, and the post-launch append removed. The catalog seal (seal_tool_catalog at lifespan) correctly excludes conversation-scoped registrations, and tests assert the real (non-mock) resolution path matches between materialize and create_agent().

One material issue to reconcile: the enable_sub_agents "behaviour change" is not implemented.

The PR description states:

Behaviour change: enable_sub_agents now adds task_tool_set to an explicit tools list too. Previously the flag was silently ignored once tools was set.

And the e2e table's first row lists task_tool_set in resolved_tools and task in the LLM-received tools for tools: [glob, grep] with sub-agents on.

But resolve_tool_specs short-circuits on an explicit list:

if tools is not None:
    return list(tools)   # enable_sub_agents is ignored

task_tool_set is only appended when tools is None. The new unit test test_openhands_profile_tools_selection_is_used_as_given asserts the opposite of the stated change: an explicit [terminal] list with enable_sub_agents=True resolves to [terminal] only (no task_tool_set).

So the code, the unit test, the docstrings ("enable_sub_agents adds the sub-agent tool set in every case" in settings/model.py and agent_profile.py), and the PR description all disagree. Whichever is the intended behavior, they need to be made consistent before this is safe to ship - a profile editor reading the description/field docstring would expect toggling sub-agents to add the delegation tool regardless of an explicit tool list, and would get silently different behavior.

This PR also touches tool resolution / agent behavior (the launch path now resolves tools through resolve_tool_specs instead of passing tools: None through), which is in the eval-risk category. I don't see an eval-monitor link or maintainer eval confirmation in the description, so I'm leaving a COMMENT rather than approving - recommend a human maintainer sanity-check tool resolution against a lightweight eval (especially the tools=None default path, which now bakes in browser_available at resolve time instead of post-launch).

No other issues found. The catalog, draft-materialize, capability flags, and the profile_launch consolidation all look correct and well-tested.

[RISK ASSESSMENT]

• [Overall PR] Risk: MEDIUM. The refactor is sound and well-tested, but (1) the documented enable_sub_agents behavior change does not match the implementation, and (2) it changes the tool-resolution path for launches, which could affect benchmark behavior. Reconcile the docstring/description with the code and confirm no eval regression before merge.

[simonrosenberg]

Thanks — the enable_sub_agents mismatch was real, and it was mine: I split that behaviour change out into #5158 (issue #5157) to keep this PR additive, rewrote the description, but left two field descriptions behind still promising it.

Fixed in 875d2b8: OpenHandsAgentSettings.tools and OpenHandsAgentProfile.tools are back to describing the current behaviour (the sub-agent tool set is added only when tools is unset). The code and the unit test were already correct for this branch — test_openhands_profile_tools_selection_is_used_as_given asserts today's behaviour on purpose, and #5158 flips it along with the code.

On the earlier review's eval-risk point: that risk now belongs entirely to #5158, so this PR no longer changes any agent's toolset — it only reports what would already be built and adds GET /api/tools/catalog.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 875d2b8414ba10ff7e4b347c77c4eb37a409170d
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/8b236f7f-fd01-49b5-937d-c5f570bcce4f

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: f36523e212addd45d7245b2eeea064bf6edb7439
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/7b386eaf-1f15-4598-ad59-820d22d375c2

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

Reviewed the tool-catalog + truthful-materialize feature at head f36523e2. The design is clean and the implementation is solid; I found no material bugs.

What I verified against the code:

• resolve_tool_specs (openhands/sdk/tool/defaults.py) is the single defaulting point: None -> exec set + the subset of ENV_DEPENDENT_DEFAULT_TOOL_NAMES the caller reports via runtime_tools + task_tool_set when enable_sub_agents; a list (incl. []) is used as given. No machine probing inside the SDK.
• Launch and materialize share gather_profile_launch_inputs (profile_launch.py), so both resolve against the same skill catalog and runtime_tools. The preview therefore equals what resolve_agent_profile(...).create_agent() builds - confirmed by test_dry_run_tools_match_the_launched_agent parametrized over tools in {None, [], [glob]} x browser on/off.
• The catalog is sealed once in api_lifespan (api.py) after presets + --import-modules; list_tool_catalog filters by _CATALOG_NAMES, so per-conversation client tools and tool_module_qualnames imports registered later are correctly excluded. ClientTool.user_selectable = False and the seal test (test_sealed_catalog_ignores_later_registrations) cover this. No KeyError risk since names is filtered before indexing _TOOL_CLASSES.
• Browser is no longer injected post-launch in _initialize/add_runtime_tools; it now lives in the resolved tools list. The profile path resolves runtime_tools before create_agent() re-resolves the (now concrete) list, so there is no double-application and no drop.
• Draft materialize: body.profile overrides name with the path param; None/{} loads the stored profile (404 if missing); a draft missing name fails Pydantic (422). Matches the contract.
• server_info.capabilities advertises tool_catalog_v1 and agent_profile_draft_materialize_v1 for feature detection.
• No # type: ignore in the changed files; no version bumps in pyproject.toml.

Eval-risk flag (not approving)

This PR changes tool resolution and defaulting - what tools an agent actually launches with, including the conditions under which browser_tool_set is attached. That is agent-behavior territory that can plausibly shift benchmark/evaluation performance (e.g. browser availability, tool-set composition). Per the repo review guide I am leaving a COMMENT rather than approving, and flagging this for a human maintainer to decide after running lightweight evals. I did not find an eval-monitor link or a maintainer confirmation in the PR description/comments.

If eval evidence has already been gathered (a run on openhands-eval-monitor.vercel.app plus a maintainer confirmation), treat this flag as satisfied.

Risk Assessment

MEDIUM - The change is well-structured and well-tested at the unit level, but it alters the agent's resolved toolset on the launch path, which is eval-sensitive. The scope cuts (no per-tool provenance, enable_sub_agents split to #5158, Docker host-probe tracked in #5141) are documented and reasonable.

Recommendation: A human maintainer should confirm there is no eval regression before merge; otherwise the code itself looks good to go.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 5ac623b67b52cab04742b2012169b9ffd9b11743
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/e5b6b9e1-0c7b-4db3-b55b-85b97e32c8c6

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

⚠️ OpenHands gave up on this review for commit 5ac623b67b52 after 2h (last state: active).

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 2695be607abe9985a2c8445a41eaca6d1d92aae2
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/9aca6165-35bf-492a-8d8a-b79248c9957e

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

Re-reviewed at head 2695be6 (the prior bot review was at f36523e2; new commits since). The design is clean and I found no material bugs. The implementation matches the PR description: resolve_tool_specs is the single defaulting point, launch and materialize share gather_profile_launch_inputs, the catalog is sealed once in api_lifespan, and the v2→v3 migration folds enable_sub_agents into tools while dropping enable_switch_llm_tool without pinning.

What I verified against the code

• resolve_tool_specs (openhands/sdk/tool/defaults.py) is the single defaulting point: None → exec set (+ browser only when enable_browser); a list (incl. []) is used as given. Used by both create_agent and _build_openhands_settings, so the materialize preview equals what a launch builds — pinned by test_dry_run_tools_match_the_launched_agent (parametrized over tools ∈ {None, [], [glob]} × browser on/off).
• gather_profile_launch_inputs (profile_launch.py) is shared by _resolve_agent_from_profile and materialize_agent_profile, so preview and launch resolve against the same skill catalog + browser probe. Browser is now a resolver input rather than a post-launch model_copy injection; the old "explicit tools never amended" invariant is preserved because resolve_tool_specs only adds browser when tools is None.
• Catalog seal (api.py / registry.py): seal_tool_catalog() snapshots _REG once after presets + --import-modules; list_tool_catalog filters by _CATALOG_NAMES, so per-conversation client tools and tool_module_qualnames imports registered later are excluded. _TOOL_CLASSES/_USABILITY_REG/_REG are all populated together under _LOCK, so the tool_classes[name] indexing in list_tool_catalog cannot KeyError. _CATALOG_NAMES is None is a graceful pre-seal fallback. Covered by test_sealed_catalog_ignores_later_registrations.
• Migration (agent_profile.py): _migrate_v2_to_v3 pops enable_sub_agents/enable_switch_llm_tool before the extra="forbid" validation, so old v2 payloads load. The ACP branch correctly leaves tools untouched. The four test_v2_* cases pin every row of the migration table.
• Draft materialize (agent_profiles_router.py): body.profile is validated as the AgentProfile discriminated union, so a draft missing llm_profile_ref or with a foreign field returns 422; a bare/{} body falls back to the stored profile (404 if absent). store.list() == [] after a draft confirms nothing is persisted.

One minor note (not blocking)

list_tool_catalog hardcodes usable=True for built-in classes added in the extension loop (registry.py:250). Today only SwitchLLMTool is a selectable builtin and it has no is_usable gate, so this is accurate. It would mis-report if a future selectable builtin gained a runtime-conditional is_usable() — worth a follow-up to derive usable from the class the same way registered tools do, but not a live bug now.

⚠️ Eval-risk — leaving COMMENT, not APPROVE

Per the repo review policy, I am not approving because this PR changes agent tool availability on the profile launch path:

• Browser resolution moved from a post-launch injection into the resolver (browser_available input).
• switch_llm is dropped from the default toolset on the profile path (enable_switch_llm_tool pinned False); agents whose profile had it on lose switch_llm until explicitly selected.
• Sub-agent delegation is now a tools selection rather than the enable_sub_agents switch.

These are squarely in "tool calling/execution" territory and could plausibly affect benchmark/eval performance. The PR description provides CI-green + local pytest + a mock-LLM canvas e2e, but no eval-monitor link and no human-maintainer confirmation of benchmark results, so the eval-risk requirement is not satisfied.

Recommendation: a human maintainer should decide after running a lightweight eval (or confirm eval evidence if already available) before merging.

[RISK ASSESSMENT]

• Overall PR ⚠️ Risk: 🟡 MEDIUM — clean, well-tested implementation with a deliberate, well-documented breaking change; the medium rating is for the unverified eval/benchmark impact of changing which tools a launched agent receives.

KEY INSIGHT: tools is now the single source of truth for an agent's toolset across both launch paths, with resolve_tool_specs as the one defaulting point — a real simplification that removes the conflicting enable_sub_agents/enable_switch_llm_tool side-channels.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[simonrosenberg]

Addressed the minor note in 681d891d: list_tool_catalog now derives a built-in's usable from the class via _usability_from_subclass, the same path registered tools take, instead of hardcoding True. Added test_catalog_reports_a_selectable_builtin_as_unusable, which I confirmed fails against the previous hardcoded value.

Not re-requesting a review for a one-line change on a path you already assessed as not-a-live-bug. The eval-risk flag is the remaining gate and needs a human maintainer — noted in the PR description.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 681d891d45ea5d93d0eed3ae0cb2f25f5f00c4f8
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/80dbe824-9a3b-4abd-b9a4-1b79407449f7

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

The design is clean and the implementation is sound. I traced the full data flow (catalog -> profile tools -> resolve_tool_specs -> resolve_tool/BUILT_IN_TOOL_CLASSES fallback -> launched agent) and verified the key invariants:

• Catalog <-> profile <-> launch consistency holds. The catalog lists registered tools by their registered names and SDK built-ins by class name; resolve_tool's class-name fallback means a SwitchLLMTool pick stored in tools resolves at launch. The new test_dry_run_tools_match_the_launched_agent proves resolved_settings.tools equals what create_agent() builds across tools in {None, [], [glob]} x browser on/off -- a real end-to-end assertion, not a mock.
• Migration is correct. extra="forbid" is on the profile, but _migrate_v2_to_v3 pops enable_sub_agents/enable_switch_llm_tool before validation, so old v2 payloads load. The four test_v2_* cases pin every row of the migration table, and the v3 persisted-settings baseline + compat gate are in place.
• Double-resolution is idempotent. _build_openhands_settings resolves None -> exec set (+browser), then create_agent calls resolve_tool_specs again on an already-non-None list, which is a no-op pass-through. No behavior drift on the legacy settings path.
• Seal is well-scoped. seal_tool_catalog() freezes _REG; later conversation/client-tool registrations stay out of the catalog while remaining resolvable. SDK built-ins are always offered regardless of the seal, which is correct since they are deterministic and process-resident.
• No secret exposure in the draft materialize path: a draft can only reference an existing stored LLM profile, and resolved_settings is dumped with secrets redacted (no expose context).

All relevant tests pass locally (test_registry, test_defaults, test_agent_profile, test_resolver, test_tool_router, test_agent_profiles_router, test_agent_profile_conv_start, test_check_persisted_settings_compat).

Eval-risk flag (not approving)

This PR changes agent behavior: browser injection moved from a post-launch model_copy into the resolver, and enable_sub_agents / enable_switch_llm_tool are retired from profiles (the latter dropped without compensation, so agents lose switch_llm until re-selected). That alters the toolset an agent launches with, which plausibly affects benchmark/evaluation performance.

I did not find an eval-monitor link or a human maintainer's eval confirmation in the PR description or comments. Per the repo review policy I'm leaving a COMMENT rather than approving -- this is not a finding against the code, just a request for a human to run lightweight evals (or confirm a run on the eval monitor) before merge.

Risk assessment

• Overall PR: MEDIUM -- no correctness/security bugs found, but it is a deliberately breaking change to the agent-profile schema and the launched toolset, so it warrants eval validation before merge.

Verdict: Code is merge-ready from a correctness standpoint; deferring the approval decision to a human maintainer pending eval evidence.

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 501593eb4f73233db475e973e83b51d9eaa51f2f
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/317b1dca-5beb-44f0-a76c-35f8e0da21dd

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Taste rating: needs improvement — the catalog/seal/migration design is clean and well tested, but I found two concrete problems on the legacy agent_settings launch path, both in create_agent.

Material findings

1. create_agent now appends task_tool_set on top of an explicit tools list when enable_sub_agents=True (openhands-sdk/openhands/sdk/settings/model.py:1409). Verified on this head: OpenHandsAgentSettings(llm=..., tools=[Tool(name='terminal')], enable_sub_agents=True).create_agent().tools → ['terminal', 'task_tool_set'], and tools=[] with the switch → ['task_tool_set']. At base, an explicit list was used as given (self.tools if self.tools is not None else default_tool_specs(...)). This silently changes what the legacy settings launch path builds, and it contradicts the tools field's own description in the same file ("[] is an explicitly bare agent; a non-empty list is used exactly as given") and the docstring on test_create_agent_empty_tools_stays_bare ("no default injection … keeps its old meaning"). Your earlier PR comment said this flip belonged to #5158 and that this branch kept "added only when tools is unset" — after the merge, the flip is here but nothing documents it. Either gate the append on tools is None, or update the field description and call the legacy-path change out in the release notes.

2. Selecting switch_llm on the legacy settings path now crashes at agent init (openhands-sdk/openhands/sdk/settings/model.py:1416). Verified: OpenHandsAgentSettings(llm=..., tools=[Tool(name='switch_llm')]) (default enable_switch_llm_tool=True) → Agent._initialize raises ValueError: Duplicate tool names found: {'switch_llm'}, because resolve_tool now resolves the snake_case name (new in this PR) while include_default_tools also appends SwitchLLMTool. The profile path is safe because the resolver pins the flag False, but the catalog this PR ships actively offers switch_llm as a pick, and any client that mirrors that pick into tools on the still-default-True settings path hits the crash. Mirroring the dedup you already do for sub-agents (skip the append when switch_llm is already in tools) fixes it.

Everything else I traced checks out: the v2→v3 migration matches the description's table (and the test_v2_* cases pin it), the seal correctly excludes post-startup registrations while built-ins stay offered, resolve_tool_specs re-resolution in create_agent is an idempotent pass-through, and the draft-materialize path keeps secrets redacted. CI on 501593e is fully green.

Eval-risk gate

This PR changes the toolset agents launch with (every profile loses switch_llm until re-selected; browser injection moved into the resolver; plus the legacy-path flips above). I found no eval-monitor link or human maintainer confirmation in the PR description or comments. Per the repo review policy I am not approving — this needs a human maintainer to weigh the eval impact (the description itself flags it) and decide on the two findings above.

[RISK ASSESSMENT]

MEDIUM-HIGH. Deliberately breaking profile-schema change with a documented one-time tool loss, plus the two verified legacy-path issues above (one silent behavior change, one crash). No security exposure found; the draft-materialize oracle is authenticated and redacted. Key architectural insight: making tools the single source of truth is the right direction, but the transition leaves the legacy settings model with two switches whose interaction with explicit tools is now both undocumented (sub-agents) and self-contradictory (switch_llm double-add) — the pinning the profile resolver does is exactly what the legacy path is missing.

Verdict: COMMENT — two material findings + eval-risk gate; deferring to a human maintainer.

Improve this review? If feedback seems incorrect or irrelevant, update the repository's .agents/skills/custom-codereview-guide.md (with the /codereview trigger), then re-request review. The reviewer reads the guide from the PR head.

Resolve with AI? Install the iterate skill and run /iterate.

Was this review helpful? React with 👍 or 👎.

[rajshah4]

Thanks for the tool-selection work here, including the recent default-tool deduplication fix. We tested this together with the Canvas PR and prepared a small follow-up.

It preserves the legacy enable_switch_llm_tool=false setting when tools are unset and recognizes the SwitchLLMTool alias when folding settings into tool selections, avoiding duplicate registration. We also updated affected test expectations and assertions to inspect the initialized conversation agent.

Local full-suite runs passed 6,403 SDK tests and 2,184 agent-server tests using an isolated test environment. After a final assertion-only cleanup, all 94 conversation-router tests and scoped pre-commit checks passed again. Mock-model integration checks and a second local review also passed. Remote CI has not run on our commits yet.

Would you prefer a small follow-up PR or commits to cherry-pick?

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: ec301c8545d0ad20b836682f1d04c9fac26c72fd
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/3ff7f4de-d011-43f5-b035-41138ddbcc47

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Taste rating: needs improvement — the catalog/seal/draft-materialize design is clean and well tested, and both findings from the previous review round are properly fixed in ec301c8/fd3c3eb. But tracing the new switch-folding logic against the base behavior surfaced two regressions on this head, one of which @rajshah4's follow-up comment independently confirms.

Material findings

1. enable_switch_llm_tool=False is ignored on the legacy settings path when tools is unset (openhands-sdk/openhands/sdk/settings/model.py:1413). resolve_tool_specs(None) unconditionally includes switch_llm, and the loop below only ever appends — it never removes. So OpenHandsAgentSettings(llm=..., enable_switch_llm_tool=False) (tools unset) now launches with switch_llm, where at base the flag gated the include_default_tools append and the agent had no such tool. This silently re-enables LLM switching for every stored legacy agent_settings payload that opted out. The only flag-off test uses tools=[], which is why it slips through. @rajshah4's comment above ("preserves the legacy enable_switch_llm_tool=false setting when tools are unset") describes the same defect and has a fix prepared — worth landing before merge.

2. The v2→v3 migration (and the seed fold) is not behaviour-preserving for enable_sub_agents=True + explicit tools (openhands-sdk/openhands/sdk/profiles/agent_profile.py:385). At v2, an explicit tools list was used as given and the switch only fed the default set — the exact semantics ec301c8 restored in create_agent and pinned in test_enable_sub_agents_does_not_reach_an_explicit_tools_list. Yet fold_tool_switches_into_tools appends task_tool_set to explicit lists, so a stored profile with enable_sub_agents: true, tools: [glob] (launched as [glob, switch_llm] at v2) migrates to [glob, task_tool_set, switch_llm] — the agent gains sub-agent delegation it never had. That contradicts the PR's "behaviour-preserving" migration claim, the table's "the appends are free" rationale, and the deferral of honor-the-switch-with-explicit-tools to #5158. test_v2_sub_agents_switch_appends_to_an_explicit_list currently pins the changed behavior.

Everything else I traced checks out: the seal correctly excludes post-startup registrations while built-ins stay offered under snake_case names, the Agent._initialize dedup makes include_default_tools idempotent against tools (the cloud double-delivery crash is covered), the draft-materialize path keeps secrets redacted, browser availability as a resolver input matches launch vs. preview, and the rest of the migration table is genuinely behavior-preserving (including the tools: [] → [switch_llm] row and the pinned-browser degradation, which is now tested). CI on ec301c8 is fully green (58/58 checks).

Eval-risk gate

This PR changes the toolset agents launch with (default set gains switch_llm, browser injection moved into the resolver, profile tool switches retired). I found no eval-monitor link or human maintainer eval confirmation in the PR description or comments, so per the repository review policy this stays a COMMENT for a human maintainer to decide after lightweight evals — independent of the two findings above.

[RISK ASSESSMENT] HIGH — deliberate breaking change to the profile REST contract (old canvas clients get 422 on save; lockstep deploy with OpenHands/OpenHands#17516 required), plus a schema migration that rewrites every stored v2 profile's toolset on load. The migration and legacy-path semantics are exactly where the two findings sit, so human validation of the fold logic (and the eval gate) is needed before merge.

Verdict: COMMENT — two behavioral regressions in the switch-folding logic to fix (one already has a prepared follow-up from @rajshah4), plus the eval-risk gate requires human maintainer sign-off.

Key architectural insight: the two retired switches had different reach — enable_switch_llm_tool attached to every agent, enable_sub_agents only fed the default set — so folding them through one uniform code path (the append loop in create_agent, the fold in fold_tool_switches_into_tools) keeps getting the reach wrong in one direction or the other; each channel needs its own gate.

Improve this review? If feedback seems incorrect or irrelevant, update the repository's .agents/skills/custom-codereview-guide.md (with the /codereview trigger), then re-request review. The reviewer reads the guide from the PR head.

Resolve with AI? Install the
iterate skill
and run /iterate.

Was this review helpful? React with 👍 or 👎.

[all-hands-bot]

resolve_tool_specs(None) now always includes switch_llm, and the loop below only ever appends — so a legacy settings payload with enable_switch_llm_tool=False and tools unset still launches with switch_llm. At base this config had no switch_llm (the flag gated the include_default_tools append), so this silently re-enables LLM switching for users who explicitly opted out. The only flag-off test (test_agent_settings_omits_switch_llm_tool_when_disabled) uses tools=[], which is why this slips through. Suggest filtering it back out when self.tools is None and not self.enable_switch_llm_tool, plus a tools=None + flag-off test. (@rajshah4's follow-up comment on this PR describes the same fix.)

[all-hands-bot]

This append gives enable_sub_agents a reach it never had: at schema v2 an explicit tools list was used as given and the switch only fed the default set — the same semantics ec301c8 restored for create_agent and pinned in test_enable_sub_agents_does_not_reach_an_explicit_tools_list. So a stored v2 profile with enable_sub_agents: true and tools: [glob] launched with [glob, switch_llm], but migrates to [glob, task_tool_set, switch_llm] — the agent gains sub-agent delegation it never had, contradicting the "behaviour-preserving" migration claim (and the deferral of that behavior change to #5158). Gate this append on tools is None, mirroring create_agent; the same applies to the build_seed_profile fold. Note test_v2_sub_agents_switch_appends_to_an_explicit_list currently pins the changed behavior.

[all-hands-bot]

⚠️ OpenHands gave up on this review for commit ec301c8545d0 after 2h (last state: active).

This comment was posted by an AI agent (OpenHands).