From af934e28392080f2a4e12dc951d6c0921f010a41 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:09:21 -0400 Subject: [PATCH 1/4] feat(agent-profiles): one launch pipeline for every conversation start Collapse the several code paths that built a launch agent into prepare_agent_launch(), the single SDK function that resolves an Agent Profile's references and applies the runtime-dependent and per-launch pieces. conversation_service, the Docker runtime's mediation and the materialize preview all call it, so a profile named `default` and a named one build the same agent, and a preview can no longer disagree with a launch. Adds an inline `agent_profile` draft and a per-launch `llm_profile_ref` override, deprecates `agent_settings` (converted into an inline profile so it takes the same pipeline), and returns one structured error for dangling LLM/MCP references. Fixes #5141 Co-authored-by: openhands Co-Authored-By: Claude Opus 5 (1M context) --- .pr/launch_parity_e2e.py | 286 +++++ .pr/launch_parity_e2e_output.txt | 22 + .../openhands/agent_server/agent_launch.py | 215 ++++ .../agent_server/agent_profiles_router.py | 40 +- .../agent_server/conversation_router.py | 10 +- .../agent_server/conversation_service.py | 253 +---- .../agent_server/docker_runtime/mediation.py | 118 +- .../agent_server/docker_runtime/routers.py | 39 +- .../agent_server/server_details_router.py | 1 + .../sdk/conversation/message_request.py | 19 + .../openhands/sdk/conversation/request.py | 146 +-- .../openhands/sdk/profiles/__init__.py | 16 + .../openhands/sdk/profiles/agent_profile.py | 14 + .../openhands/sdk/profiles/resolver.py | 761 +++++++++---- openhands-sdk/openhands/sdk/settings/model.py | 2 +- .../docker_runtime/test_mediation.py | 98 +- tests/agent_server/test_acp_skill_sourcing.py | 22 +- .../test_agent_launch_additions.py | 126 ++- .../agent_server/test_agent_launch_parity.py | 344 ++++++ .../test_agent_profile_conv_start.py | 1007 +++++++---------- .../agent_server/test_conversation_router.py | 60 +- tests/sdk/profiles/test_launch.py | 496 ++++++++ 22 files changed, 2831 insertions(+), 1264 deletions(-) create mode 100644 .pr/launch_parity_e2e.py create mode 100644 .pr/launch_parity_e2e_output.txt create mode 100644 openhands-agent-server/openhands/agent_server/agent_launch.py create mode 100644 openhands-sdk/openhands/sdk/conversation/message_request.py create mode 100644 tests/agent_server/test_agent_launch_parity.py create mode 100644 tests/sdk/profiles/test_launch.py diff --git a/.pr/launch_parity_e2e.py b/.pr/launch_parity_e2e.py new file mode 100644 index 0000000000..a1ac62dd68 --- /dev/null +++ b/.pr/launch_parity_e2e.py @@ -0,0 +1,286 @@ +"""Live end-to-end check of the unified launch pipeline (#5141). + +Starts a real agent-server, stores two identically-configured Agent Profiles — +one named ``default`` — and launches a conversation through every product path: +``agent_profile_id``, an inline ``agent_profile`` draft, and the deprecated +``agent_settings``. Then compares the agents the server actually built, plus +the ``materialize`` preview of the same profile. + +Run: uv run python .pr/launch_parity_e2e.py +""" + +from __future__ import annotations + +import json +import os +import shutil +import signal +import subprocess +import sys +import tempfile +import time +from pathlib import Path +from typing import Any + +import httpx + + +PORT = 8971 +BASE = f"http://127.0.0.1:{PORT}" + +MCP_CONFIG = { + "fetch": {"url": "https://fetch.invalid/mcp"}, + "other": {"url": "https://other.invalid/mcp"}, +} +PROFILE_BODY: dict[str, Any] = { + "agent_kind": "openhands", + "llm_profile_ref": "primary", + "tools": [{"name": "terminal"}, {"name": "glob"}], + "system_message_suffix": "PROFILE_SUFFIX", + "disabled_skills": ["git"], + "enable_switch_llm_tool": False, + "tool_concurrency_limit": 3, + "mcp_server_refs": ["fetch"], + "condenser": {"kind": "NoOpCondenser", "enabled": False}, +} + + +def start_server(home: Path) -> subprocess.Popen: + env = { + **os.environ, + "OH_PERSISTENCE_DIR": str(home / "persistence"), + "OPENHANDS_SUPPRESS_BANNER": "1", + } + log = (home / "server.log").open("w") + process = subprocess.Popen( + [sys.executable, "-m", "openhands.agent_server", "--port", str(PORT)], + env=env, + stdout=log, + stderr=subprocess.STDOUT, + ) + for _ in range(120): + if process.poll() is not None: + sys.exit(f"server exited early; see {home / 'server.log'}") + try: + if httpx.get(f"{BASE}/health", timeout=2).status_code == 200: + return process + except httpx.HTTPError: + time.sleep(0.5) + sys.exit("server did not become healthy") + + +def expect(response: httpx.Response, *codes: int) -> Any: + if response.status_code not in codes: + sys.exit( + f"{response.request.method} {response.request.url} " + f"-> {response.status_code}: {response.text[:500]}" + ) + return response.json() if response.content else None + + +def agent_view(agent: dict[str, Any]) -> dict[str, Any]: + """The agent fields a profile owns, minus per-launch identity.""" + context = agent.get("agent_context") or {} + llm = dict(agent["llm"]) + # api_key is masked on a launch and redacted in a preview; usage ids and + # metrics are per-conversation. + for volatile in ("usage_id", "metrics", "service_id", "api_key"): + llm.pop(volatile, None) + return { + "llm": llm, + "tools": [tool["name"] for tool in agent["tools"]], + "mcp_config": sorted(agent.get("mcp_config") or {}), + "skills": sorted(skill["name"] for skill in context.get("skills") or []), + "system_message_suffix": context.get("system_message_suffix"), + "disabled_skills": context.get("disabled_skills"), + "load_project_skills": context.get("load_project_skills"), + "load_memory": context.get("load_memory"), + "condenser_off": agent.get("condenser") is None, + "critic": agent.get("critic"), + "tool_concurrency_limit": agent.get("tool_concurrency_limit"), + "switch_llm": "SwitchLLMTool" in (agent.get("include_default_tools") or []), + "has_datetime": bool(context.get("current_datetime")), + } + + +def launch(client: httpx.Client, workspace: Path, **source: Any) -> dict[str, Any]: + body = { + "workspace": {"kind": "LocalWorkspace", "working_dir": str(workspace)}, + **source, + } + return expect( + client.post("/api/conversations", params={"include_skills": "true"}, json=body), + 200, + 201, + ) + + +def main() -> int: + home = Path(tempfile.mkdtemp(prefix="launch-parity-")) + workspace = home / "workspace" + workspace.mkdir() + process = start_server(home) + try: + client = httpx.Client(base_url=BASE, timeout=120) + + # Global settings: the shared MCP list plus a legacy inline LLM. + expect( + client.patch( + "/api/settings", + json={ + "agent_settings_diff": { + "llm": {"model": "gpt-4o", "api_key": "sk-e2e"}, + "mcp_config": MCP_CONFIG, + } + }, + ), + 200, + ) + expect( + client.post( + "/api/profiles/primary", + json={ + "llm": {"model": "gpt-4o", "api_key": "sk-e2e"}, + "include_secrets": True, + }, + ), + 200, + 201, + ) + + # Two profiles, identical but for the name. + for name in ("default", "default-copy"): + expect(client.post(f"/api/agent-profiles/{name}", json=PROFILE_BODY), 201) + listed = expect(client.get("/api/agent-profiles"), 200) + ids = {p["name"]: p["id"] for p in listed["profiles"]} + + results: dict[str, dict[str, Any]] = {} + results["default (agent_profile_id)"] = agent_view( + launch(client, workspace, agent_profile_id=ids["default"])["agent"] + ) + results["default-copy (agent_profile_id)"] = agent_view( + launch(client, workspace, agent_profile_id=ids["default-copy"])["agent"] + ) + results["inline (agent_profile)"] = agent_view( + launch( + client, + workspace, + agent_profile={**PROFILE_BODY, "name": "inline-draft"}, + )["agent"] + ) + + # The deprecated path: the same configuration as an agent_settings dump. + legacy = { + "agent_kind": "openhands", + "llm": {"model": "gpt-4o", "api_key": "sk-e2e"}, + "tools": PROFILE_BODY["tools"], + "enable_switch_llm_tool": False, + "tool_concurrency_limit": 3, + "mcp_config": {"fetch": MCP_CONFIG["fetch"]}, + "condenser": PROFILE_BODY["condenser"], + "agent_context": { + "system_message_suffix": "PROFILE_SUFFIX", + "disabled_skills": ["git"], + "current_datetime": "2020-01-01T00:00", + }, + } + legacy_agent = launch(client, workspace, agent_settings=legacy)["agent"] + results["legacy (agent_settings)"] = agent_view(legacy_agent) + legacy_datetime = (legacy_agent["agent_context"] or {})["current_datetime"] + print( + f"legacy current_datetime sent 2020-01-01T00:00, launched with " + f"{legacy_datetime}" + ) + stale_timestamp = str(legacy_datetime).startswith("2020") + + # The preview of the same profile. + preview = expect(client.post("/api/agent-profiles/default/materialize"), 200) + if not preview["valid"]: + sys.exit(f"materialize invalid: {preview['errors']}") + previewed = preview["resolved_settings"] + results["materialize (default)"] = agent_view( + { + "llm": previewed["llm"], + "tools": previewed["tools"], + "mcp_config": previewed["mcp_config"], + "agent_context": previewed["agent_context"], + # A disabled condenser builds no condenser on the agent. + "condenser": None if not previewed["condenser"]["enabled"] else {}, + "critic": None, + "tool_concurrency_limit": previewed["tool_concurrency_limit"], + "include_default_tools": ( + ["SwitchLLMTool"] if previewed["enable_switch_llm_tool"] else [] + ), + } + ) + + # A dangling reference must fail the launch with a structured error. + expect( + client.post( + "/api/agent-profiles/broken", + json={ + **PROFILE_BODY, + "llm_profile_ref": "gone", + "mcp_server_refs": ["nope"], + }, + ), + 201, + ) + broken_id = { + p["name"]: p["id"] + for p in expect(client.get("/api/agent-profiles"), 200)["profiles"] + }["broken"] + error = client.post( + "/api/conversations", + json={ + "agent_profile_id": broken_id, + "workspace": {"kind": "LocalWorkspace", "working_dir": str(workspace)}, + }, + ) + + # The deprecated payload carries the client's own skill catalog (canvas + # assembles one today), so that field is the client's, not the pipeline's. + baseline_key = "default (agent_profile_id)" + baseline = results[baseline_key] + print(f"\nbaseline: {baseline_key}") + print(json.dumps(baseline, indent=2, sort_keys=True)) + failures = [] + for name, view in results.items(): + if name == baseline_key: + continue + ignored = {"skills"} if name == "legacy (agent_settings)" else set() + diff = { + field: (baseline[field], view[field]) + for field in baseline + if field not in ignored and baseline[field] != view[field] + } + print(f"\n{name}: {'MATCH' if not diff else 'DIFF ' + json.dumps(diff)}") + if diff: + failures.append(name) + + print(f"\ndangling refs -> HTTP {error.status_code} {error.text[:300]}") + if error.status_code != 422: + failures.append("dangling refs status") + detail = error.json().get("detail", {}) + if detail.get("dangling_llm_profile_ref") != "gone" or detail.get( + "dangling_mcp_server_refs" + ) != ["nope"]: + failures.append("dangling refs detail") + + if stale_timestamp: + failures.append("stale current_datetime") + + print("\nRESULT:", "FAIL " + ", ".join(failures) if failures else "PASS") + return 1 if failures else 0 + finally: + process.send_signal(signal.SIGINT) + try: + process.wait(timeout=30) + except subprocess.TimeoutExpired: + process.kill() + shutil.copy(home / "server.log", "/tmp/launch-parity-server.log") + shutil.rmtree(home, ignore_errors=True) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.pr/launch_parity_e2e_output.txt b/.pr/launch_parity_e2e_output.txt new file mode 100644 index 0000000000..39162164da --- /dev/null +++ b/.pr/launch_parity_e2e_output.txt @@ -0,0 +1,22 @@ +legacy current_datetime sent 2020-01-01T00:00, launched with 2026-09-17T14:08:55.977969-04:00 + +baseline: default (agent_profile_id) +{ + ], + }, + ], + ], + ] +} + +default-copy (agent_profile_id): MATCH + +inline (agent_profile): MATCH + +legacy (agent_settings): MATCH + +materialize (default): MATCH + +dangling refs -> HTTP 422 {"detail":{"code":"unresolved_profile_references","message":"LLM profile 'gone' not found; MCP server(s) not configured: nope","dangling_llm_profile_ref":"gone","dangling_mcp_server_refs":["nope"]}} + +RESULT: PASS diff --git a/openhands-agent-server/openhands/agent_server/agent_launch.py b/openhands-agent-server/openhands/agent_server/agent_launch.py new file mode 100644 index 0000000000..b233fc155d --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/agent_launch.py @@ -0,0 +1,215 @@ +"""Turn a conversation start request into the agent it launches with. + +Every launch path (local conversations, the Docker runtime, and the +materialize preview) resolves its agent here, through +:func:`~openhands.sdk.profiles.prepare_agent_launch`. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any, cast +from uuid import UUID + +from pydantic import ValidationError + +from openhands.agent_server.persistence.models import PersistedSettings +from openhands.agent_server.persistence.store import ( + get_agent_profile_store, + get_llm_profile_store, +) +from openhands.agent_server.skills_service import discover_profile_skills +from openhands.sdk.agent.base import AgentBase +from openhands.sdk.conversation.request import StartConversationRequest +from openhands.sdk.profiles import ( + ACPAgentProfile, + AgentLaunchCatalog, + AgentLaunchError, + AgentLaunchPlan, + AgentLaunchRuntime, + OpenHandsAgentProfile, + ProfileNotFound, + agent_settings_launch_source, + prepare_agent_launch, +) +from openhands.sdk.profiles.resolver import ACPSkillSourcing, ProfileOrigin +from openhands.sdk.settings.model import validate_agent_settings +from openhands.sdk.tool import BROWSER_TOOL_NAME, is_tool_usable +from openhands.sdk.utils.cipher import Cipher +from openhands.sdk.utils.deprecation import warn_deprecated + + +@dataclass(frozen=True, kw_only=True) +class LaunchSource: + """A request's agent source, loaded and ready to resolve.""" + + source: OpenHandsAgentProfile | ACPAgentProfile | AgentBase + catalog: AgentLaunchCatalog | None + profile_origin: ProfileOrigin | None + + +def _error_text(exc: Exception) -> str: + # A ValidationError's str() echoes the rejected input. + if isinstance(exc, ValidationError): + return "; ".join(err["msg"] for err in exc.errors()) + return str(exc) + + +def launch_runtime( + settings: PersistedSettings, + *, + acp_skill_sourcing: ACPSkillSourcing, + browser_available: bool | None = None, +) -> AgentLaunchRuntime: + """This server's launch runtime; ``browser_available=None`` probes this process.""" + if browser_available is None: + browser_available = is_tool_usable(BROWSER_TOOL_NAME) + context = settings.agent_settings.agent_context + return AgentLaunchRuntime( + browser_available=browser_available, + acp_skill_sourcing=acp_skill_sourcing, + stream=True, + load_memory=bool(context and context.load_memory), + ) + + +def load_stored_profile(profile_id: UUID) -> OpenHandsAgentProfile | ACPAgentProfile: + store = get_agent_profile_store() + name = store.name_for_id(profile_id) + if name is None: + raise ProfileNotFound(f"Agent profile with id '{profile_id}' not found") + try: + return store.load(name) + except FileNotFoundError as exc: + raise ProfileNotFound( + f"Agent profile '{name}' (id={profile_id}) not found" + ) from exc + except ValueError as exc: + raise AgentLaunchError(f"Failed to load agent profile '{name}': {exc}") from exc + + +def profile_catalog( + profile: OpenHandsAgentProfile | ACPAgentProfile, + *, + cipher: Cipher | None, + settings: PersistedSettings, + runtime: AgentLaunchRuntime, +) -> AgentLaunchCatalog: + """Resolve ``profile`` against this server's stores and skill catalog.""" + skills = None + if runtime.uses_skill_catalog(profile.agent_kind): + try: + skills = discover_profile_skills() + except Exception as exc: + raise AgentLaunchError( + f"Skill discovery failed for profile '{profile.name}': {exc}" + ) from exc + return AgentLaunchCatalog( + llm_store=get_llm_profile_store(), + mcp_config=settings.agent_settings.mcp_config, + skills=skills, + cipher=cipher, + ) + + +def load_launch_source( + request: StartConversationRequest, + *, + cipher: Cipher | None, + settings: PersistedSettings, + runtime: AgentLaunchRuntime, +) -> LaunchSource: + """Load the request's agent source. Blocking: call from a worker thread.""" + agent = cast(AgentBase | None, request.agent) + if request.agent_profile_id is not None: + profile = load_stored_profile(request.agent_profile_id) + origin: ProfileOrigin = "stored" + elif request.agent_profile is not None: + profile = request.agent_profile + origin = "inline" + elif agent is not None: + return LaunchSource(source=agent, catalog=None, profile_origin=None) + else: + warn_deprecated( + "StartConversationRequest.agent_settings", + deprecated_in="1.50.0", + removed_in="1.55.0", + details="Use agent_profile_id or agent_profile instead.", + ) + context = {"cipher": cipher} if request.secrets_encrypted else None + try: + agent_settings = validate_agent_settings( + request.agent_settings, context=context + ) + except (TypeError, ValueError) as exc: + raise AgentLaunchError( + f"Invalid agent_settings: {_error_text(exc)}" + ) from exc + profile, catalog = agent_settings_launch_source(agent_settings) + return LaunchSource(source=profile, catalog=catalog, profile_origin=None) + + return LaunchSource( + source=profile, + catalog=profile_catalog( + profile, cipher=cipher, settings=settings, runtime=runtime + ), + profile_origin=origin, + ) + + +def apply_launch( + request: StartConversationRequest, + source: LaunchSource, + runtime: AgentLaunchRuntime, + *, + build_agent: bool = True, +) -> tuple[StartConversationRequest, AgentLaunchPlan]: + """Resolve ``source`` and fold the result into a request carrying only ``agent``. + + The profile's secret scope is enforced on ``request.secrets`` here, so a + caller cannot widen it by sending more secrets than the profile allows. + """ + try: + plan = prepare_agent_launch( + source.source, + catalog=source.catalog, + runtime=runtime, + additions=request.agent_launch_additions, + profile_origin=source.profile_origin, + build_agent=build_agent, + ) + except AgentLaunchError: + raise + except (TypeError, ValueError) as exc: + raise AgentLaunchError(f"Agent failed to resolve: {_error_text(exc)}") from exc + secrets = request.secrets + if plan.allowed_secrets is not None: + secrets = { + name: value + for name, value in secrets.items() + if name in plan.allowed_secrets + } + updates: dict[str, Any] = { + "agent_profile_id": None, + "agent_profile": None, + "agent_settings": None, + "agent_launch_additions": None, + "secrets": secrets, + } + if plan.agent is not None: + updates["agent"] = plan.agent + return request.model_copy(update=updates), plan + + +def prepare_launch_request( + request: StartConversationRequest, + *, + cipher: Cipher | None, + settings: PersistedSettings, + runtime: AgentLaunchRuntime, +) -> tuple[StartConversationRequest, AgentLaunchPlan]: + """Load and resolve a request's agent. Blocking: call from a worker thread.""" + source = load_launch_source( + request, cipher=cipher, settings=settings, runtime=runtime + ) + return apply_launch(request, source, runtime) 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..5bee68770c 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -22,6 +22,7 @@ get_config, store_errors, ) +from openhands.agent_server.agent_launch import launch_runtime from openhands.agent_server.persistence import ( PersistedSettings, get_agent_profile_store, @@ -506,39 +507,42 @@ async def materialize_agent_profile( 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. + # Still needed here (unlike the profile load above): the launch decrypts the + # *referenced LLM profile's* own secret. cipher = get_cipher(request) config = get_config(request) 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. + # The same runtime a launch on this server would use, so the preview cannot + # disagree with it. A Docker-runtime deployment launches in a container, + # which sources its skills from the server (#4019). + runtime = launch_runtime( + settings, + acp_skill_sourcing=( + "openhands_managed" + if config.conversation_runtime == "docker" + else config.acp_skill_sourcing + ), + ) + + # Discover skills off the event loop. A discovery failure must not 500 the + # preview: pass ``available_skills=None`` and surface it 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" - ): + if runtime.uses_skill_catalog(profile.agent_kind): 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) - llm_store = get_llm_profile_store() diagnostics = resolve_agent_profile_dry_run( profile, - llm_store=llm_store, - mcp_config=mcp_config, + llm_store=get_llm_profile_store(), + mcp_config=settings.agent_settings.mcp_config, available_skills=available_skills, cipher=cipher, + runtime=runtime, ) if discovery_error is not None: diagnostics.errors.append(f"Skill discovery failed: {discovery_error}") diff --git a/openhands-agent-server/openhands/agent_server/conversation_router.py b/openhands-agent-server/openhands/agent_server/conversation_router.py index 3782873a79..94dfddb134 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_router.py +++ b/openhands-agent-server/openhands/agent_server/conversation_router.py @@ -55,10 +55,7 @@ PluginResolutionError, ) from openhands.sdk.plugin import PluginFetchError -from openhands.sdk.profiles.resolver import ( - DanglingMcpServerRef, - ProfileNotFound, -) +from openhands.sdk.profiles import AgentLaunchError, ProfileNotFound from openhands.sdk.tool.client_tool import ClientToolRegistrationError from openhands.sdk.workspace import LocalWorkspace from openhands.tools.preset.default import get_default_tools @@ -268,10 +265,9 @@ async def start_conversation( info, is_new = await conversation_service.start_conversation(request) except ProfileNotFound as e: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) from e - except DanglingMcpServerRef as e: + except AgentLaunchError as e: raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail={"message": str(e), "dangling_mcp_server_refs": e.missing}, + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=e.to_detail() ) from e except ClientToolRegistrationError as e: raise HTTPException( diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index c510ce6b51..e0184b0da1 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -15,6 +15,10 @@ import httpx from pydantic import BaseModel +from openhands.agent_server.agent_launch import ( + launch_runtime, + prepare_launch_request, +) from openhands.agent_server.config import ACPSkillSourcing, Config, WebhookSpec from openhands.agent_server.conversation_lease import ( DEFAULT_LEASE_TTL_SECONDS, @@ -37,7 +41,6 @@ from openhands.agent_server.persistence import FileSecretsStore 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,14 +75,12 @@ 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 if TYPE_CHECKING: - from openhands.sdk.mcp.config import MCPServer from openhands.sdk.subagent.schema import AgentDefinition @@ -134,22 +135,6 @@ def _append_system_message_suffix(agent: AgentBase, addition: str) -> AgentBase: return agent.model_copy(update={"agent_context": updated_context}) -def _with_load_memory(agent: AgentBase) -> AgentBase: - """Stamp the global persistent-memory preference onto an agent. - - ``load_memory`` is a user-level setting, not part of any agent, profile or - client payload, so it is applied here regardless of how the agent reached - the request. - """ - # current_datetime stays suppressed on a synthesized context: a null - # agent_context means "no prompt context", and ACPAgent._render_suffix - # relies on that to keep a block out of the prompt. - context = agent.agent_context or AgentContext(current_datetime=None) - return agent.model_copy( - update={"agent_context": context.model_copy(update={"load_memory": True})} - ) - - def _has_git_remote(repo_root: Path, remote: str = "origin") -> bool: try: run_git_command(["git", "remote", "get-url", remote], repo_root) @@ -310,159 +295,6 @@ def _same_workspace(a: LocalWorkspace, b: LocalWorkspace) -> bool: return Path(a.working_dir).resolve() == Path(b.working_dir).resolve() -def _apply_acp_skill_sourcing( - agent: "AgentBase", sourcing: ACPSkillSourcing -) -> "AgentBase": - """Strip OpenHands-managed skills from an ACP agent under ``native`` sourcing. - - A host-local ACP CLI reads the user's own skills from its home directory, so - a second, OpenHands-managed set injected into its prompt is at best noise — - and the catalog listing tells it to call ``invoke_skill``, a tool no ACP - agent has. Container runtimes set ``openhands_managed`` because that home - configuration is absent there. Project skills are excluded either way, by - ``ACPAgent`` itself (#4019). - - A caller that sends ``agent`` / ``agent_settings`` puts its own skills on the - context, so the strip happens here rather than at profile resolution. - """ - if sourcing != "native" or not isinstance(agent, ACPAgent): - return agent - context = agent.agent_context - if context is None: - return agent - if not ( - context.skills - or context.load_user_skills - or context.load_public_skills - or context.registered_marketplaces - ): - return agent - return agent.model_copy( - update={ - "agent_context": context.model_copy( - update={ - "skills": [], - "load_user_skills": False, - "load_public_skills": False, - "registered_marketplaces": [], - } - ) - } - ) - - -def _resolve_agent_from_profile( - profile_id: "UUID", - cipher: "Cipher | None", - mcp_config: "dict[str, MCPServer]", - acp_skill_sourcing: ACPSkillSourcing = "native", -) -> "tuple[AgentBase, LaunchedAgentProfile, set[str] | None]": - """Load and resolve an agent profile by id, returning the built agent + provenance. - - The third element is the profile's secret allow-list (``None`` = unrestricted) - — strictly ``secret_refs``, with nothing added back. It is returned rather - than applied here because the secrets ride the start request, not the agent. - - Runs synchronously (call via ``asyncio.to_thread`` from async context). - - Args: - mcp_config: Global MCP servers already loaded by the caller using the - server's cipher. Passed explicitly so this free function never - touches the settings-store singleton (which may not have been - initialised with the correct cipher yet). - acp_skill_sourcing: This deployment's ACP skill policy - (``Config.acp_skill_sourcing``). Decides whether an ACP profile is - resolved with the server's managed skill catalog or with none. - - Raises: - ProfileNotFound: No stored profile has ``profile_id``. - DanglingMcpServerRef: A referenced MCP server is absent from the global config. - ValueError: Profile load or settings validation failure. - """ - from openhands.agent_server.persistence.store import ( - get_agent_profile_store, - get_llm_profile_store, - ) - from openhands.sdk.profiles.resolver import ProfileNotFound, resolve_agent_profile - from openhands.sdk.settings.model import OpenHandsAgentSettings - - store = get_agent_profile_store() - profile_name = store.name_for_id(profile_id) - if profile_name is None: - raise ProfileNotFound(f"Agent profile with id '{profile_id}' not found") - - try: - profile = store.load(profile_name) - except FileNotFoundError: - raise ProfileNotFound( - f"Agent profile '{profile_name}' (id={profile_id}) not found" - ) - except ValueError as exc: - raise ValueError( - 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 - - llm_store = get_llm_profile_store() - try: - settings_config = resolve_agent_profile( - profile, - llm_store=llm_store, - mcp_config=mcp_config, - available_skills=available_skills, - cipher=cipher, - ) - except (TypeError, ValueError) as exc: - raise ValueError(f"Profile '{profile_name}' failed to resolve: {exc}") from exc - - if isinstance(settings_config, OpenHandsAgentSettings): - # Force streaming so this launch path wires on_token: a client can't set - # llm.stream on a profile's referenced LLM ahead of time. Safe at this - # layer (not the SDK resolver) because this server wires the token - # callback whenever any llm.stream is set; a headless resolver caller - # that never wires on_token is covered by LLM's graceful degradation. - settings_config = settings_config.model_copy( - update={"llm": settings_config.llm.model_copy(update={"stream": True})} - ) - - 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, - revision=profile.revision, - secret_refs=profile.secret_refs, - ) - allowed_secrets = None if profile.secret_refs is None else set(profile.secret_refs) - return agent, launched, allowed_secrets - - def _compose_conversation_info( stored: StoredConversation, state: ConversationState, @@ -1647,9 +1479,9 @@ async def _start_conversation( f"to a different workspace" ) - # Profile resolution and the load_memory stamp must happen before - # _prepare_request_workspace (which asserts request.agent is not None) - # and before model_dump so the resolved agent is captured in request_data. + # Resolution must happen before _prepare_request_workspace (which + # asserts request.agent is not None) and before model_dump so the + # resolved agent is captured in request_data. runtime_profile = os.getenv("OH_RUNTIME_LAUNCHED_PROFILE") launched_agent_profile = ( LaunchedAgentProfile.model_validate_json(runtime_profile) @@ -1678,61 +1510,17 @@ async def _start_conversation( ) settings = PersistedSettings() - # ``ACPAgentSettings.agent_context`` is nullable, hence the guard. - stored_context = settings.agent_settings.agent_context - load_memory = bool(stored_context and stored_context.load_memory) - - if request.agent_profile_id is not None: - mcp_config = settings.agent_settings.mcp_config - ( - resolved_agent, - launched_agent_profile, - allowed_secrets, - ) = await asyncio.to_thread( - _resolve_agent_from_profile, - request.agent_profile_id, - self.cipher, - mcp_config, - acp_skill_sourcing=self.acp_skill_sourcing, - ) - updates: dict[str, Any] = {"agent": resolved_agent} - # Enforced here, not client-side: a caller that sends more secrets - # than the profile allows must not widen the agent's scope. - if allowed_secrets is not None: - updates["secrets"] = { - name: value - for name, value in request.secrets.items() - if name in allowed_secrets - } - request = request.model_copy(update=updates) - - # Applied unconditionally: a serialized agent always carries - # ``load_memory`` (model_dump emits defaults), so there is no way to - # tell a deliberate ``false`` from an echoed one. Opting a single - # conversation out needs a tri-state field; tracked separately. - if load_memory and request.agent is not None: - request = request.model_copy( - update={"agent": _with_load_memory(request.agent)} - ) - - request = request.model_copy( - update={ - "agent": _apply_acp_skill_sourcing( - request.agent, self.acp_skill_sourcing - ) - } - ) - - additions = request.agent_launch_additions - suffix = ( - additions.system_message_suffix_append.strip() - if additions and additions.system_message_suffix_append - else "" + request, plan = await asyncio.to_thread( + prepare_launch_request, + request, + cipher=self.cipher, + settings=settings, + runtime=launch_runtime( + settings, acp_skill_sourcing=self.acp_skill_sourcing + ), ) - if suffix: - request = request.model_copy( - update={"agent": _append_system_message_suffix(request.agent, suffix)} - ) + if plan.launched_profile is not None: + launched_agent_profile = plan.launched_profile request = _prepare_request_workspace( request, conversation_id, self.conversation_worktree_root @@ -1819,7 +1607,12 @@ async def _start_conversation( request_data = request.model_dump( mode="json", context={"expose_secrets": True}, - exclude={"agent_profile_id", "agent_launch_additions"}, + exclude={ + "agent_profile_id", + "agent_profile", + "agent_settings", + "agent_launch_additions", + }, ) # The agent is persisted to base_state.json (not meta.json), so it must diff --git a/openhands-agent-server/openhands/agent_server/docker_runtime/mediation.py b/openhands-agent-server/openhands/agent_server/docker_runtime/mediation.py index 2a470cf4dd..5e85017ec9 100644 --- a/openhands-agent-server/openhands/agent_server/docker_runtime/mediation.py +++ b/openhands-agent-server/openhands/agent_server/docker_runtime/mediation.py @@ -4,23 +4,29 @@ import asyncio from collections.abc import Mapping +from dataclasses import dataclass from typing import Any +import httpx from pydantic import SecretStr -from openhands.agent_server.config import Config -from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - _with_load_memory, +from openhands.agent_server.agent_launch import ( + LaunchSource, + apply_launch, + launch_runtime, + load_launch_source, ) +from openhands.agent_server.config import Config from openhands.agent_server.docker_runtime.provisioning import RuntimeIdentity +from openhands.agent_server.docker_runtime.registry import ConversationContainer from openhands.agent_server.persistence import PersistedSettings, get_settings_store from openhands.sdk.agent.base import AgentBase from openhands.sdk.conversation.request import StartConversationRequest from openhands.sdk.conversation.secret_registry import SecretRegistry +from openhands.sdk.profiles import AgentLaunchRuntime from openhands.sdk.profiles.agent_profile import LaunchedAgentProfile from openhands.sdk.secret import SecretSource, SecretValue, StaticSecret -from openhands.sdk.settings.model import validate_agent_settings +from openhands.sdk.tool import BROWSER_TOOL_NAME def materialize_secrets( @@ -53,18 +59,46 @@ def _materialize_agent_context(agent: AgentBase) -> AgentBase: ) -async def prepare_start( - body: dict[str, Any], config: Config -) -> tuple[StartConversationRequest, LaunchedAgentProfile | None]: +def container_launch_runtime( + settings: PersistedSettings, *, browser_available: bool +) -> AgentLaunchRuntime: + # A container has no host home configuration to read skills from. + return launch_runtime( + settings, + acp_skill_sourcing="openhands_managed", + browser_available=browser_available, + ) + + +async def container_browser_available(container: ConversationContainer) -> bool: + async with httpx.AsyncClient(timeout=30) as client: + response = await client.get(f"{container.host}/server_info") + response.raise_for_status() + return BROWSER_TOOL_NAME in response.json().get("usable_tools", []) + + +@dataclass(frozen=True, kw_only=True) +class PreparedStart: + """A start request whose launch was checked before its container starts.""" + + request: StartConversationRequest + source: LaunchSource + settings: PersistedSettings + launched: LaunchedAgentProfile | None + + +async def prepare_start(body: dict[str, Any], config: Config) -> PreparedStart: + """Load the launch source and check that it resolves. + + Resolution runs without building the agent, so a dangling reference fails + before a container is started. + """ body = { name: value for name, value in body.items() if value is not None or name not in {"agent", "agent_settings"} } context = {"cipher": config.cipher} if body.get("secrets_encrypted") else None - if body.get("agent_settings") is not None: - settings = validate_agent_settings(body["agent_settings"], context=context) - body = {**body, "agent": settings.create_agent(), "agent_settings": None} request = StartConversationRequest.model_validate(body, context=context) try: @@ -72,41 +106,49 @@ async def prepare_start( except (OSError, PermissionError): settings = None settings = settings or PersistedSettings() - launched = None - if request.agent_profile_id is not None: - agent, launched, allowed = await asyncio.to_thread( - _resolve_agent_from_profile, - request.agent_profile_id, - config.cipher, - settings.agent_settings.mcp_config, - acp_skill_sourcing=config.acp_skill_sourcing, - ) - secrets = request.secrets - if allowed is not None: - secrets = { - name: value for name, value in secrets.items() if name in allowed - } - request = request.model_copy( - update={"agent": agent, "agent_profile_id": None, "secrets": secrets} - ) + runtime = container_launch_runtime(settings, browser_available=False) + source = await asyncio.to_thread( + load_launch_source, + request, + cipher=config.cipher, + settings=settings, + runtime=runtime, + ) + scoped, plan = await asyncio.to_thread( + apply_launch, request, source, runtime, build_agent=False + ) + secrets = await asyncio.to_thread(materialize_secrets, scoped.secrets) + return PreparedStart( + request=request.model_copy(update={"secrets": secrets}), + source=source, + settings=settings, + launched=plan.launched_profile, + ) - context_settings = settings.agent_settings.agent_context - if context_settings is not None and context_settings.load_memory: - request = request.model_copy(update={"agent": _with_load_memory(request.agent)}) - request = request.model_copy( - update={ - "agent": await asyncio.to_thread(_materialize_agent_context, request.agent), - "secrets": await asyncio.to_thread(materialize_secrets, request.secrets), - } + +async def finish_start( + prepared: PreparedStart, runtime: AgentLaunchRuntime +) -> StartConversationRequest: + """Build the agent for the container's ``runtime``.""" + request, _ = await asyncio.to_thread( + apply_launch, prepared.request, prepared.source, runtime ) - return request, launched + agent = await asyncio.to_thread(_materialize_agent_context, request.agent) + return request.model_copy(update={"agent": agent}) def serialize_start( request: StartConversationRequest, identity: RuntimeIdentity ) -> dict[str, Any]: payload = request.model_dump( - mode="json", context={"cipher": identity.cipher}, exclude={"agent_profile_id"} + mode="json", + context={"cipher": identity.cipher}, + exclude={ + "agent_profile_id", + "agent_profile", + "agent_settings", + "agent_launch_additions", + }, ) payload["secrets_encrypted"] = True return payload diff --git a/openhands-agent-server/openhands/agent_server/docker_runtime/routers.py b/openhands-agent-server/openhands/agent_server/docker_runtime/routers.py index b1e59bd635..85cab4828a 100644 --- a/openhands-agent-server/openhands/agent_server/docker_runtime/routers.py +++ b/openhands-agent-server/openhands/agent_server/docker_runtime/routers.py @@ -13,6 +13,9 @@ from starlette.responses import JSONResponse, Response, StreamingResponse from openhands.agent_server.docker_runtime.mediation import ( + container_browser_available, + container_launch_runtime, + finish_start, materialize_secrets, prepare_start, serialize_start, @@ -33,7 +36,7 @@ ) from openhands.agent_server.utils import safe_rmtree from openhands.sdk.logger import get_logger -from openhands.sdk.profiles.resolver import DanglingMcpServerRef, ProfileNotFound +from openhands.sdk.profiles import AgentLaunchError, ProfileNotFound logger = get_logger(__name__) @@ -100,13 +103,27 @@ async def start_conversation( registry = get_registry(request) try: - prepared, launched = await prepare_start(body, registry.config) + prepared = await prepare_start(body, registry.config) identity = registry.provisioning.create(conversation_id, host_workspace) - if launched is not None and identity.launched_agent_profile is None: - identity = identity.model_copy(update={"launched_agent_profile": launched}) + except ProfileNotFound as exc: + raise HTTPException(404, str(exc)) from exc + except AgentLaunchError as exc: + raise HTTPException(422, exc.to_detail()) from exc + except ValueError as exc: + raise HTTPException(422, str(exc)) from exc + + try: + if prepared.launched is not None and identity.launched_agent_profile is None: + identity = identity.model_copy( + update={"launched_agent_profile": prepared.launched} + ) registry.provisioning.save(identity) container = await registry.get_or_create(conversation_id) - payload = serialize_start(prepared, identity) + runtime = container_launch_runtime( + prepared.settings, + browser_available=await container_browser_available(container), + ) + payload = serialize_start(await finish_start(prepared, runtime), identity) async with httpx.AsyncClient(timeout=60) as client: response = await client.post( f"{container.host}/api/conversations", @@ -114,15 +131,9 @@ async def start_conversation( headers={"X-Session-API-Key": container.api_key}, json=payload, ) - except ProfileNotFound as exc: - raise HTTPException(404, str(exc)) from exc - except DanglingMcpServerRef as exc: - raise HTTPException( - 422, - {"message": str(exc), "dangling_mcp_server_refs": exc.missing}, - ) from exc - except ValueError as exc: - raise HTTPException(422, str(exc)) from exc + except AgentLaunchError as exc: + await registry.stop(conversation_id) + raise HTTPException(422, exc.to_detail()) from exc except httpx.HTTPError as exc: await registry.stop(conversation_id) raise HTTPException( 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..e1d700bcf0 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,7 @@ class ServerInfo(BaseModel): "credential_binding_v1", "credential_binding_readiness_probe_v1", "credential_binding_activation_guard_v1", + "unified_agent_launch_v1", ] ) max_foreground_terminal_timeout_seconds: float | None = Field( diff --git a/openhands-sdk/openhands/sdk/conversation/message_request.py b/openhands-sdk/openhands/sdk/conversation/message_request.py new file mode 100644 index 0000000000..9583ac0d48 --- /dev/null +++ b/openhands-sdk/openhands/sdk/conversation/message_request.py @@ -0,0 +1,19 @@ +from typing import Literal + +from pydantic import BaseModel, Field + +from openhands.sdk.llm.message import ImageContent, Message, TextContent + + +class SendMessageRequest(BaseModel): + """Payload to send a message to the agent.""" + + role: Literal["user", "system", "assistant", "tool"] = "user" + content: list[TextContent | ImageContent] = Field(default_factory=list) + run: bool = Field( + default=False, + description="Whether the agent loop should automatically run if not running", + ) + + def create_message(self) -> Message: + return Message(role=self.role, content=self.content) diff --git a/openhands-sdk/openhands/sdk/conversation/request.py b/openhands-sdk/openhands/sdk/conversation/request.py index b99722b176..8b7a163659 100644 --- a/openhands-sdk/openhands/sdk/conversation/request.py +++ b/openhands-sdk/openhands/sdk/conversation/request.py @@ -8,12 +8,11 @@ from __future__ import annotations -from typing import Annotated, Any, Literal, cast +from typing import Annotated, Any, cast from uuid import UUID from pydantic import ( BaseModel, - ConfigDict, Discriminator, Field, Tag, @@ -24,6 +23,9 @@ from openhands.sdk.agent.acp_agent import ACPAgent as ACPAgent from openhands.sdk.agent.agent import Agent as Agent from openhands.sdk.agent.base import AgentBase +from openhands.sdk.conversation.message_request import ( + SendMessageRequest as SendMessageRequest, +) from openhands.sdk.conversation.types import ( ConversationObservabilityMetadata, ConversationObservabilitySpanName, @@ -31,8 +33,14 @@ ConversationTags, ) from openhands.sdk.hooks import HookConfig -from openhands.sdk.llm.message import ImageContent, Message, TextContent from openhands.sdk.plugin import PluginSource +from openhands.sdk.profiles.agent_profile import ( + AgentProfile, + validate_agent_profile, +) +from openhands.sdk.profiles.resolver import ( + AgentLaunchAdditions as AgentLaunchAdditions, +) from openhands.sdk.secret import SecretSource from openhands.sdk.security.analyzer import SecurityAnalyzerBase from openhands.sdk.security.confirmation_policy import ( @@ -61,35 +69,6 @@ # --------------------------------------------------------------------------- -class SendMessageRequest(BaseModel): - """Payload to send a message to the agent.""" - - role: Literal["user", "system", "assistant", "tool"] = "user" - content: list[TextContent | ImageContent] = Field(default_factory=list) - run: bool = Field( - default=False, - description="Whether the agent loop should automatically run if not running", - ) - - def create_message(self) -> Message: - return Message(role=self.role, content=self.content) - - -class AgentLaunchAdditions(BaseModel): - """Add deployment context after agent resolution.""" - - model_config = ConfigDict(extra="forbid") - - system_message_suffix_append: str | None = Field( - default=None, - max_length=32768, - description=( - "Deployment-controlled text appended to the resolved agent's " - "system-message suffix." - ), - ) - - class ConversationConfig(BaseModel): """Shared conversation configuration — everything except the agent. @@ -282,12 +261,10 @@ class ConversationConfig(BaseModel): class StartConversationRequest(ConversationConfig): """Payload to create a new conversation. - Extends :class:`ConversationConfig` with the agent specification. Supports - any concrete :class:`AgentBase` implementation, including regular OpenHands - agents and ACP agents. Clients may provide either a concrete ``agent`` - payload or an ``agent_settings`` payload; when ``agent_settings`` is provided - without ``agent``, the settings are validated with the ``agent_kind`` - discriminator and converted to the appropriate agent type. + Extends :class:`ConversationConfig` with the agent source: a stored Agent + Profile (``agent_profile_id``), an inline one (``agent_profile``), a + concrete ``agent`` built in code, or the deprecated ``agent_settings``. + Profiles and ``agent_settings`` are resolved by the server, not here. Note: the agent lives here on the *request*, deliberately not on ``ConversationConfig``. The persisted record (``StoredConversation``) does @@ -297,62 +274,85 @@ class StartConversationRequest(ConversationConfig): agent_settings: dict[str, Any] | None = Field( default=None, - exclude=True, description=( - "Optional agent settings payload. If `agent` is omitted, this is " - "validated with the AgentSettingsBase `agent_kind` discriminator and " - "used to construct the concrete agent." + "Deprecated since v1.50.0 and scheduled for removal in v1.55.0. " + "Use `agent_profile_id` or `agent_profile`. An agent settings " + "payload, validated with the `agent_kind` discriminator. The server " + "launches it as an inline Agent Profile. Ignored when `agent` is set." ), + json_schema_extra={"deprecated": True}, ) agent_profile_id: UUID | None = Field( default=None, description=( - "Optional agent profile ID. When set, the agent-server resolves the " - "referenced profile server-side (stores + cipher are required) and " - "builds the agent from it. Mutually exclusive with `agent` and " - "`agent_settings`. The SDK validator enforces exclusivity only — " - "resolution happens in conversation_service, not here." + "Stored Agent Profile to launch. The server resolves it. Mutually " + "exclusive with the other agent sources." + ), + ) + agent_profile: AgentProfile | None = Field( + default=None, + description=( + "Inline Agent Profile draft to launch. Resolved exactly like a " + "stored profile and not saved. Mutually exclusive with the other " + "agent sources." ), ) agent: AgentBase = Field(default=cast(AgentBase, None)) @model_validator(mode="before") @classmethod - def _populate_agent_from_settings(cls, data: Any) -> Any: + def _normalize_agent_source(cls, data: Any) -> Any: if not isinstance(data, dict): return data - payload = dict(data) - has_profile_id = payload.get("agent_profile_id") is not None - has_agent = payload.get("agent") is not None - has_agent_settings = payload.get("agent_settings") is not None - if has_profile_id and (has_agent or has_agent_settings): + # An explicit null means "not this source": clients round-trip the whole + # family, and ``agent`` has no null-able type of its own. + payload = { + name: value + for name, value in data.items() + if value is not None + or name not in ("agent", "agent_settings", "agent_profile") + } + profile_sources = [ + name + for name in ("agent_profile_id", "agent_profile") + if payload.get(name) is not None + ] + other_sources = [ + name + for name in ("agent", "agent_settings") + if payload.get(name) is not None + ] + if len(profile_sources) + bool(other_sources) > 1: raise ValueError( - "`agent_profile_id` is mutually exclusive with" - " `agent` and `agent_settings`" + f"`{profile_sources[0]}` is mutually exclusive with " + + ", ".join( + f"`{name}`" for name in [*profile_sources[1:], *other_sources] + ) ) - if not has_profile_id: - if payload.get("agent") is None and has_agent_settings: - from openhands.sdk.settings.model import validate_agent_settings - - try: - payload["agent"] = validate_agent_settings( - payload["agent_settings"] - ).create_agent() - except (TypeError, ValueError) as exc: - raise ValueError(str(exc)) from exc - elif isinstance(payload.get("agent"), dict): - agent_payload = dict(payload["agent"]) - if "kind" not in agent_payload and "llm" in agent_payload: - agent_payload["kind"] = "Agent" - payload["agent"] = agent_payload + if payload.get("agent_profile") is not None: + payload["agent_profile"] = validate_agent_profile(payload["agent_profile"]) + if isinstance(payload.get("agent"), dict): + agent_payload = dict(payload["agent"]) + if "kind" not in agent_payload and "llm" in agent_payload: + agent_payload["kind"] = "Agent" + payload["agent"] = agent_payload return payload @model_validator(mode="after") def _require_agent(self) -> StartConversationRequest: - if self.agent is None and self.agent_profile_id is None: + has_profile = ( + self.agent_profile_id is not None or self.agent_profile is not None + ) + if not has_profile and self.agent is None and self.agent_settings is None: + raise ValueError( + "One of `agent`, `agent_profile_id`, `agent_profile`, or" + " `agent_settings` must be provided" + ) + additions = self.agent_launch_additions + if additions is not None and additions.llm_profile_ref and not has_profile: raise ValueError( - "One of `agent`, `agent_settings`, or" - " `agent_profile_id` must be provided" + "`agent_launch_additions.llm_profile_ref` requires" + " `agent_profile_id` or `agent_profile`" ) return self diff --git a/openhands-sdk/openhands/sdk/profiles/__init__.py b/openhands-sdk/openhands/sdk/profiles/__init__.py index 1dc2bf6b24..87c22f6a7f 100644 --- a/openhands-sdk/openhands/sdk/profiles/__init__.py +++ b/openhands-sdk/openhands/sdk/profiles/__init__.py @@ -26,9 +26,17 @@ rename_llm_profile, ) from openhands.sdk.profiles.resolver import ( + AgentLaunchAdditions, + AgentLaunchCatalog, + AgentLaunchError, + AgentLaunchPlan, + AgentLaunchRuntime, AgentProfileDiagnostics, DanglingMcpServerRef, ProfileNotFound, + UnresolvedProfileReferences, + agent_settings_launch_source, + prepare_agent_launch, resolve_agent_profile, resolve_agent_profile_dry_run, ) @@ -41,6 +49,11 @@ __all__ = [ "AGENT_PROFILE_SCHEMA_VERSION", "ACPAgentProfile", + "AgentLaunchAdditions", + "AgentLaunchCatalog", + "AgentLaunchError", + "AgentLaunchPlan", + "AgentLaunchRuntime", "AgentProfile", "AgentProfileBase", "AgentProfileDiagnostics", @@ -54,11 +67,14 @@ "ProfileReferenced", "ProfileVerificationSettings", "SEED_PROFILE_NAME", + "UnresolvedProfileReferences", + "agent_settings_launch_source", "build_profile_verification", "build_seed_profile", "cascade_rename", "delete_llm_profile", "find_referrers", + "prepare_agent_launch", "rename_llm_profile", "resolve_agent_profile", "resolve_agent_profile_dry_run", diff --git a/openhands-sdk/openhands/sdk/profiles/agent_profile.py b/openhands-sdk/openhands/sdk/profiles/agent_profile.py index 78f7e5dbaf..df6cc333c9 100644 --- a/openhands-sdk/openhands/sdk/profiles/agent_profile.py +++ b/openhands-sdk/openhands/sdk/profiles/agent_profile.py @@ -313,6 +313,20 @@ class LaunchedAgentProfile(BaseModel): "null preserves unrestricted behavior for older conversations." ), ) + inline: bool = Field( + default=False, + description=( + "True when the conversation was launched from an inline " + "`agent_profile` draft rather than a stored profile." + ), + ) + llm_profile_ref: str | None = Field( + default=None, + description=( + "Per-launch LLM profile override from `agent_launch_additions`. " + "null means the profile's own `llm_profile_ref` was used." + ), + ) def allows_secret(self, name: str) -> bool: return self.secret_refs is None or name in self.secret_refs diff --git a/openhands-sdk/openhands/sdk/profiles/resolver.py b/openhands-sdk/openhands/sdk/profiles/resolver.py index 4fb26297e2..56bc070ba2 100644 --- a/openhands-sdk/openhands/sdk/profiles/resolver.py +++ b/openhands-sdk/openhands/sdk/profiles/resolver.py @@ -1,11 +1,13 @@ -"""``resolve_agent_profile()`` — the join point between profiles and execution. +"""Build a conversation's agent from an Agent Profile. -A profile carries *references* (``llm_profile_ref`` / ``mcp_server_refs``) plus a -``disabled_skills`` deny-list, and is secret-free at rest; an -:data:`~openhands.sdk.settings.model.AgentSettingsConfig` embeds the resolved -``llm`` / ``mcp_config`` / skills. This module resolves the former into the -latter so ``create_agent`` / ``apply_agent_settings_diff`` / -``validate_agent_settings`` stay unchanged. See epic #3713. +:func:`prepare_agent_launch` is the single place a launch agent is built. A +profile carries *references* (``llm_profile_ref`` / ``mcp_server_refs``) plus a +``disabled_skills`` deny-list and is secret-free at rest; the launch resolves +those against an :class:`AgentLaunchCatalog`, then applies the +runtime-dependent and per-launch pieces (:class:`AgentLaunchRuntime`, +:class:`AgentLaunchAdditions`). :func:`resolve_agent_profile_dry_run` calls the +same function with side effects off, so a preview can never disagree with a +launch. See epic #3713 and #5141. Skills are *not* modeled like MCP servers. ``mcp_server_refs`` is a safe allow-list because ``mcp_config`` is a complete, persisted, user-authored map. @@ -29,33 +31,44 @@ from __future__ import annotations import shlex -from collections.abc import Container -from typing import TYPE_CHECKING, Any +from collections.abc import Container, Mapping, Sequence +from dataclasses import dataclass +from datetime import datetime +from typing import TYPE_CHECKING, Any, Literal -from pydantic import BaseModel, Field, SecretStr +from pydantic import BaseModel, ConfigDict, Field, SecretStr from openhands.sdk.context.agent_context import AgentContext from openhands.sdk.mcp.config import MCPServer from openhands.sdk.profiles.agent_profile import ( ACPAgentProfile, + LaunchedAgentProfile, OpenHandsAgentProfile, ) +from openhands.sdk.profiles.seed import build_seed_profile from openhands.sdk.settings.acp_providers import get_acp_provider from openhands.sdk.settings.model import ( AGENT_SETTINGS_SCHEMA_VERSION, AgentSettingsConfig, + OpenHandsAgentSettings, validate_agent_settings, ) from openhands.sdk.skills import Skill +from openhands.sdk.tool.defaults import default_tool_specs from openhands.sdk.utils.pydantic_secrets import REDACTED_SECRET_VALUE if TYPE_CHECKING: + from openhands.sdk.agent.base import AgentBase from openhands.sdk.llm.llm import LLM from openhands.sdk.llm.llm_profile_store import LLMProfileLoader from openhands.sdk.utils.cipher import Cipher +ACPSkillSourcing = Literal["native", "openhands_managed"] +ProfileOrigin = Literal["stored", "inline"] + + class ProfileNotFound(Exception): """A referenced profile (e.g. ``llm_profile_ref``) does not exist. @@ -78,20 +91,156 @@ def __init__(self, missing: list[str]) -> None: ) +class AgentLaunchError(ValueError): + """A launch request that cannot be satisfied as given.""" + + code = "invalid_agent_launch" + + def to_detail(self) -> dict[str, Any]: + return {"code": self.code, "message": str(self)} + + +class UnresolvedProfileReferences(AgentLaunchError): + """An Agent Profile references an LLM profile or MCP servers that don't exist.""" + + code = "unresolved_profile_references" + + def __init__( + self, + *, + llm_profile_ref: str | None = None, + mcp_server_refs: Sequence[str] = (), + ) -> None: + self.llm_profile_ref = llm_profile_ref + self.mcp_server_refs = list(mcp_server_refs) + problems = [] + if llm_profile_ref is not None: + problems.append(f"LLM profile {llm_profile_ref!r} not found") + if self.mcp_server_refs: + problems.append( + "MCP server(s) not configured: " + ", ".join(self.mcp_server_refs) + ) + super().__init__("; ".join(problems)) + + def to_detail(self) -> dict[str, Any]: + return { + **super().to_detail(), + "dangling_llm_profile_ref": self.llm_profile_ref, + "dangling_mcp_server_refs": self.mcp_server_refs, + } + + +class AgentLaunchAdditions(BaseModel): + """Per-launch additions applied on top of the resolved agent. + + Additions never widen what a profile exposes: they carry no tools, MCP + servers, skills or secrets. + """ + + model_config = ConfigDict(extra="forbid") + + system_message_suffix_append: str | None = Field( + default=None, + max_length=32768, + description=( + "Deployment-controlled text appended to the resolved agent's " + "system-message suffix." + ), + ) + llm_profile_ref: str | None = Field( + default=None, + min_length=1, + description=( + "LLM profile to launch an OpenHands Agent Profile with instead of " + "its own `llm_profile_ref`. Recorded in `launched_agent_profile`; " + "the stored profile is not modified. Only valid with " + "`agent_profile_id` or `agent_profile`." + ), + ) + + +class AgentLaunchRuntime(BaseModel): + """Launch inputs that depend on the runtime the agent will run in.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + browser_available: bool = Field( + default=False, + description=( + "Whether the runtime can run the browser tool set. Added to a " + "profile that uses the default tool set." + ), + ) + acp_skill_sourcing: ACPSkillSourcing = Field( + default="native", + description=( + "'native': an ACP CLI reads its own skills, so none are injected. " + "'openhands_managed': inject the skill catalog (container runtimes)." + ), + ) + stream: bool = Field( + default=False, + description=( + "Force token streaming on a profile's LLM. Set by servers that wire " + "a token callback." + ), + ) + load_memory: bool = Field( + default=False, + description="The user's global persistent-memory preference.", + ) + + def uses_skill_catalog(self, agent_kind: str) -> bool: + """Whether a profile of ``agent_kind`` launches with the skill catalog.""" + return agent_kind != "acp" or self.acp_skill_sourcing == "openhands_managed" + + +@dataclass(frozen=True, kw_only=True) +class AgentLaunchCatalog: + """Shared resources an Agent Profile's references resolve against. + + ``skills`` is the discovered skill catalog; ``None`` means discovery was not + run. ``base_settings`` supplies the fields a profile does not model (e.g. + ``critic_api_key``); only the deprecated ``agent_settings`` launch sets it. + """ + + llm_store: LLMProfileLoader + mcp_config: Mapping[str, MCPServer] + skills: Sequence[Skill] | None + cipher: Cipher | None = None + base_settings: AgentSettingsConfig | None = None + + +@dataclass(frozen=True, kw_only=True) +class AgentLaunchPlan: + """What :func:`prepare_agent_launch` resolved. + + ``settings`` is ``None`` for a raw ``agent`` launch; ``agent`` is ``None`` + when the launch was prepared with ``build_agent=False``. + ``allowed_secrets`` is the profile's secret allow-list (``None`` = + unrestricted). + """ + + settings: AgentSettingsConfig | None + agent: AgentBase | None + launched_profile: LaunchedAgentProfile | None + allowed_secrets: frozenset[str] | None + + class AgentProfileDiagnostics(BaseModel): - """Side-effect-free report of what :func:`resolve_agent_profile` would do. + """Side-effect-free report of what :func:`prepare_agent_launch` would do. Consumed by ``POST /{id}/materialize`` (#3719) and the canvas editor. The - verdict (:attr:`valid`) and the dangling-ref lists match exactly what a real - resolve produces; :attr:`resolved_settings` is the redacted settings dump - (present only when :attr:`valid`). + verdict (:attr:`valid`) and :attr:`resolved_settings` come from the same + launch function a conversation start uses; :attr:`resolved_settings` is the + redacted settings dump (present only when :attr:`valid`). """ agent_kind: str valid: bool = False errors: list[str] = Field(default_factory=list) - # OpenHands LLM reference. + # OpenHands LLM reference (the per-launch override when one was given). llm_profile_ref: str | None = None llm_profile_resolved: bool = False llm_api_key_set: bool = False @@ -101,25 +250,20 @@ class AgentProfileDiagnostics(BaseModel): resolved_mcp_config_keys: list[str] = Field(default_factory=list) dangling_mcp_server_refs: list[str] = Field(default_factory=list) - # Skill selection (OpenHands only). ``disabled_skills`` is a deny-list over - # the discovered catalog, so — unlike ``mcp_server_refs`` — it can never - # dangle: a disabled name absent from the catalog is a harmless no-op. - # ``resolved_skills`` is what would actually reach the agent (catalog minus - # disabled). + # Skill selection. ``disabled_skills`` is a deny-list over the discovered + # catalog, so — unlike ``mcp_server_refs`` — it can never dangle. + # ``resolved_skills`` is what would actually reach the agent. disabled_skills: list[str] = Field(default_factory=list) resolved_skills: list[str] = Field(default_factory=list) # Secret scope (both variants). ``None`` = every secret the conversation is # started with; a list = only those names, with nothing added back. - # No dangling report: this is an - # allow-list over what a launch supplies, so an unmatched name is a no-op. secret_refs: list[str] | None = None # ACP provider credential channels the editor/materialize checks (ACP only). # These are NOT jointly required: authentication needs the API key *or* one # of the file-content credentials, and the base URL is optional proxy - # routing. Keeping them in separate fields lets the editor mark set/missing - # honestly instead of treating a working api-key-only setup as incomplete. + # routing. acp_api_key_secret_name: str | None = None acp_base_url_secret_name: str | None = None acp_file_secret_names: list[str] = Field(default_factory=list) @@ -128,20 +272,10 @@ class AgentProfileDiagnostics(BaseModel): resolved_settings: dict[str, Any] | None = None -def _server_names(mcp_config: dict[str, MCPServer]) -> list[str]: - return list(mcp_config) - - def _partition_refs( refs: list[str], available: Container[str] ) -> tuple[list[str], list[str]]: - """Split ``refs`` into ``(resolved, dangling)`` by membership in ``available``. - - Order-preserving and de-duplicated: a name repeated in ``refs`` is kept once, - in first position. Shared by the MCP and skill filters so both partition - identically — in particular both collapse duplicate refs, which the ACP skill - path needs (``AgentContext`` rejects duplicate skill names). - """ + """Split ``refs`` into order-preserving, de-duplicated ``(resolved, dangling)``.""" seen: set[str] = set() resolved: list[str] = [] dangling: list[str] = [] @@ -154,41 +288,24 @@ def _partition_refs( def _compute_mcp_filter( - mcp_config: dict[str, MCPServer], + mcp_config: Mapping[str, MCPServer], refs: list[str] | None, ) -> tuple[dict[str, MCPServer], list[str], list[str]]: - """Resolve ``mcp_server_refs`` against the user's ``mcp_config``. - - ``None`` → passthrough (all servers); a non-null list filters to the named - keys. Returns ``(filtered_servers, resolved_names, dangling_names)``. - """ + """Resolve ``mcp_server_refs``: ``None`` keeps every server, a list filters.""" if refs is None: - return mcp_config, _server_names(mcp_config), [] + return dict(mcp_config), list(mcp_config), [] resolved, dangling = _partition_refs(refs, mcp_config) return {k: mcp_config[k] for k in resolved}, resolved, dangling def _apply_disabled_skills( - available_skills: list[Skill] | None, + available_skills: Sequence[Skill] | None, disabled: list[str], ) -> list[Skill]: - """Filter the discovered skill catalog by the profile's deny-list. - - Skills are discovered from many incomplete, drifting sources, so selection - is by *exclusion*, not by an allow-list of names (which would dangle when the - catalog a profile was authored against differs from the one resolved at - launch — the #4017 root cause). A disabled name absent from the catalog is a - harmless no-op. - - The catalog is de-duplicated by name (last occurrence wins, matching - ``load_all_skills``'s later-source-overrides precedence) so a caller passing a - colliding catalog — the app-server's "fuller catalog" merges multiple sources - whose names can collide — cannot trip ``AgentContext``'s duplicate-name - validator. So this can never raise. - - ``available_skills is None`` (discovery skipped or failed) → no skills, the - caller having surfaced its own signal. ``disabled == []`` → the whole - (de-duplicated) catalog. + """Filter the discovered skill catalog by a deny-list. + + De-duplicated by name (last occurrence wins, matching ``load_all_skills``) + so a colliding catalog cannot trip ``AgentContext``'s duplicate-name check. """ if not available_skills: return [] @@ -197,6 +314,18 @@ def _apply_disabled_skills( return [s for s in by_name.values() if s.name not in denied] +def _launch_skills( + profile: OpenHandsAgentProfile | ACPAgentProfile, + catalog: AgentLaunchCatalog, + runtime: AgentLaunchRuntime, +) -> list[Skill]: + if isinstance(profile, OpenHandsAgentProfile): + return _apply_disabled_skills(catalog.skills, profile.disabled_skills) + if not runtime.uses_skill_catalog(profile.agent_kind): + return [] + return _apply_disabled_skills(catalog.skills, []) + + def _api_key_set(llm: LLM) -> bool: """``True`` when the resolved LLM carries a non-empty, non-redacted key.""" api_key = llm.api_key @@ -209,14 +338,7 @@ def _api_key_set(llm: LLM) -> bool: def _acp_credential_channels( acp_server: str, ) -> tuple[str | None, str | None, list[str]]: - """Provider credential channels for ``acp_server`` via ``ACP_PROVIDERS``. - - Returns ``(api_key_env_var, base_url_env_var, file_secret_names)`` kept - separate by role: the API-key env var and the file-content credentials are - *alternative* auth mechanisms (one suffices), and the base URL is optional - proxy routing — not jointly required. All empty/``None`` for ``'custom'`` - servers, whose creds the user manages directly. - """ + """``(api_key_env_var, base_url_env_var, file_secret_names)`` for a server.""" info = get_acp_provider(acp_server) if info is None: return None, None, [] @@ -228,73 +350,78 @@ def _build_openhands_settings( profile: OpenHandsAgentProfile, llm: LLM, mcp_config: dict[str, MCPServer], - filtered_skills: list[Skill], + skills: list[Skill], + base: AgentSettingsConfig | None, ) -> AgentSettingsConfig: - """Compose the resolved ``OpenHandsAgentSettings`` from a profile + LLM. + """Compose ``OpenHandsAgentSettings`` from a profile and its resolved references. - ``filtered_skills`` (the discovered catalog minus ``disabled_skills``) is the - sole user/public skill source (profiles no longer embed skills). ``load_project_skills=True`` lets ``LocalConversation`` lazily load - repo-scoped project skills, which can't be resolved here (no workspace yet); - ``disabled_skills`` is carried onto the context so that lazy load applies the - same deny-list. ``load_user_skills`` / ``load_public_skills`` stay False on - purpose: user/public skills already arrive via ``filtered_skills``, so - enabling the flags would double-load them. + repo-scoped skills (no workspace exists yet), and ``disabled_skills`` rides + the context so that lazy load honors the same deny-list. """ - payload = { - "schema_version": AGENT_SETTINGS_SCHEMA_VERSION, - "agent_kind": "openhands", + context_fields: dict[str, Any] = { + "skills": skills, + "system_message_suffix": profile.system_message_suffix, + "load_project_skills": True, + "disabled_skills": profile.disabled_skills, + "current_datetime": datetime.now().astimezone(), + } + fields: dict[str, Any] = { "agent": profile.agent, "llm": llm, "mcp_config": mcp_config, - # Tri-state passthrough; create_agent materializes None. "tools": profile.tools, - "agent_context": AgentContext( - skills=filtered_skills, - system_message_suffix=profile.system_message_suffix, - load_project_skills=True, - disabled_skills=profile.disabled_skills, - ), "condenser": profile.condenser, - "verification": profile.verification.model_dump(), "enable_sub_agents": profile.enable_sub_agents, "enable_switch_llm_tool": profile.enable_switch_llm_tool, "tool_concurrency_limit": profile.tool_concurrency_limit, } - return validate_agent_settings(payload) + if isinstance(base, OpenHandsAgentSettings): + return base.model_copy( + update={ + **fields, + "agent_context": base.agent_context.model_copy(update=context_fields), + "verification": base.verification.model_copy( + update=profile.verification.model_dump() + ), + } + ) + return validate_agent_settings( + { + "schema_version": AGENT_SETTINGS_SCHEMA_VERSION, + "agent_kind": "openhands", + **fields, + "agent_context": AgentContext(**context_fields), + "verification": profile.verification.model_dump(), + } + ) def _build_acp_settings( profile: ACPAgentProfile, mcp_config: dict[str, MCPServer], - managed_skills: list[Skill], + skills: list[Skill], + base: AgentSettingsConfig | None, ) -> AgentSettingsConfig: - """Compose the resolved ``ACPAgentSettings`` from a profile. - - ``acp_command`` is stored as a shell string and split into the settings' - token list. No credential is set — provider creds ride - ``state.secret_registry``. - - ``load_project_skills`` stays ``False``: an ACP CLI reads ``AGENTS.md`` / - ``CLAUDE.md`` and its own project skills from the session cwd, so loading - them here would duplicate that content in the prompt (#4019). - ``managed_skills`` is what the *deployment* chose to inject — empty when the - ACP CLI can reach its own host configuration, non-empty in a container where - it cannot. ``current_datetime=None`` matches ACP's no-timestamp convention. - A ``custom`` server has no default command, so one must be supplied. + """Compose ``ACPAgentSettings`` from a profile. + + No credential is set — provider creds ride ``state.secret_registry``. + Project skills stay off because the ACP CLI reads the repository itself + (#4019), and ``current_datetime=None`` matches ACP's no-timestamp + convention. A ``custom`` server has no default command. """ command = shlex.split(profile.acp_command) if profile.acp_command else [] if profile.acp_server == "custom" and not command: - raise ValueError( + raise AgentLaunchError( "acp_command is required when acp_server='custom' — there is no " "default launch command to fall back to" ) - agent_context = AgentContext( - skills=managed_skills, current_datetime=None, load_project_skills=False - ) - payload = { - "schema_version": AGENT_SETTINGS_SCHEMA_VERSION, - "agent_kind": "acp", + context_fields: dict[str, Any] = { + "skills": skills, + "current_datetime": None, + "load_project_skills": False, + } + fields: dict[str, Any] = { "acp_server": profile.acp_server, "acp_model": profile.acp_model, "acp_session_mode": profile.acp_session_mode, @@ -303,9 +430,264 @@ def _build_acp_settings( "acp_command": command, "acp_args": list(profile.acp_args) if profile.acp_args else [], "mcp_config": mcp_config, - "agent_context": agent_context, } - return validate_agent_settings(payload) + base_context = None if base is None else base.agent_context + if base is not None and base.agent_kind == "acp": + context = ( + base_context.model_copy(update=context_fields) + if base_context is not None + else AgentContext(**context_fields) + ) + return base.model_copy(update={**fields, "agent_context": context}) + return validate_agent_settings( + { + "schema_version": AGENT_SETTINGS_SCHEMA_VERSION, + "agent_kind": "acp", + **fields, + "agent_context": AgentContext(**context_fields), + } + ) + + +def _load_llm(catalog: AgentLaunchCatalog, name: str) -> LLM | None: + try: + return catalog.llm_store.load(name, cipher=catalog.cipher) + except FileNotFoundError: + return None + + +def _resolve_settings( + profile: OpenHandsAgentProfile | ACPAgentProfile, + catalog: AgentLaunchCatalog, + runtime: AgentLaunchRuntime, + llm_profile_ref: str | None, +) -> AgentSettingsConfig: + """Resolve a profile's references; every dangling one is reported at once.""" + mcp_config, _, dangling_mcp = _compute_mcp_filter( + catalog.mcp_config, profile.mcp_server_refs + ) + skills = _launch_skills(profile, catalog, runtime) + if isinstance(profile, ACPAgentProfile): + if dangling_mcp: + raise UnresolvedProfileReferences(mcp_server_refs=dangling_mcp) + return _build_acp_settings(profile, mcp_config, skills, catalog.base_settings) + + llm_ref = llm_profile_ref or profile.llm_profile_ref + llm = _load_llm(catalog, llm_ref) + if llm is None or dangling_mcp: + raise UnresolvedProfileReferences( + llm_profile_ref=llm_ref if llm is None else None, + mcp_server_refs=dangling_mcp, + ) + return _build_openhands_settings( + profile, llm, mcp_config, skills, catalog.base_settings + ) + + +def _finish_context( + context: AgentContext | None, + *, + is_acp: bool, + runtime: AgentLaunchRuntime, + additions: AgentLaunchAdditions | None, +) -> AgentContext | None: + updates: dict[str, Any] = {} + addition = ( + (additions.system_message_suffix_append or "").strip() if additions else "" + ) + if addition: + existing = ((context and context.system_message_suffix) or "").strip() + updates["system_message_suffix"] = ( + f"{existing}\n\n{addition}" if existing else addition + ) + if runtime.load_memory: + updates["load_memory"] = True + if ( + is_acp + and not runtime.uses_skill_catalog("acp") + and context is not None + and ( + context.skills + or context.load_user_skills + or context.load_public_skills + or context.registered_marketplaces + ) + ): + updates.update( + skills=[], + load_user_skills=False, + load_public_skills=False, + registered_marketplaces=[], + ) + if not updates: + return context + # A missing context means "no prompt context", so none is synthesized with + # a timestamp. + base = context if context is not None else AgentContext(current_datetime=None) + return base.model_copy(update=updates) + + +def _finish_settings( + settings: AgentSettingsConfig, + runtime: AgentLaunchRuntime, + additions: AgentLaunchAdditions | None, +) -> AgentSettingsConfig: + if not isinstance(settings, OpenHandsAgentSettings): + return settings.model_copy( + update={ + "agent_context": _finish_context( + settings.agent_context, + is_acp=True, + runtime=runtime, + additions=additions, + ) + } + ) + tools = settings.tools + if tools is None: + tools = default_tool_specs( + enable_sub_agents=settings.enable_sub_agents, + enable_browser=runtime.browser_available, + ) + llm = settings.llm + if runtime.stream: + llm = llm.model_copy(update={"stream": True}) + return settings.model_copy( + update={ + "tools": tools, + "llm": llm, + "agent_context": _finish_context( + settings.agent_context, + is_acp=False, + runtime=runtime, + additions=additions, + ), + } + ) + + +def _finish_agent( + agent: AgentBase, + runtime: AgentLaunchRuntime, + additions: AgentLaunchAdditions | None, +) -> AgentBase: + from openhands.sdk.agent.acp_agent import ACPAgent + + context = _finish_context( + agent.agent_context, + is_acp=isinstance(agent, ACPAgent), + runtime=runtime, + additions=additions, + ) + if context is agent.agent_context: + return agent + return agent.model_copy(update={"agent_context": context}) + + +def prepare_agent_launch( + source: OpenHandsAgentProfile | ACPAgentProfile | AgentBase, + *, + catalog: AgentLaunchCatalog | None = None, + runtime: AgentLaunchRuntime | None = None, + additions: AgentLaunchAdditions | None = None, + profile_origin: ProfileOrigin | None = "stored", + build_agent: bool = True, +) -> AgentLaunchPlan: + """Build the agent a conversation launches with. + + ``source`` is an Agent Profile (resolved against ``catalog``) or a raw + agent, which only gets the runtime's memory/ACP-skill policy and + ``additions`` applied. ``profile_origin`` selects the recorded provenance: + ``None`` records none. ``build_agent=False`` skips ``create_agent()``, the + only step with side effects (a subscription LLM refreshes its credentials). + + Raises: + UnresolvedProfileReferences: the profile's LLM or MCP references dangle. + AgentLaunchError: the launch inputs are inconsistent. + """ + runtime = runtime or AgentLaunchRuntime() + llm_override = additions.llm_profile_ref if additions else None + + if not isinstance(source, OpenHandsAgentProfile | ACPAgentProfile): + if llm_override is not None: + raise AgentLaunchError( + "agent_launch_additions.llm_profile_ref requires an Agent Profile" + ) + return AgentLaunchPlan( + settings=None, + agent=_finish_agent(source, runtime, additions), + launched_profile=None, + allowed_secrets=None, + ) + + if catalog is None: + raise TypeError("An Agent Profile launch requires a catalog") + if llm_override is not None and isinstance(source, ACPAgentProfile): + raise AgentLaunchError( + "agent_launch_additions.llm_profile_ref does not apply to ACP profiles" + ) + + settings = _finish_settings( + _resolve_settings(source, catalog, runtime, llm_override), + runtime, + additions, + ) + launched = None + if profile_origin is not None: + launched = LaunchedAgentProfile( + agent_profile_id=source.id, + revision=source.revision, + secret_refs=source.secret_refs, + inline=profile_origin == "inline", + llm_profile_ref=llm_override, + ) + return AgentLaunchPlan( + settings=settings, + agent=settings.create_agent() if build_agent else None, + launched_profile=launched, + allowed_secrets=( + None if source.secret_refs is None else frozenset(source.secret_refs) + ), + ) + + +@dataclass(frozen=True) +class _FixedLLMLoader: + name: str + llm: LLM + + def load(self, name: str, *, cipher: Cipher | None = None) -> LLM: # noqa: ARG002 + if name != self.name: + raise FileNotFoundError(name) + return self.llm + + +_AGENT_SETTINGS_PROFILE_NAME = "agent_settings" + + +def agent_settings_launch_source( + settings: AgentSettingsConfig, +) -> tuple[OpenHandsAgentProfile | ACPAgentProfile, AgentLaunchCatalog]: + """Convert a deprecated ``agent_settings`` launch into an inline profile. + + The payload's own LLM, MCP servers and skills become the catalog, so the + launch resolves exactly what the client sent. + """ + profile = build_seed_profile( + settings, _AGENT_SETTINGS_PROFILE_NAME, name=_AGENT_SETTINGS_PROFILE_NAME + ) + context = settings.agent_context + if isinstance(profile, OpenHandsAgentProfile) and context is not None: + profile = profile.model_copy( + update={"disabled_skills": list(context.disabled_skills)} + ) + catalog = AgentLaunchCatalog( + llm_store=_FixedLLMLoader(_AGENT_SETTINGS_PROFILE_NAME, settings.llm), + mcp_config=settings.mcp_config, + skills=list(context.skills) if context is not None else [], + base_settings=settings, + ) + return profile, catalog def resolve_agent_profile( @@ -318,40 +700,27 @@ def resolve_agent_profile( ) -> AgentSettingsConfig: """Resolve a profile's references into a validated ``AgentSettingsConfig``. - ``mcp_config`` is the user's globally-configured MCP server map, already - decrypted by the caller (the agent-server runs settings decryption - before calling). ``available_skills`` is the server-discovered skill catalog - (the agent-server caller passes the result of ``load_all_skills``); an - OpenHands profile keeps all of it except the names in ``disabled_skills``, - and an ACP profile keeps all of it (it has no deny-list). ``None`` means the - caller injected no catalog — discovery was not run, failed, or, for ACP, the - 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. + The reference-resolution step of :func:`prepare_agent_launch`, without the + runtime and per-launch pieces. ``available_skills`` is the skill catalog the + agent gets (minus ``disabled_skills`` for an OpenHands profile). Raises: ProfileNotFound: ``llm_profile_ref`` does not exist (OpenHands path). DanglingMcpServerRef: an ``mcp_server_refs`` entry is not in ``mcp_config``. """ - filtered_mcp, _, dangling = _compute_mcp_filter(mcp_config, profile.mcp_server_refs) - if dangling: - raise DanglingMcpServerRef(dangling) - - if isinstance(profile, OpenHandsAgentProfile): - filtered_skills = _apply_disabled_skills( - available_skills, profile.disabled_skills - ) - try: - llm = llm_store.load(profile.llm_profile_ref, cipher=cipher) - except FileNotFoundError as e: - 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_acp_settings( - profile, filtered_mcp, _apply_disabled_skills(available_skills, []) + catalog = AgentLaunchCatalog( + llm_store=llm_store, + mcp_config=mcp_config, + skills=available_skills, + cipher=cipher, ) + runtime = AgentLaunchRuntime(acp_skill_sourcing="openhands_managed") + try: + return _resolve_settings(profile, catalog, runtime, None) + except UnresolvedProfileReferences as e: + if e.mcp_server_refs: + raise DanglingMcpServerRef(e.mcp_server_refs) from e + raise ProfileNotFound(f"LLM profile {e.llm_profile_ref!r} not found") from e def resolve_agent_profile_dry_run( @@ -361,65 +730,46 @@ def resolve_agent_profile_dry_run( mcp_config: dict[str, MCPServer], available_skills: list[Skill] | None, cipher: Cipher | None = None, + runtime: AgentLaunchRuntime | None = None, + additions: AgentLaunchAdditions | None = None, ) -> AgentProfileDiagnostics: - """Compute :class:`AgentProfileDiagnostics` without raising or side effects. - - Mirrors :func:`resolve_agent_profile`'s composition but records dangling LLM / - MCP refs as diagnostics instead of raising, so the editor / ``/materialize`` - (#3719) can show a faithful set/missing report with secrets redacted. Skills - use a deny-list (``disabled_skills``) that can't dangle, so there is no skill - error to report — ``resolved_skills`` is just the catalog minus the disabled - names. ``available_skills=None`` (discovery skipped or failed) means no - user/public skills resolve. + """Report what :func:`prepare_agent_launch` would build, without raising. + + Runs the launch with ``build_agent=False``. ``runtime`` defaults to one that + gives an ACP profile the supplied ``available_skills``. """ - filtered_mcp, resolved, dangling = _compute_mcp_filter( + runtime = runtime or AgentLaunchRuntime(acp_skill_sourcing="openhands_managed") + catalog = AgentLaunchCatalog( + llm_store=llm_store, + mcp_config=mcp_config, + skills=available_skills, + cipher=cipher, + ) + _, resolved_mcp, dangling_mcp = _compute_mcp_filter( mcp_config, profile.mcp_server_refs ) diagnostics = AgentProfileDiagnostics( agent_kind=profile.agent_kind, mcp_server_refs=profile.mcp_server_refs, - resolved_mcp_config_keys=resolved, - dangling_mcp_server_refs=dangling, + resolved_mcp_config_keys=resolved_mcp, + dangling_mcp_server_refs=dangling_mcp, secret_refs=profile.secret_refs, + resolved_skills=[s.name for s in _launch_skills(profile, catalog, runtime)], ) - if dangling: - diagnostics.errors.append( - "MCP server(s) not configured: " + ", ".join(dangling) - ) - # Skill selection report. Deny-list semantics: the catalog minus disabled - # names, never dangling. An ACP profile has no deny-list of its own — its - # catalog is whatever the deployment injects (empty unless the caller passes - # one), and never includes project skills (#4019). if isinstance(profile, OpenHandsAgentProfile): - filtered_skills = _apply_disabled_skills( - available_skills, profile.disabled_skills - ) diagnostics.disabled_skills = profile.disabled_skills - else: - filtered_skills = _apply_disabled_skills(available_skills, []) - diagnostics.resolved_skills = [s.name for s in filtered_skills] - - llm: LLM | None = None - if isinstance(profile, OpenHandsAgentProfile): - diagnostics.llm_profile_ref = profile.llm_profile_ref + llm_ref = (additions and additions.llm_profile_ref) or profile.llm_profile_ref + diagnostics.llm_profile_ref = llm_ref try: - llm = llm_store.load(profile.llm_profile_ref, cipher=cipher) - diagnostics.llm_profile_resolved = True - diagnostics.llm_api_key_set = _api_key_set(llm) - except FileNotFoundError: - diagnostics.errors.append( - f"LLM profile {profile.llm_profile_ref!r} not found" - ) + llm = _load_llm(catalog, llm_ref) except Exception as e: - # Keep the dry-run total: the store can raise filelock.TimeoutError - # (lock contention), OSError, or a validation error before its own - # handler runs. Surface those as a diagnostic instead of crashing - # the editor preview (#3719) — distinct from a definitively-missing - # profile above. - diagnostics.errors.append( - f"Could not load LLM profile {profile.llm_profile_ref!r}: {e}" - ) + # The store can raise lock timeouts or validation errors; keep the + # preview total. + diagnostics.errors.append(f"Could not load LLM profile {llm_ref!r}: {e}") + else: + diagnostics.llm_profile_resolved = llm is not None + diagnostics.llm_api_key_set = llm is not None and _api_key_set(llm) else: ( diagnostics.acp_api_key_secret_name, @@ -427,29 +777,30 @@ def resolve_agent_profile_dry_run( diagnostics.acp_file_secret_names, ) = _acp_credential_channels(profile.acp_server) - diagnostics.valid = not diagnostics.errors - if diagnostics.valid: - # Building settings can still fail on input that passes profile - # validation (e.g. an acp_command with unbalanced shell quotes, which - # shlex.split rejects). Keep the dry-run total: surface such failures as - # diagnostics rather than raising, matching the API contract. + if not diagnostics.errors: try: - if isinstance(profile, OpenHandsAgentProfile): - # valid here implies the LLM load above succeeded; gate - # explicitly rather than via assert (stripped under python -O). - if llm is None: - raise RuntimeError( - "OpenHands profile marked valid without a resolved LLM" - ) - settings = _build_openhands_settings( - profile, llm, filtered_mcp, filtered_skills + plan = prepare_agent_launch( + profile, + catalog=catalog, + runtime=runtime, + additions=additions, + build_agent=False, + ) + except UnresolvedProfileReferences as e: + if e.mcp_server_refs: + diagnostics.errors.append( + "MCP server(s) not configured: " + ", ".join(e.mcp_server_refs) + ) + if e.llm_profile_ref is not None: + diagnostics.errors.append( + f"LLM profile {e.llm_profile_ref!r} not found" ) - else: - settings = _build_acp_settings(profile, filtered_mcp, filtered_skills) - # No expose context => secrets redacted (mcp env/headers, llm api_key). - diagnostics.resolved_settings = settings.model_dump(mode="json") except Exception as e: - diagnostics.valid = False diagnostics.errors.append(f"Failed to build agent settings: {e}") + else: + if plan.settings is not None: + # No expose context => secrets redacted (mcp env/headers, api_key). + diagnostics.resolved_settings = plan.settings.model_dump(mode="json") + diagnostics.valid = not diagnostics.errors return diagnostics diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 5bac9b061e..581b2a556a 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -34,7 +34,7 @@ from pydantic.fields import FieldInfo from openhands.sdk.context.agent_context import AgentContext -from openhands.sdk.conversation.request import SendMessageRequest +from openhands.sdk.conversation.message_request import SendMessageRequest from openhands.sdk.conversation.types import ( ConversationObservabilityMetadata, ConversationObservabilitySpanName, diff --git a/tests/agent_server/docker_runtime/test_mediation.py b/tests/agent_server/docker_runtime/test_mediation.py index 614329a013..7ccf38df13 100644 --- a/tests/agent_server/docker_runtime/test_mediation.py +++ b/tests/agent_server/docker_runtime/test_mediation.py @@ -5,6 +5,9 @@ from openhands.agent_server.config import Config from openhands.agent_server.docker_runtime.mediation import ( + PreparedStart, + container_launch_runtime, + finish_start, prepare_start, serialize_start, ) @@ -15,8 +18,11 @@ ) from openhands.sdk import LLM, Agent from openhands.sdk.context import AgentContext -from openhands.sdk.conversation.request import StartConversationRequest -from openhands.sdk.profiles import OpenHandsAgentProfile +from openhands.sdk.conversation.request import ( + AgentLaunchAdditions, + StartConversationRequest, +) +from openhands.sdk.profiles import OpenHandsAgentProfile, UnresolvedProfileReferences from openhands.sdk.secret import LookupSecret, StaticSecret from openhands.sdk.workspace import LocalWorkspace @@ -32,6 +38,13 @@ def config(tmp_path, monkeypatch) -> Config: ) +async def _finish(prepared: PreparedStart) -> StartConversationRequest: + return await finish_start( + prepared, + container_launch_runtime(prepared.settings, browser_available=False), + ) + + @pytest.mark.asyncio async def test_materializes_request_secret_sources(tmp_path, monkeypatch): runtime_config = config(tmp_path, monkeypatch) @@ -53,15 +66,14 @@ def get_value(secret): }, ) - prepared, launched = await prepare_start( - request.model_dump(mode="json"), runtime_config - ) - assert launched is None + prepared = await prepare_start(request.model_dump(mode="json"), runtime_config) + assert prepared.launched is None assert looked_up == ["http://127.0.0.1:8123/api/settings/secrets/SELECTED"] - assert isinstance(prepared.secrets["SELECTED"], StaticSecret) + assert isinstance(prepared.request.secrets["SELECTED"], StaticSecret) + finished = await _finish(prepared) identity = RuntimeProvisioningStore(runtime_config).create(uuid4()) - payload = serialize_start(prepared, identity) + payload = serialize_start(finished, identity) assert "selected-value" not in str(payload) assert "outer-session" not in str(payload) received = StartConversationRequest.model_validate( @@ -83,8 +95,9 @@ async def test_materializes_agent_context_secret_sources(tmp_path, monkeypatch): ), ), ) - prepared, _ = await prepare_start(request.model_dump(mode="json"), runtime_config) - context = prepared.agent.agent_context + prepared = await prepare_start(request.model_dump(mode="json"), runtime_config) + finished = await _finish(prepared) + context = finished.agent.agent_context assert context is not None assert context.secrets is not None source = context.secrets["CONTEXT_SECRET"] @@ -93,7 +106,7 @@ async def test_materializes_agent_context_secret_sources(tmp_path, monkeypatch): @pytest.mark.asyncio -async def test_profile_uses_existing_resolver_and_secret_allowlist( +async def test_profile_launch_scopes_secrets_and_stamps_provenance( tmp_path, monkeypatch ): runtime_config = config(tmp_path, monkeypatch) @@ -112,7 +125,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.agent_launch.discover_profile_skills", lambda: [], ) monkeypatch.setattr( @@ -127,10 +140,59 @@ async def test_profile_uses_existing_resolver_and_secret_allowlist( }, ) - prepared, launched = await prepare_start( - request.model_dump(mode="json"), runtime_config + prepared = await prepare_start(request.model_dump(mode="json"), runtime_config) + assert set(prepared.request.secrets) == {"ALLOWED"} + assert prepared.launched is not None + assert prepared.launched.agent_profile_id == profile.id + assert prepared.launched.revision == profile.revision + + finished = await _finish(prepared) + assert finished.agent_profile_id is None + assert set(finished.secrets) == {"ALLOWED"} + assert finished.agent.llm.api_key is not None + assert finished.agent.llm.api_key.get_secret_value() == "model-key" + + +@pytest.mark.asyncio +async def test_dangling_llm_profile_ref_fails_before_any_container_work( + tmp_path, monkeypatch +): + runtime_config = config(tmp_path, monkeypatch) + profile = OpenHandsAgentProfile( + name="docker-dangling-profile", + llm_profile_ref="never-saved", + tools=[], + ) + get_agent_profile_store().save(profile) + monkeypatch.setattr( + "openhands.agent_server.agent_launch.discover_profile_skills", + lambda: [], + ) + request = StartConversationRequest( + workspace=LocalWorkspace(working_dir="/workspace"), + agent_profile_id=profile.id, + ) + + with pytest.raises(UnresolvedProfileReferences) as exc_info: + await prepare_start(request.model_dump(mode="json"), runtime_config) + assert exc_info.value.llm_profile_ref == "never-saved" + + +@pytest.mark.asyncio +async def test_serialize_start_drops_launch_only_fields(tmp_path, monkeypatch): + runtime_config = config(tmp_path, monkeypatch) + request = StartConversationRequest( + workspace=LocalWorkspace(working_dir="/workspace"), + agent=Agent(llm=LLM(model="test"), tools=[]), + agent_launch_additions=AgentLaunchAdditions( + system_message_suffix_append="" + ), ) - assert set(prepared.secrets) == {"ALLOWED"} - assert prepared.agent_profile_id is None - assert launched is not None - assert launched.agent_profile_id == profile.id + identity = RuntimeProvisioningStore(runtime_config).create(uuid4()) + + payload = serialize_start(request, identity) + + assert "agent_launch_additions" not in payload + assert "agent_profile_id" not in payload + assert "agent_profile" not in payload + assert "agent_settings" not in payload diff --git a/tests/agent_server/test_acp_skill_sourcing.py b/tests/agent_server/test_acp_skill_sourcing.py index 8cd19541dc..a4ffe0a242 100644 --- a/tests/agent_server/test_acp_skill_sourcing.py +++ b/tests/agent_server/test_acp_skill_sourcing.py @@ -15,11 +15,11 @@ import pytest from openhands.agent_server.config import ACPSkillSourcing, Config -from openhands.agent_server.conversation_service import _apply_acp_skill_sourcing from openhands.sdk import LLM, Agent, Conversation from openhands.sdk.agent import ACPAgent, AgentBase from openhands.sdk.context import AgentContext from openhands.sdk.marketplace.registration import MarketplaceRegistration +from openhands.sdk.profiles import AgentLaunchRuntime, prepare_agent_launch from openhands.sdk.settings.model import validate_agent_settings from openhands.sdk.skills import Skill @@ -28,6 +28,14 @@ MANAGED_SKILL = "managed-catalog-skill" +def _launch(agent: AgentBase, sourcing: ACPSkillSourcing) -> AgentBase: + plan = prepare_agent_launch( + agent, runtime=AgentLaunchRuntime(acp_skill_sourcing=sourcing) + ) + assert plan.agent is not None + return plan.agent + + def _managed_skill() -> Skill: return Skill( name=MANAGED_SKILL, @@ -102,7 +110,7 @@ def test_repo_context_never_reaches_the_acp_prompt( tmp_path: Path, sourcing: ACPSkillSourcing ) -> None: project = _workspace(tmp_path) - agent = _apply_acp_skill_sourcing( + agent = _launch( _acp_agent(skills=[_managed_skill()], load_project_skills=True), sourcing ) suffix = _installed_suffix(agent, project) @@ -112,7 +120,7 @@ def test_repo_context_never_reaches_the_acp_prompt( def test_native_sourcing_strips_managed_skills(tmp_path: Path) -> None: project = _workspace(tmp_path) - agent = _apply_acp_skill_sourcing( + agent = _launch( _acp_agent(skills=[_managed_skill()], load_project_skills=True), "native" ) assert agent.agent_context is not None @@ -122,7 +130,7 @@ def test_native_sourcing_strips_managed_skills(tmp_path: Path) -> None: def test_managed_sourcing_keeps_managed_skills(tmp_path: Path) -> None: project = _workspace(tmp_path) - agent = _apply_acp_skill_sourcing( + agent = _launch( _acp_agent(skills=[_managed_skill()], load_project_skills=True), "openhands_managed", ) @@ -134,7 +142,7 @@ def test_managed_sourcing_keeps_managed_skills(tmp_path: Path) -> None: def test_native_sourcing_clears_lazy_skill_sources() -> None: """Flags and marketplace registrations resolve to skills later, so a strip that only emptied ``skills`` would let them back in.""" - agent = _apply_acp_skill_sourcing( + agent = _launch( _acp_agent( load_user_skills=True, load_public_skills=True, @@ -159,9 +167,9 @@ def test_native_sourcing_leaves_a_non_acp_agent_alone() -> None: tools=[], agent_context=AgentContext(skills=[_managed_skill()]), ) - assert _apply_acp_skill_sourcing(agent, "native") is agent + assert _launch(agent, "native") is agent def test_native_sourcing_is_a_no_op_without_skills() -> None: agent = _acp_agent() - assert _apply_acp_skill_sourcing(agent, "native") is agent + assert _launch(agent, "native") is agent diff --git a/tests/agent_server/test_agent_launch_additions.py b/tests/agent_server/test_agent_launch_additions.py index a2911f838c..2400a0aa02 100644 --- a/tests/agent_server/test_agent_launch_additions.py +++ b/tests/agent_server/test_agent_launch_additions.py @@ -4,14 +4,18 @@ from uuid import uuid4 import pytest -from pydantic import ValidationError +from pydantic import SecretStr, ValidationError from openhands.agent_server.conversation_service import ( ConversationService, _append_system_message_suffix, ) from openhands.agent_server.event_service import EventService -from openhands.agent_server.models import LaunchedAgentProfile, StoredConversation +from openhands.agent_server.models import StoredConversation +from openhands.agent_server.persistence import ( + get_agent_profile_store, + get_llm_profile_store, +) from openhands.sdk import LLM, Agent, AgentContext from openhands.sdk.agent.acp_agent import ACPAgent from openhands.sdk.conversation.request import ( @@ -22,6 +26,8 @@ ConversationExecutionStatus, ConversationState, ) +from openhands.sdk.profiles import OpenHandsAgentProfile +from openhands.sdk.secret import StaticSecret from openhands.sdk.tool.client_tool import ClientToolSpec from openhands.sdk.workspace import LocalWorkspace @@ -34,6 +40,9 @@ description="Control the Canvas UI.", parameters={"type": "object", "properties": {}}, ) +_DISCOVER_PATH = "openhands.agent_server.agent_launch.discover_profile_skills" +_BROWSER_PROBE_PATH = "openhands.agent_server.agent_launch.is_tool_usable" +_LLM_PROFILE_REF = "default" def _agent(suffix: str | None = None) -> Agent: @@ -43,6 +52,23 @@ def _agent(suffix: str | None = None) -> Agent: ) +def _store_profile(suffix: str) -> OpenHandsAgentProfile: + get_llm_profile_store().save( + _LLM_PROFILE_REF, + LLM(model="gpt-4o", usage_id="agent", api_key=SecretStr("llm-key")), + include_secrets=True, + ) + profile = OpenHandsAgentProfile( + name="my-profile", + revision=5, + llm_profile_ref=_LLM_PROFILE_REF, + system_message_suffix=suffix, + tools=[], + ) + get_agent_profile_store().save(profile) + return profile + + def _mock_event_service(state: ConversationState) -> AsyncMock: event_service = AsyncMock(spec=EventService) event_service.get_state.return_value = state @@ -95,32 +121,19 @@ def test_launch_addition_uses_existing_acp_prompt_path(): @pytest.mark.parametrize("profile_launch", [False, True]) @pytest.mark.asyncio async def test_launch_additions_apply_after_agent_resolution(profile_launch, tmp_path): - profile_id = uuid4() - resolved_agent = _agent("PROFILE_BASELINE") - launched = LaunchedAgentProfile(agent_profile_id=profile_id, revision=5) additions = AgentLaunchAdditions( system_message_suffix_append=f" {_RUNTIME_SERVICES} ", ) - request = ( - StartConversationRequest( - agent_profile_id=profile_id, - workspace=LocalWorkspace(working_dir=str(tmp_path)), - agent_launch_additions=additions, - client_tools=[_CANVAS_UI], - ) - if profile_launch - else StartConversationRequest( - agent=resolved_agent, - workspace=LocalWorkspace(working_dir=str(tmp_path)), - agent_launch_additions=additions, - client_tools=[_CANVAS_UI], - ) - ) - state = ConversationState( - id=uuid4(), - agent=resolved_agent, - workspace=request.workspace, - execution_status=ConversationExecutionStatus.IDLE, + if profile_launch: + profile = _store_profile("PROFILE_BASELINE") + source: dict[str, Any] = {"agent_profile_id": profile.id} + else: + source = {"agent": _agent("PROFILE_BASELINE")} + request = StartConversationRequest( + **source, + workspace=LocalWorkspace(working_dir=str(tmp_path)), + agent_launch_additions=additions, + client_tools=[_CANVAS_UI], ) captured: dict[str, Any] = {} service = ConversationService(conversations_dir=tmp_path) @@ -129,13 +142,18 @@ async def test_launch_additions_apply_after_agent_resolution(profile_launch, tmp async def capture_start(stored, **kwargs): captured["stored"] = stored captured["agent"] = kwargs.get("agent") - return _mock_event_service(state) + return _mock_event_service( + ConversationState( + id=uuid4(), + agent=kwargs["agent"], + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) + ) with ( - patch( - "openhands.agent_server.conversation_service._resolve_agent_from_profile", - return_value=(resolved_agent, launched, None), - ) as resolve_profile, + patch(_DISCOVER_PATH, return_value=[]), + patch(_BROWSER_PROBE_PATH, return_value=False), patch.object( service, "_start_event_service", @@ -155,9 +173,8 @@ async def capture_start(stored, **kwargs): assert stored.client_tools == [_CANVAS_UI] assert stored.tool_module_qualnames == {} if profile_launch: - resolve_profile.assert_called_once() - else: - resolve_profile.assert_not_called() + assert stored.launched_agent_profile is not None + assert stored.launched_agent_profile.revision == 5 restored_agent = type(agent).model_validate(agent.model_dump(mode="json")) assert restored_agent.agent_context is not None @@ -167,3 +184,46 @@ async def capture_start(stored, **kwargs): assert [tool.name for tool in restored_agent.tools] == ["canvas_ui_client"] restored = StoredConversation.model_validate(stored.model_dump(mode="json")) assert restored.client_tools == [_CANVAS_UI] + + +@pytest.mark.asyncio +async def test_launch_additions_do_not_widen_a_profile_secret_scope(tmp_path): + """Additions carry deployment context, never a wider scope than the profile.""" + profile = _store_profile("PROFILE_BASELINE").model_copy(update={"secret_refs": []}) + get_agent_profile_store().save(profile) + request = StartConversationRequest( + agent_profile_id=profile.id, + workspace=LocalWorkspace(working_dir=str(tmp_path)), + agent_launch_additions=AgentLaunchAdditions( + system_message_suffix_append=_RUNTIME_SERVICES, + ), + secrets={"GITHUB_TOKEN": StaticSecret(value=SecretStr("gh"))}, + ) + captured: dict[str, Any] = {} + service = ConversationService(conversations_dir=tmp_path) + service._event_services = {} + + async def capture_start(stored, **kwargs): + captured["stored"] = stored + return _mock_event_service( + ConversationState( + id=uuid4(), + agent=kwargs["agent"], + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) + ) + + with ( + patch(_DISCOVER_PATH, return_value=[]), + patch(_BROWSER_PROBE_PATH, return_value=False), + patch.object( + service, + "_start_event_service", + new_callable=AsyncMock, + side_effect=capture_start, + ), + ): + await service.start_conversation(request) + + assert captured["stored"].secrets == {} diff --git a/tests/agent_server/test_agent_launch_parity.py b/tests/agent_server/test_agent_launch_parity.py new file mode 100644 index 0000000000..69d5e69697 --- /dev/null +++ b/tests/agent_server/test_agent_launch_parity.py @@ -0,0 +1,344 @@ +"""Every product launch path builds the same agent (#5141). + +The profile's *name* must not matter, the deprecated ``agent_settings`` payload +must take the same pipeline, and the ``materialize`` preview must agree with a +real launch field by field. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any +from uuid import UUID, uuid4 + +import pytest +from fastapi.testclient import TestClient +from pydantic import SecretStr + +from openhands.agent_server.agent_launch import launch_runtime, prepare_launch_request +from openhands.agent_server.api import create_app +from openhands.agent_server.config import Config +from openhands.agent_server.persistence import ( + PersistedSettings, + get_agent_profile_store, + get_llm_profile_store, + get_settings_store, +) +from openhands.sdk.agent import Agent +from openhands.sdk.context import AgentContext +from openhands.sdk.conversation.request import StartConversationRequest +from openhands.sdk.llm import LLM +from openhands.sdk.profiles import ( + AgentLaunchAdditions, + OpenHandsAgentProfile, + ProfileNotFound, + UnresolvedProfileReferences, +) +from openhands.sdk.secret import StaticSecret +from openhands.sdk.settings.model import ( + LLMSummarizingCondenserSettings, + OpenHandsAgentSettings, + validate_agent_settings, +) +from openhands.sdk.skills import Skill +from openhands.sdk.tool import Tool +from openhands.sdk.workspace import LocalWorkspace + + +_CATALOG = [Skill(name="kept", content="x"), Skill(name="denied", content="y")] + + +@pytest.fixture(autouse=True) +def stub_skill_discovery(monkeypatch): + """Pin the catalog: real discovery clones the public skills repository.""" + monkeypatch.setattr( + "openhands.agent_server.agent_launch.discover_profile_skills", + lambda: list(_CATALOG), + ) + monkeypatch.setattr( + "openhands.agent_server.agent_profiles_router.discover_profile_skills", + lambda: list(_CATALOG), + ) + + +@pytest.fixture +def config() -> Config: + return Config(static_files_path=None, session_api_keys=[], secret_key=None) + + +@pytest.fixture +def settings(config: Config) -> PersistedSettings: + stored = PersistedSettings( + agent_settings=validate_agent_settings( + { + "agent_kind": "openhands", + "llm": {"model": "gpt-4o", "usage_id": "agent"}, + "mcp_config": {"fetch": {"url": "https://fetch.test"}}, + } + ) + ) + get_settings_store(config).save(stored) + return stored + + +@pytest.fixture +def llm_profile() -> str: + get_llm_profile_store().save( + "primary", + LLM(model="gpt-4o", api_key=SecretStr("sk-primary"), usage_id="agent"), + include_secrets=True, + ) + get_llm_profile_store().save( + "alternate", + LLM(model="claude-opus-5", api_key=SecretStr("sk-alt"), usage_id="agent"), + include_secrets=True, + ) + return "primary" + + +def _profile(name: str, llm_profile_ref: str) -> OpenHandsAgentProfile: + return OpenHandsAgentProfile( + name=name, + llm_profile_ref=llm_profile_ref, + tools=[Tool(name="terminal")], + system_message_suffix="PROFILE_SUFFIX", + disabled_skills=["denied"], + enable_switch_llm_tool=False, + tool_concurrency_limit=3, + mcp_server_refs=[], + ) + + +def _launch( + profile_id: UUID | None, + settings: PersistedSettings, + *, + agent_settings: dict[str, Any] | None = None, + additions: AgentLaunchAdditions | None = None, +) -> tuple[StartConversationRequest, Any]: + request = StartConversationRequest( + agent_profile_id=profile_id, + agent_settings=agent_settings, + workspace=LocalWorkspace(working_dir="/tmp/parity"), + agent_launch_additions=additions, + ) + return prepare_launch_request( + request, + cipher=None, + settings=settings, + runtime=launch_runtime( + settings, acp_skill_sourcing="native", browser_available=True + ), + ) + + +def _agent_fingerprint(agent: Agent) -> dict[str, Any]: + """The agent fields a profile owns, with launch-time values normalized.""" + context = agent.agent_context + assert context is not None + return { + "llm": agent.llm.model_dump(mode="json", exclude={"stream"}), + "stream": agent.llm.stream, + "tools": [t.name for t in agent.tools], + "mcp_keys": sorted(agent.mcp_config), + "skills": sorted(s.name for s in context.skills), + "suffix": context.system_message_suffix, + "disabled_skills": context.disabled_skills, + "load_project_skills": context.load_project_skills, + "load_memory": context.load_memory, + "condenser": agent.condenser.model_dump(mode="json") + if agent.condenser + else None, + "critic": agent.critic.model_dump(mode="json") if agent.critic else None, + "concurrency": agent.tool_concurrency_limit, + "switch_llm": "switch_llm" in [t.name for t in agent.tools] + or "SwitchLLMTool" in agent.include_default_tools, + } + + +def test_a_profile_named_default_launches_like_any_other( + settings: PersistedSettings, llm_profile: str +) -> None: + store = get_agent_profile_store() + store.save(_profile("default", llm_profile)) + store.save(_profile("default-copy", llm_profile)) + + ids = {s["name"]: UUID(str(s["id"])) for s in store.list_summaries()} + _, default_plan = _launch(ids["default"], settings) + _, copy_plan = _launch(ids["default-copy"], settings) + + assert isinstance(default_plan.agent, Agent) + assert isinstance(copy_plan.agent, Agent) + assert _agent_fingerprint(default_plan.agent) == _agent_fingerprint(copy_plan.agent) + + +def test_agent_settings_launch_takes_the_profile_pipeline( + settings: PersistedSettings, llm_profile: str +) -> None: + """The deprecated payload becomes an inline profile, so the launch-time + fields (a fresh timestamp, forced streaming) come out the same.""" + payload = { + "agent_kind": "openhands", + "llm": {"model": "gpt-4o", "usage_id": "agent", "api_key": "sk-primary"}, + "tools": [{"name": "terminal"}], + "enable_switch_llm_tool": False, + "tool_concurrency_limit": 3, + # A client sends the whole stored condenser, as a stored profile carries + # it; an omitted key would instead inherit the LLM's token limit. + "condenser": LLMSummarizingCondenserSettings().model_dump(mode="json"), + "agent_context": AgentContext( + skills=[_CATALOG[0]], + disabled_skills=["denied"], + system_message_suffix="PROFILE_SUFFIX", + current_datetime="2020-01-01T00:00", + ).model_dump(mode="json"), + } + before = datetime.now().astimezone() + + _, plan = _launch(None, settings, agent_settings=payload) + + assert isinstance(plan.agent, Agent) + context = plan.agent.agent_context + assert context is not None + assert isinstance(context.current_datetime, datetime) + assert context.current_datetime >= before + assert plan.agent.llm.stream is True + assert plan.launched_profile is None + + store = get_agent_profile_store() + store.save(_profile("named", llm_profile)) + profile_id = UUID(str(store.list_summaries()[0]["id"])) + _, profile_plan = _launch(profile_id, settings) + assert isinstance(profile_plan.agent, Agent) + assert _agent_fingerprint(plan.agent) == _agent_fingerprint(profile_plan.agent) + + +def test_materialize_matches_a_real_launch( + config: Config, settings: PersistedSettings, llm_profile: str +) -> None: + store = get_agent_profile_store() + store.save(_profile("named", llm_profile)) + profile_id = UUID(str(store.list_summaries()[0]["id"])) + + client = TestClient(create_app(config)) + response = client.post("/api/agent-profiles/named/materialize") + assert response.status_code == 200 + body = response.json() + assert body["valid"] is True, body["errors"] + assert body["resolved_skills"] == ["kept"] + + _, plan = _launch(profile_id, settings) + assert isinstance(plan.settings, OpenHandsAgentSettings) + launched = plan.settings.model_dump(mode="json") + previewed = body["resolved_settings"] + + # Only the launch timestamp may differ. + for dump in (launched, previewed): + dump["agent_context"].pop("current_datetime") + assert previewed == launched + + +def test_inline_profile_draft_launches_and_is_marked_inline( + settings: PersistedSettings, llm_profile: str +) -> None: + draft = _profile("draft", llm_profile) + request = StartConversationRequest( + agent_profile=draft, + workspace=LocalWorkspace(working_dir="/tmp/parity"), + ) + _, plan = prepare_launch_request( + request, + cipher=None, + settings=settings, + runtime=launch_runtime( + settings, acp_skill_sourcing="native", browser_available=False + ), + ) + + assert isinstance(plan.agent, Agent) + assert plan.launched_profile is not None + assert plan.launched_profile.inline is True + assert plan.launched_profile.agent_profile_id == draft.id + # Nothing was stored. + assert get_agent_profile_store().list() == [] + + +def test_a_per_launch_llm_override_is_applied_and_recorded( + settings: PersistedSettings, llm_profile: str +) -> None: + store = get_agent_profile_store() + store.save(_profile("named", llm_profile)) + profile_id = UUID(str(store.list_summaries()[0]["id"])) + + _, plan = _launch( + profile_id, + settings, + additions=AgentLaunchAdditions(llm_profile_ref="alternate"), + ) + + assert isinstance(plan.agent, Agent) + assert plan.agent.llm.model == "claude-opus-5" + assert plan.launched_profile is not None + assert plan.launched_profile.llm_profile_ref == "alternate" + # The stored profile is untouched. + stored = store.load("named") + assert isinstance(stored, OpenHandsAgentProfile) + assert stored.llm_profile_ref == llm_profile + + +def test_secret_scope_is_enforced_on_the_request( + settings: PersistedSettings, llm_profile: str +) -> None: + store = get_agent_profile_store() + scoped = _profile("scoped", llm_profile).model_copy( + update={"secret_refs": ["ALLOWED"]} + ) + store.save(scoped) + request = StartConversationRequest( + agent_profile_id=scoped.id, + workspace=LocalWorkspace(working_dir="/tmp/parity"), + secrets={ + "ALLOWED": StaticSecret(value=SecretStr("a")), + "OTHER": StaticSecret(value=SecretStr("b")), + }, + ) + prepared, plan = prepare_launch_request( + request, + cipher=None, + settings=settings, + runtime=launch_runtime(settings, acp_skill_sourcing="native"), + ) + + assert set(prepared.secrets) == {"ALLOWED"} + assert plan.allowed_secrets == frozenset({"ALLOWED"}) + + +def test_a_dangling_reference_fails_the_launch_with_both_names( + settings: PersistedSettings, +) -> None: + broken = OpenHandsAgentProfile( + name="broken", llm_profile_ref="gone", mcp_server_refs=["nope"] + ) + get_agent_profile_store().save(broken) + + with pytest.raises(UnresolvedProfileReferences) as exc_info: + _launch(broken.id, settings) + + detail = exc_info.value.to_detail() + assert detail["code"] == "unresolved_profile_references" + assert detail["dangling_llm_profile_ref"] == "gone" + assert detail["dangling_mcp_server_refs"] == ["nope"] + + +def test_unknown_profile_id_fails_the_launch(settings: PersistedSettings) -> None: + with pytest.raises(ProfileNotFound): + _launch(uuid4(), settings) + + +def test_agent_settings_is_marked_deprecated_in_the_api_schema( + config: Config, +) -> None: + schema = create_app(config).openapi()["components"]["schemas"] + field = schema["StartConversationRequest"]["properties"]["agent_settings"] + assert field["deprecated"] is True + assert "agent_profile_id" in field["description"] diff --git a/tests/agent_server/test_agent_profile_conv_start.py b/tests/agent_server/test_agent_profile_conv_start.py index 3d99f5b8d8..4ed4f9cc20 100644 --- a/tests/agent_server/test_agent_profile_conv_start.py +++ b/tests/agent_server/test_agent_profile_conv_start.py @@ -1,7 +1,7 @@ -"""Tests for agent_profile_id at conversation start + LaunchedAgentProfile provenance. +"""Tests for launching a conversation from an Agent Profile. Covers: -- start-from-profile (OpenHands + ACP paths) +- the server launch pipeline (``agent_launch``) for the OpenHands + ACP paths - mutual-exclusivity validation (SDK layer) - unknown-id 404 / dangling-ref 422 (router layer) - LaunchedAgentProfile provenance round-trip through StoredConversation @@ -19,6 +19,10 @@ from fastapi.testclient import TestClient from pydantic import SecretStr, ValidationError +from openhands.agent_server.agent_launch import ( + load_stored_profile, + prepare_launch_request, +) from openhands.agent_server.config import Config from openhands.agent_server.conversation_router import conversation_router from openhands.agent_server.conversation_service import ConversationService @@ -30,26 +34,44 @@ StartConversationRequest, StoredConversation, ) -from openhands.agent_server.persistence import PersistedSettings +from openhands.agent_server.persistence import ( + PersistedSettings, + get_agent_profile_store, + get_llm_profile_store, +) from openhands.sdk import LLM, Agent, AgentBase, AgentContext from openhands.sdk.conversation.state import ( ConversationExecutionStatus, ConversationState, ) +from openhands.sdk.mcp.config import coerce_mcp_config +from openhands.sdk.profiles import ( + AgentLaunchPlan, + AgentLaunchRuntime, + ProfileNotFound, + UnresolvedProfileReferences, +) from openhands.sdk.profiles.agent_profile import ( ACPAgentProfile, OpenHandsAgentProfile, ) -from openhands.sdk.profiles.resolver import ( - DanglingMcpServerRef, - ProfileNotFound, -) from openhands.sdk.secret import StaticSecret from openhands.sdk.settings.model import ACPAgentSettings, OpenHandsAgentSettings from openhands.sdk.skills import Skill +from openhands.sdk.tool import BROWSER_TOOL_NAME, Tool from openhands.sdk.workspace import LocalWorkspace +# The launch pipeline binds these names in ``agent_launch``. +_DISCOVER_PATH = "openhands.agent_server.agent_launch.discover_profile_skills" +_BROWSER_PROBE_PATH = "openhands.agent_server.agent_launch.is_tool_usable" +# The settings read that feeds the launch runtime resolves through the +# package-level name ``conversation_service`` imports inside the function body. +_SETTINGS_STORE_PATH = "openhands.agent_server.persistence.get_settings_store" + +LLM_PROFILE_REF = "default" + + # --------------------------------------------------------------------------- # Fixtures # --------------------------------------------------------------------------- @@ -71,12 +93,18 @@ def mock_conversation_service(): return AsyncMock(spec=ConversationService) +def _store_llm_profile(name: str = LLM_PROFILE_REF, **updates) -> LLM: + llm = LLM(model="gpt-4o", usage_id="agent", api_key=SecretStr("llm-key"), **updates) + get_llm_profile_store().save(name, llm, include_secrets=True) + return llm + + def _make_openhands_profile(profile_id: UUID | None = None) -> OpenHandsAgentProfile: return OpenHandsAgentProfile( id=profile_id or uuid4(), name="my-profile", revision=3, - llm_profile_ref="default", + llm_profile_ref=LLM_PROFILE_REF, ) @@ -93,6 +121,49 @@ def _make_agent() -> Agent: return Agent(llm=LLM(model="gpt-4o", usage_id="llm"), tools=[]) +def _runtime(**updates) -> AgentLaunchRuntime: + """A pinned launch runtime: no host probing, streaming forced as a server does.""" + return AgentLaunchRuntime( + **{ + "browser_available": False, + "acp_skill_sourcing": "native", + "stream": True, + **updates, + } + ) + + +def _skill(name: str) -> Skill: + return Skill(name=name, content=f"{name} content") + + +def _launch( + profile: OpenHandsAgentProfile | ACPAgentProfile, + *, + runtime: AgentLaunchRuntime | None = None, + settings: PersistedSettings | None = None, + skills: list[Skill] | None = None, + secrets: dict[str, Any] | None = None, + discover: MagicMock | None = None, +) -> tuple[StartConversationRequest, AgentLaunchPlan]: + """Store ``profile`` and run the server launch pipeline against it.""" + _store_llm_profile() + get_agent_profile_store().save(profile) + request = StartConversationRequest( + agent_profile_id=profile.id, + workspace=LocalWorkspace(working_dir="/tmp"), + secrets=secrets or {}, + ) + discovery = discover or MagicMock(return_value=skills or []) + with patch(_DISCOVER_PATH, discovery): + return prepare_launch_request( + request, + cipher=None, + settings=settings or PersistedSettings(), + runtime=runtime or _runtime(), + ) + + # --------------------------------------------------------------------------- # SDK-layer: mutual exclusivity (StartConversationRequest) # --------------------------------------------------------------------------- @@ -115,6 +186,19 @@ def test_agent_alone_is_valid(self): assert req.agent is not None assert req.agent_profile_id is None + def test_an_explicit_null_source_is_not_a_source(self): + """Clients round-trip the whole family, sending null for the unused keys.""" + req = StartConversationRequest.model_validate( + { + "agent": None, + "agent_profile": None, + "agent_profile_id": str(uuid4()), + "workspace": {"working_dir": "/tmp"}, + } + ) + assert req.agent_profile_id is not None + assert req.agent is None + def test_agent_profile_id_and_agent_is_invalid(self): with pytest.raises(ValidationError, match="mutually exclusive"): StartConversationRequest( @@ -151,418 +235,212 @@ def test_agent_profile_id_present_in_request_payload(self): # --------------------------------------------------------------------------- -# Service-layer: _resolve_agent_from_profile helper +# Server launch pipeline: agent_launch # --------------------------------------------------------------------------- -# The helper does local imports inside the function body; patch at the source modules. -_STORE_PATH = "openhands.agent_server.persistence.store.get_agent_profile_store" -_LLM_STORE_PATH = "openhands.agent_server.persistence.store.get_llm_profile_store" -_RESOLVE_PATH = "openhands.sdk.profiles.resolver.resolve_agent_profile" -# 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" -# 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" - -class TestResolveAgentFromProfile: +class TestPrepareLaunchFromProfile: def test_unknown_id_raises_profile_not_found(self): - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - - with patch(_STORE_PATH) as MockStore: - MockStore.return_value.name_for_id.return_value = None - with pytest.raises(ProfileNotFound, match="not found"): - _resolve_agent_from_profile(uuid4(), cipher=None, mcp_config={}) + with pytest.raises(ProfileNotFound, match="not found"): + load_stored_profile(uuid4()) def test_openhands_profile_resolves_to_agent_and_stamps_launched(self): - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - - # OpenHands profiles always discover the catalog (deny-list needs the - # full set), threaded through to the resolver as available_skills. profile = _make_openhands_profile() - agent = _make_agent() - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch(_DISCOVER_PATH, return_value=[]) as MockDiscover, - # Pin the environment probe: tools=None profiles get browser - # 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", - return_value=False, - ), - ): - store_inst = MockStore.return_value - store_inst.name_for_id.return_value = profile.name - store_inst.load.return_value = profile + request, plan = _launch(profile) - mock_config = MagicMock() - mock_config.create_agent.return_value = agent - MockResolve.return_value = mock_config + assert isinstance(plan.agent, Agent) + assert plan.agent.llm.model == "gpt-4o" + assert request.agent is plan.agent + assert request.agent_profile_id is None + assert plan.launched_profile is not None + assert plan.launched_profile.agent_profile_id == profile.id + assert plan.launched_profile.revision == profile.revision + assert plan.launched_profile.inline is False + assert plan.launched_profile.llm_profile_ref is None - result_agent, launched, _ = _resolve_agent_from_profile( - profile.id, cipher=None, mcp_config={} - ) + def test_openhands_profile_keeps_the_catalog_minus_disabled_skills(self): + """The deny-list needs the whole catalog, not an allow-list (#4017).""" + profile = _make_openhands_profile().model_copy( + update={"disabled_skills": ["noisy"]} + ) + discover = MagicMock(return_value=[_skill("useful"), _skill("noisy")]) - assert result_agent is agent - assert launched.agent_profile_id == profile.id - assert launched.revision == profile.revision - # OpenHands discovery always runs; its result is threaded through. - MockDiscover.assert_called_once() - assert MockResolve.call_args.kwargs["available_skills"] == [] + _, plan = _launch(profile, skills=None, discover=discover) - def test_openhands_profile_forces_llm_stream_true(self): - """A profile-launched OpenHands conversation must guarantee on_token - wiring (#4014): unlike an inline agent_settings launch, a client can't - set llm.stream ahead of time on a profile's referenced LLM. This - agent-server layer forces it after resolution — not the SDK resolver, - which runs for every caller including headless/scripted ones.""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - from openhands.sdk.settings.model import OpenHandsAgentSettings + discover.assert_called_once() + assert plan.agent is not None + assert plan.agent.agent_context is not None + assert [s.name for s in plan.agent.agent_context.skills] == ["useful"] + def test_openhands_profile_forces_llm_stream_true(self): + """A client cannot set ``llm.stream`` on a profile's referenced LLM, so the + server forces it to guarantee on_token wiring (#4014). The stored LLM + profile is left alone.""" + _store_llm_profile(stream=False) profile = _make_openhands_profile() - # A real (unmocked) settings object so isinstance(...) narrows for real. - resolved_settings = OpenHandsAgentSettings( - llm=LLM(model="gpt-4o", usage_id="agent", stream=False) - ) - assert resolved_settings.llm.stream is False - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH, return_value=resolved_settings), - 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 + _, plan = _launch(profile, runtime=_runtime(stream=True)) - result_agent, _, _ = _resolve_agent_from_profile( - profile.id, cipher=None, mcp_config={} - ) + assert plan.agent is not None + assert plan.agent.llm.stream is True + assert get_llm_profile_store().load(LLM_PROFILE_REF).stream is False + + def test_openhands_profile_keeps_stream_off_without_the_runtime_flag(self): + _store_llm_profile(stream=False) + profile = _make_openhands_profile() - assert result_agent.llm.stream is True - # The original resolved settings object is untouched (model_copy, not - # a mutation), and the referenced LLM profile on disk is never - # rewritten — unlike a client-side self-heal that persists the flag. - assert resolved_settings.llm.stream is False + _, plan = _launch(profile, runtime=_runtime(stream=False)) - def test_acp_profile_does_not_force_llm_stream(self): - """The stream-forcing guarantee is OpenHands-only: ACP agents emit - their own message chunks through the ACP bridge without exposing an - LLM the same way (event_service.py's streaming_enabled already treats - every ACPAgent as streaming-capable regardless of llm.stream).""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) + assert plan.agent is not None + assert plan.agent.llm.stream is False + def test_acp_profile_does_not_force_llm_stream(self): + """ACP agents stream through the ACP bridge, not an LLM token callback.""" profile = _make_acp_profile() - agent = MagicMock() - - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - ): - 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 - _resolve_agent_from_profile(profile.id, cipher=None, mcp_config={}) + _, plan = _launch(profile, runtime=_runtime(stream=True)) - # No model_copy/mutation attempted on an ACP (non-OpenHandsAgentSettings) - # resolved settings object. - mock_config.model_copy.assert_not_called() + assert plan.agent is not None + assert plan.agent.llm.stream is False def test_acp_profile_skips_discovery_under_native_sourcing(self): - """A host-local ACP CLI reads the user's own skills from its home - directory, so the server injects none and skips discovery entirely - (#4019).""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - + """A host-local ACP CLI reads the user's own skills, so none are injected + and discovery never runs (#4019).""" profile = _make_acp_profile() + discover = MagicMock(return_value=[_skill("managed")]) - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch(_DISCOVER_PATH, return_value=[Skill(name="a", content="x")]) as Disc, - ): - store_inst = MockStore.return_value - store_inst.name_for_id.return_value = profile.name - store_inst.load.return_value = profile - MockResolve.return_value = MagicMock() - - _resolve_agent_from_profile( - profile.id, cipher=None, mcp_config={}, acp_skill_sourcing="native" - ) + _, plan = _launch( + profile, runtime=_runtime(acp_skill_sourcing="native"), discover=discover + ) - Disc.assert_not_called() - assert MockResolve.call_args.kwargs["available_skills"] is None + discover.assert_not_called() + assert plan.agent is not None + assert plan.agent.agent_context is not None + assert plan.agent.agent_context.skills == [] def test_acp_profile_gets_catalog_under_managed_sourcing(self): - """In a container the CLI has no host home to read skills from, so the - server supplies its discovered catalog instead (#4019).""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - + """In a container the CLI has no host home to read skills from.""" profile = _make_acp_profile() - catalog = [Skill(name="a", content="x")] + discover = MagicMock(return_value=[_skill("managed")]) - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch(_DISCOVER_PATH, return_value=catalog) as Disc, - ): - store_inst = MockStore.return_value - store_inst.name_for_id.return_value = profile.name - store_inst.load.return_value = profile - MockResolve.return_value = MagicMock() - - _resolve_agent_from_profile( - profile.id, - cipher=None, - mcp_config={}, - acp_skill_sourcing="openhands_managed", - ) + _, plan = _launch( + profile, + runtime=_runtime(acp_skill_sourcing="openhands_managed"), + discover=discover, + ) - Disc.assert_called_once() - assert MockResolve.call_args.kwargs["available_skills"] == catalog + discover.assert_called_once() + assert plan.agent is not None + assert plan.agent.agent_context is not None + assert [s.name for s in plan.agent.agent_context.skills] == ["managed"] 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).""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - + """The serving-layer counterpart of the SDK's deterministic default (#3978).""" 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( - "openhands.agent_server.conversation_service.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={} - ) + _, plan = _launch(profile, runtime=_runtime(browser_available=True)) - MockUsable.assert_called_once_with("browser_tool_set") - assert [tool.name for tool in result_agent.tools] == ["browser_tool_set"] + assert plan.agent is not None + names = [tool.name for tool in plan.agent.tools] + assert BROWSER_TOOL_NAME in names + assert "terminal" in names 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={} - ) + _, plan = _launch(profile, runtime=_runtime(browser_available=False)) - assert result_agent is agent + assert plan.agent is not None + names = [tool.name for tool in plan.agent.tools] + assert BROWSER_TOOL_NAME not in names + assert "terminal" in names 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.""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - + """An explicit profile tools list ([] included) is authoritative.""" profile = _make_openhands_profile().model_copy(update={"tools": []}) - 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=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={} - ) + _, plan = _launch(profile, runtime=_runtime(browser_available=True)) - MockUsable.assert_not_called() - assert result_agent is agent + assert plan.agent is not None + assert plan.agent.tools == [] - def test_acp_profile_never_gets_browser_injection(self): - """ACP agents own their tooling — the injection is OpenHands-only.""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, + def test_openhands_explicit_tool_list_is_used_verbatim(self): + profile = _make_openhands_profile().model_copy( + update={"tools": [Tool(name="terminal")]} ) - profile = _make_acp_profile() - agent = _make_agent() + _, plan = _launch(profile, runtime=_runtime(browser_available=True)) - 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=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={} - ) + assert plan.agent is not None + assert [tool.name for tool in plan.agent.tools] == ["terminal"] - MockUsable.assert_not_called() - assert result_agent is agent + def test_acp_profile_never_gets_browser_injection(self): + """ACP agents own their tooling — the injection is OpenHands-only.""" + profile = _make_acp_profile() - def test_openhands_default_profile_triggers_discovery(self): - """An OpenHands profile always discovers the skill catalog (the deny-list - needs the full set, minus disabled names). The default deny-list is [] - (all discovered); there is no discovery-skip path anymore (#4017).""" - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) + _, plan = _launch(profile, runtime=_runtime(browser_available=True)) - profile = _make_openhands_profile() - assert profile.disabled_skills == [] # the default: disable nothing - agent = _make_agent() + assert plan.agent is not None + assert plan.agent.tools == [] - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch(_DISCOVER_PATH, return_value=[]) as MockDiscover, - ): - 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 + def test_acp_profile_resolves_to_acp_agent(self): + from openhands.sdk.agent.acp_agent import ACPAgent + + profile = _make_acp_profile() - _resolve_agent_from_profile(profile.id, cipher=None, mcp_config={}) + _, plan = _launch(profile) - MockDiscover.assert_called_once() + assert isinstance(plan.agent, ACPAgent) + assert plan.agent.acp_server == "claude-code" + assert plan.launched_profile is not None + assert plan.launched_profile.agent_profile_id == profile.id + assert plan.launched_profile.revision == profile.revision - def test_dangling_mcp_server_ref_propagates(self): - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, + def test_dangling_mcp_server_ref_raises(self): + profile = _make_openhands_profile().model_copy( + update={"mcp_server_refs": ["missing-server"]} ) - profile = _make_openhands_profile() - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch(_DISCOVER_PATH, return_value=[]), - ): - store_inst = MockStore.return_value - store_inst.name_for_id.return_value = profile.name - store_inst.load.return_value = profile - MockResolve.side_effect = DanglingMcpServerRef(["missing-server"]) + with pytest.raises(UnresolvedProfileReferences) as exc_info: + _launch(profile) - with pytest.raises(DanglingMcpServerRef) as exc_info: - _resolve_agent_from_profile(profile.id, cipher=None, mcp_config={}) - assert "missing-server" in exc_info.value.missing + assert exc_info.value.mcp_server_refs == ["missing-server"] + assert exc_info.value.llm_profile_ref is None - def test_acp_profile_resolves_to_acp_agent(self): - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, + def test_resolved_mcp_config_is_filtered_to_the_refs(self): + profile = _make_openhands_profile().model_copy( + update={"mcp_server_refs": ["kept"]} + ) + settings = PersistedSettings( + agent_settings=OpenHandsAgentSettings( + mcp_config=coerce_mcp_config( + { + "mcpServers": { + "kept": {"command": "echo", "args": ["kept"]}, + "dropped": {"command": "echo", "args": ["dropped"]}, + } + } + ) + ) ) - from openhands.sdk.agent.acp_agent import ACPAgent - # ACP profiles carry no user/public skills, so discovery never runs and - # the resolver receives available_skills=None. - profile = _make_acp_profile() - acp_agent = MagicMock(spec=ACPAgent) + _, plan = _launch(profile, settings=settings) - with ( - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH) as MockResolve, - patch(_DISCOVER_PATH) as MockDiscover, - ): - 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 = acp_agent - MockResolve.return_value = mock_config - - result_agent, launched, _ = _resolve_agent_from_profile( - profile.id, cipher=None, mcp_config={} - ) + assert plan.agent is not None + assert list(plan.agent.mcp_config) == ["kept"] + + def test_missing_llm_profile_ref_raises_with_the_ref(self): + profile = _make_openhands_profile().model_copy( + update={"llm_profile_ref": "gone"} + ) - assert result_agent is acp_agent - assert launched.agent_profile_id == profile.id - assert launched.revision == profile.revision - MockDiscover.assert_not_called() - assert MockResolve.call_args.kwargs["available_skills"] is None + with pytest.raises(UnresolvedProfileReferences) as exc_info: + _launch(profile) + + assert exc_info.value.llm_profile_ref == "gone" + assert exc_info.value.mcp_server_refs == [] # --------------------------------------------------------------------------- @@ -570,40 +448,40 @@ def test_acp_profile_resolves_to_acp_agent(self): # --------------------------------------------------------------------------- -def _resolved_settings_for( - agent_kind: str, -) -> tuple[ - OpenHandsAgentProfile | ACPAgentProfile, OpenHandsAgentSettings | ACPAgentSettings -]: - """A profile plus the settings the SDK resolver builds from it. +def _profile_for(agent_kind: str) -> OpenHandsAgentProfile | ACPAgentProfile: + return _make_acp_profile() if agent_kind == "acp" else _make_openhands_profile() - Mirrors ``profiles/resolver.py``: both variants always get an - ``AgentContext``, and neither ``AgentProfile`` variant has a field that - could carry the user's memory preference. - """ - if agent_kind == "acp": - return _make_acp_profile(), ACPAgentSettings( - acp_command=["echo", "acp"], - agent_context=AgentContext(skills=[], current_datetime=None), - ) - return _make_openhands_profile(), OpenHandsAgentSettings( - llm=LLM(model="gpt-4o", usage_id="agent"), - agent_context=AgentContext(skills=[]), + +def _mock_event_service(state: ConversationState) -> AsyncMock: + event_service = AsyncMock(spec=EventService) + event_service.get_state.return_value = state + event_service.stored = MagicMock( + launched_agent_profile=None, + client_tools=[], + title=None, + metrics=None, + created_at=datetime.now(UTC), + updated_at=datetime.now(UTC), + forked_from_conversation_id=None, + forked_from_event_id=None, + parent_conversation_id=None, ) + return event_service async def _start_from_profile( tmp_path, profile: OpenHandsAgentProfile | ACPAgentProfile, - resolved_settings: OpenHandsAgentSettings | ACPAgentSettings, persisted_settings: PersistedSettings, ) -> tuple[StoredConversation, Any]: """Launch from ``profile`` and return the captured ``(StoredConversation, agent)``. - Only the stores are stubbed, so ``_resolve_agent_from_profile`` and the - settings read that feeds it both run for real — this is the path a client - reaches by sending ``agent_profile_id`` alone, with no ``agent_settings``. + Only the settings store, skill discovery and the browser probe are stubbed, + so the launch pipeline runs for real against the profile stores — this is + the path a client reaches by sending ``agent_profile_id`` alone. """ + _store_llm_profile() + get_agent_profile_store().save(profile) request = StartConversationRequest( agent_profile_id=profile.id, workspace=LocalWorkspace(working_dir=str(tmp_path)), @@ -614,41 +492,24 @@ async def capture_start(stored, **kwargs): agent = kwargs["agent"] captured["stored"] = stored captured["agent"] = agent - event_service = AsyncMock(spec=EventService) - event_service.get_state.return_value = ConversationState( - id=uuid4(), - agent=agent, - workspace=request.workspace, - execution_status=ConversationExecutionStatus.IDLE, - ) - event_service.stored = MagicMock( - launched_agent_profile=None, - client_tools=[], - title=None, - metrics=None, - created_at=datetime.now(UTC), - updated_at=datetime.now(UTC), - forked_from_conversation_id=None, - forked_from_event_id=None, - parent_conversation_id=None, + return _mock_event_service( + ConversationState( + id=uuid4(), + agent=agent, + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) ) - return event_service service = ConversationService(conversations_dir=tmp_path) service._event_services = {} with ( patch(_SETTINGS_STORE_PATH) as MockSettingsStore, - patch(_STORE_PATH) as MockStore, - patch(_LLM_STORE_PATH), - patch(_RESOLVE_PATH, return_value=resolved_settings), patch(_DISCOVER_PATH, return_value=[]), # 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", - return_value=False, - ), + patch(_BROWSER_PROBE_PATH, return_value=False), patch.object( service, "_start_event_service", @@ -657,8 +518,6 @@ async def capture_start(stored, **kwargs): ), ): MockSettingsStore.return_value.load.return_value = persisted_settings - MockStore.return_value.name_for_id.return_value = profile.name - MockStore.return_value.load.return_value = profile await service.start_conversation(request) return captured["stored"], captured["agent"] @@ -673,13 +532,14 @@ async def _start_with_agent( ) -> Any: """Launch via a concrete ``agent`` or a raw ``agent_settings`` payload and return the captured agent. - - Neither shape touches profile resolution, so unlike ``_start_from_profile`` - only the settings-store read that feeds ``load_memory`` needs stubbing. """ + source: dict[str, Any] = ( + {"agent": cast(AgentBase, agent)} + if agent is not None + else {"agent_settings": agent_settings} + ) request = StartConversationRequest( - agent=cast(AgentBase, agent), - agent_settings=agent_settings, + **source, workspace=LocalWorkspace(working_dir=str(tmp_path)), ) captured: dict[str, Any] = {} @@ -687,31 +547,21 @@ async def _start_with_agent( async def capture_start(stored, **kwargs): launched_agent = kwargs["agent"] captured["agent"] = launched_agent - event_service = AsyncMock(spec=EventService) - event_service.get_state.return_value = ConversationState( - id=uuid4(), - agent=launched_agent, - workspace=request.workspace, - execution_status=ConversationExecutionStatus.IDLE, - ) - event_service.stored = MagicMock( - launched_agent_profile=None, - client_tools=[], - title=None, - metrics=None, - created_at=datetime.now(UTC), - updated_at=datetime.now(UTC), - forked_from_conversation_id=None, - forked_from_event_id=None, - parent_conversation_id=None, + return _mock_event_service( + ConversationState( + id=uuid4(), + agent=launched_agent, + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) ) - return event_service service = ConversationService(conversations_dir=tmp_path) service._event_services = {} with ( patch(_SETTINGS_STORE_PATH) as MockSettingsStore, + patch(_BROWSER_PROBE_PATH, return_value=False), patch.object( service, "_start_event_service", @@ -731,65 +581,19 @@ async def test_start_from_profile_stamps_launched_agent_profile_on_stored( self, tmp_path ): """_start_conversation passes launched_agent_profile to StoredConversation.""" - profile_id = uuid4() - agent = _make_agent() - launched_agent_profile = LaunchedAgentProfile( - agent_profile_id=profile_id, revision=5 - ) - request = StartConversationRequest( - agent_profile_id=profile_id, - workspace=LocalWorkspace(working_dir=str(tmp_path)), - ) + profile = _make_openhands_profile() - captured: dict[str, Any] = {} - mock_state = ConversationState( - id=uuid4(), - agent=agent, - workspace=request.workspace, - execution_status=ConversationExecutionStatus.IDLE, + stored, agent = await _start_from_profile( + tmp_path, profile, PersistedSettings() ) - with patch( - "openhands.agent_server.conversation_service._resolve_agent_from_profile", - return_value=(agent, launched_agent_profile, None), - ): - service = ConversationService(conversations_dir=tmp_path) - service._event_services = {} - - with patch.object( - service, "_start_event_service", new_callable=AsyncMock - ) as mock_ses: - mock_es = AsyncMock(spec=EventService) - mock_es.get_state.return_value = mock_state - mock_es.stored = MagicMock( - launched_agent_profile=launched_agent_profile, - client_tools=[], - title=None, - metrics=None, - created_at=datetime.now(UTC), - updated_at=datetime.now(UTC), - forked_from_conversation_id=None, - forked_from_event_id=None, - parent_conversation_id=None, - ) - - async def capture_start(stored, **kwargs): - captured["stored"] = stored - captured["agent"] = kwargs.get("agent") - return mock_es - - mock_ses.side_effect = capture_start - - info, is_new = await service.start_conversation(request) - - stored = captured.get("stored") - assert stored is not None, "StoredConversation was not captured" assert stored.launched_agent_profile is not None - assert stored.launched_agent_profile.agent_profile_id == profile_id - assert stored.launched_agent_profile.revision == 5 + assert stored.launched_agent_profile.agent_profile_id == profile.id + assert stored.launched_agent_profile.revision == profile.revision # The resolved agent (not None) must be passed to _start_event_service # (it is persisted to base_state.json, not meta.json). - assert captured["agent"] is not None + assert agent is not None + assert stored.model_dump(mode="json").get("agent_profile_id") is None @pytest.mark.asyncio async def test_profile_not_found_propagates(self, tmp_path): @@ -797,34 +601,34 @@ async def test_profile_not_found_propagates(self, tmp_path): agent_profile_id=uuid4(), workspace=LocalWorkspace(working_dir=str(tmp_path)), ) + service = ConversationService(conversations_dir=tmp_path) + service._event_services = {} - with patch( - "openhands.agent_server.conversation_service._resolve_agent_from_profile", - side_effect=ProfileNotFound("profile not found"), - ): - service = ConversationService(conversations_dir=tmp_path) - service._event_services = {} - - with pytest.raises(ProfileNotFound): - await service.start_conversation(request) + with pytest.raises(ProfileNotFound): + await service.start_conversation(request) @pytest.mark.asyncio async def test_dangling_ref_propagates_from_service(self, tmp_path): + _store_llm_profile() + profile = _make_openhands_profile().model_copy( + update={"mcp_server_refs": ["mcp-server-x"]} + ) + get_agent_profile_store().save(profile) request = StartConversationRequest( - agent_profile_id=uuid4(), + agent_profile_id=profile.id, workspace=LocalWorkspace(working_dir=str(tmp_path)), ) + service = ConversationService(conversations_dir=tmp_path) + service._event_services = {} - with patch( - "openhands.agent_server.conversation_service._resolve_agent_from_profile", - side_effect=DanglingMcpServerRef(["mcp-server-x"]), + with ( + patch(_DISCOVER_PATH, return_value=[]), + patch(_BROWSER_PROBE_PATH, return_value=False), ): - service = ConversationService(conversations_dir=tmp_path) - service._event_services = {} - - with pytest.raises(DanglingMcpServerRef) as exc_info: + with pytest.raises(UnresolvedProfileReferences) as exc_info: await service.start_conversation(request) - assert "mcp-server-x" in exc_info.value.missing + + assert "mcp-server-x" in exc_info.value.mcp_server_refs @pytest.mark.parametrize("agent_kind", ["openhands", "acp"]) @pytest.mark.asyncio @@ -837,15 +641,14 @@ async def test_profile_launch_inherits_the_stored_memory_preference( Settings → Memory toggle would read as enabled while every conversation started from a named OpenHands profile or an ACP profile ignored it. """ - profile, resolved_settings = _resolved_settings_for(agent_kind) persisted = PersistedSettings( agent_settings=OpenHandsAgentSettings( agent_context=AgentContext(load_memory=True) ) ) - stored, agent = await _start_from_profile( - tmp_path, profile, resolved_settings, persisted + _, agent = await _start_from_profile( + tmp_path, _profile_for(agent_kind), persisted ) assert agent.agent_context is not None @@ -873,10 +676,8 @@ async def test_profile_launch_inherits_the_stored_memory_preference( async def test_profile_launch_leaves_memory_off_without_the_preference( self, tmp_path, persisted_settings ): - profile, resolved_settings = _resolved_settings_for("openhands") - - stored, agent = await _start_from_profile( - tmp_path, profile, resolved_settings, persisted_settings + _, agent = await _start_from_profile( + tmp_path, _make_openhands_profile(), persisted_settings ) assert agent.agent_context is not None @@ -888,12 +689,7 @@ class TestConversationServiceStartWithDirectAgent: async def test_direct_agent_launch_inherits_the_stored_memory_preference( self, tmp_path ): - """Same guarantee as the profile launch, for the ``agent`` shape. - - ``request.agent`` is already set when a client sends ``agent`` - directly, so this path never touched ``_resolve_agent_from_profile``'s - stamp and silently dropped the global preference before this fix. - """ + """Same guarantee as the profile launch, for the ``agent`` shape.""" persisted = PersistedSettings( agent_settings=OpenHandsAgentSettings( agent_context=AgentContext(load_memory=True) @@ -942,11 +738,8 @@ async def test_direct_agent_launch_leaves_memory_off_without_the_preference( async def test_agent_settings_launch_inherits_the_stored_memory_preference( self, tmp_path ): - """Locks in the third shape: ``_populate_agent_from_settings`` converts - this to ``request.agent`` before ``_start_conversation`` even runs, so - it needs the same coverage as the ``agent`` shape above, not just the - two paths that were already tested pre-fix. - """ + """The deprecated ``agent_settings`` shape needs the same coverage as the + two shapes that carry an agent or a profile reference.""" persisted = PersistedSettings( agent_settings=OpenHandsAgentSettings( agent_context=AgentContext(load_memory=True) @@ -1071,8 +864,10 @@ def test_profile_not_found_returns_404(self, client, mock_conversation_service): def test_dangling_mcp_server_ref_returns_422( self, client, mock_conversation_service ): - mock_conversation_service.start_conversation.side_effect = DanglingMcpServerRef( - ["missing-server", "another-missing"] + mock_conversation_service.start_conversation.side_effect = ( + UnresolvedProfileReferences( + mcp_server_refs=["missing-server", "another-missing"] + ) ) client.app.dependency_overrides[get_conversation_service] = lambda: ( mock_conversation_service @@ -1085,8 +880,31 @@ def test_dangling_mcp_server_ref_returns_422( resp = client.post("/api/conversations", json=payload) assert resp.status_code == 422 detail = resp.json().get("detail", {}) - assert "dangling_mcp_server_refs" in detail + assert detail["code"] == "unresolved_profile_references" assert "missing-server" in detail["dangling_mcp_server_refs"] + assert detail["dangling_llm_profile_ref"] is None + + def test_dangling_llm_profile_ref_returns_422( + self, client, mock_conversation_service + ): + """A launch never silently falls back to a different LLM, so the client + gets a structured error it can turn into a fix-it prompt.""" + mock_conversation_service.start_conversation.side_effect = ( + UnresolvedProfileReferences(llm_profile_ref="gone") + ) + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service + ) + + payload = { + "agent_profile_id": str(uuid4()), + "workspace": {"working_dir": "/tmp/test", "kind": "LocalWorkspace"}, + } + resp = client.post("/api/conversations", json=payload) + assert resp.status_code == 422 + detail = resp.json().get("detail", {}) + assert detail["dangling_llm_profile_ref"] == "gone" + assert detail["dangling_mcp_server_refs"] == [] # No dangling-skill 422: skills use a deny-list (disabled_skills) that can't # dangle — a disabled name absent from the catalog is a no-op, so a profile @@ -1126,29 +944,34 @@ def test_stored_conversation_without_profile_has_none(self): ) assert stored.launched_agent_profile is None - def test_agent_profile_id_excluded_from_stored_conversation_persistence(self): - """Regression: agent_profile_id must NOT appear in StoredConversation payload. + def test_agent_sources_excluded_from_stored_conversation_persistence(self): + """Regression: no agent source may appear in the StoredConversation payload. - StartConversationRequest.model_dump() includes agent_profile_id for HTTP - transport. _start_conversation excludes it before building StoredConversation - (the field is resolved into launched_agent_profile); this test verifies that a - StoredConversation built from a resolved request contains neither the raw - profile UUID nor re-exposes it. + StartConversationRequest.model_dump() includes the launch inputs for HTTP + transport. _start_conversation excludes them before building + StoredConversation (they are resolved into the agent and + launched_agent_profile). """ profile_id = uuid4() - # Simulate the resolved state: agent is set, agent_profile_id excluded. request = StartConversationRequest( agent_profile_id=profile_id, workspace=LocalWorkspace(working_dir="/tmp"), ) - # Mirror what _start_conversation does: exclude agent_profile_id from - # the persistence payload before constructing StoredConversation. - request_data = request.model_dump(mode="json", exclude={"agent_profile_id"}) - agent = _make_agent() - request_data["agent"] = agent.model_dump(mode="json") + request_data = request.model_dump( + mode="json", + exclude={ + "agent_profile_id", + "agent_profile", + "agent_settings", + "agent_launch_additions", + }, + ) + request_data["agent"] = _make_agent().model_dump(mode="json") stored = StoredConversation(id=uuid4(), **request_data) dumped = stored.model_dump(mode="json") assert "agent_profile_id" not in dumped + assert "agent_profile" not in dumped + assert "agent_settings" not in dumped def test_launched_agent_profile_in_conversation_info(self): profile_id = uuid4() @@ -1200,46 +1023,24 @@ def test_launched_agent_profile_survives_json_serialization(self, tmp_path): class TestProfileSecretScope: """``secret_refs`` narrows a launch's secrets, enforced server-side (#17236).""" - def _resolve(self, profile): - from openhands.agent_server.conversation_service import ( - _resolve_agent_from_profile, - ) - - 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=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 = _make_agent() - MockResolve.return_value = mock_config - _, _, allowed = _resolve_agent_from_profile( - profile.id, cipher=None, mcp_config={} - ) - return allowed + def _allowed_secrets(self, profile) -> frozenset[str] | None: + _, plan = _launch(profile) + return plan.allowed_secrets def test_an_unscoped_profile_reports_no_restriction(self): - assert self._resolve(_make_openhands_profile()) is None + assert self._allowed_secrets(_make_openhands_profile()) is None def test_a_scoped_profile_reports_its_allow_list(self): - profile = _make_openhands_profile() - profile = profile.model_copy(update={"secret_refs": ["GITHUB_TOKEN"]}) - assert self._resolve(profile) == {"GITHUB_TOKEN"} + profile = _make_openhands_profile().model_copy( + update={"secret_refs": ["GITHUB_TOKEN"]} + ) + assert self._allowed_secrets(profile) == {"GITHUB_TOKEN"} def test_a_scoped_acp_profile_gets_no_implicit_provider_credentials(self): # Strict: an ACP profile must list its own credential to receive it. profile = _make_acp_profile().model_copy(update={"secret_refs": []}) - assert self._resolve(profile) == set() + assert self._allowed_secrets(profile) == set() - @pytest.mark.asyncio @pytest.mark.parametrize( ("secret_refs", "expected"), [ @@ -1250,15 +1051,29 @@ def test_a_scoped_acp_profile_gets_no_implicit_provider_credentials(self): (["MISSING"], set()), ], ) - async def test_start_conversation_drops_secrets_the_profile_disallows( - self, tmp_path, secret_refs, expected - ): + def test_launch_drops_secrets_the_profile_disallows(self, secret_refs, expected): """The filter runs on the request, so a client cannot widen the scope.""" profile = _make_openhands_profile().model_copy( update={"secret_refs": secret_refs} ) - captured: dict[str, Any] = {} + request, _ = _launch( + profile, + secrets={ + "GITHUB_TOKEN": StaticSecret(value=SecretStr("gh")), + "DATADOG_API_KEY": StaticSecret(value=SecretStr("dd")), + }, + ) + + assert set(request.secrets) == expected + + @pytest.mark.asyncio + async def test_start_conversation_persists_only_the_allowed_secrets(self, tmp_path): + profile = _make_openhands_profile().model_copy( + update={"secret_refs": ["GITHUB_TOKEN"]} + ) + _store_llm_profile() + get_agent_profile_store().save(profile) request = StartConversationRequest( agent_profile_id=profile.id, workspace=LocalWorkspace(working_dir=str(tmp_path)), @@ -1267,56 +1082,32 @@ async def test_start_conversation_drops_secrets_the_profile_disallows( "DATADOG_API_KEY": StaticSecret(value=SecretStr("dd")), }, ) + captured: dict[str, Any] = {} + + async def capture(stored, **kwargs): + captured["secrets"] = dict(stored.secrets) + return _mock_event_service( + ConversationState( + id=uuid4(), + agent=kwargs["agent"], + workspace=request.workspace, + execution_status=ConversationExecutionStatus.IDLE, + ) + ) async with ConversationService( conversations_dir=tmp_path / "conversations" ) as service: 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=False, - ), + patch(_BROWSER_PROBE_PATH, return_value=False), patch.object( - service, "_start_event_service", new_callable=AsyncMock - ) as mock_ses, + service, + "_start_event_service", + new_callable=AsyncMock, + side_effect=capture, + ), ): - store_inst = MockStore.return_value - store_inst.name_for_id.return_value = profile.name - store_inst.load.return_value = profile - agent = _make_agent() - mock_config = MagicMock() - mock_config.create_agent.return_value = agent - MockResolve.return_value = mock_config - - mock_es = AsyncMock(spec=EventService) - mock_es.get_state.return_value = ConversationState( - id=uuid4(), - agent=agent, - workspace=request.workspace, - execution_status=ConversationExecutionStatus.IDLE, - ) - mock_es.stored = MagicMock( - launched_agent_profile=None, - client_tools=[], - title=None, - metrics=None, - created_at=datetime.now(UTC), - updated_at=datetime.now(UTC), - forked_from_conversation_id=None, - forked_from_event_id=None, - parent_conversation_id=None, - ) - - async def capture(stored, **kwargs): - captured["secrets"] = dict(stored.secrets) - return mock_es - - mock_ses.side_effect = capture - await service.start_conversation(request) - assert set(captured["secrets"]) == expected + assert set(captured["secrets"]) == {"GITHUB_TOKEN"} diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index 61fbfc69f1..b9077668ad 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -34,8 +34,10 @@ PluginResolutionError, ) from openhands.sdk.plugin import PluginFetchError +from openhands.sdk.profiles import AgentLaunchError from openhands.sdk.security.llm_analyzer import LLMSecurityAnalyzer from openhands.sdk.settings import AGENT_SETTINGS_SCHEMA_VERSION +from openhands.sdk.settings.model import validate_agent_settings from openhands.sdk.workspace import LocalWorkspace @@ -644,9 +646,12 @@ def test_start_conversation_accepts_openhands_agent_settings( assert response.status_code == 201 request = mock_conversation_service.start_conversation.call_args.args[0] - assert request.agent.kind == "Agent" - assert request.agent.llm.model == "settings-model" - assert "agent_settings" not in request.model_dump(mode="json") + # The agent is built by the launch pipeline, not at the request edge. + assert request.agent is None + assert request.agent_settings is not None + assert request.agent_settings["llm"]["model"] == "settings-model" + dumped = request.model_dump(mode="json") + assert dumped["agent_settings"]["llm"]["model"] == "settings-model" finally: client.app.dependency_overrides.clear() @@ -698,8 +703,9 @@ 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 - assert {tool.name for tool in request.agent.tools} == { + agent = validate_agent_settings(request.agent_settings).create_agent() + assert "SwitchLLMTool" in agent.include_default_tools + assert {tool.name for tool in agent.tools} == { "terminal", "file_editor", "task_tracker", @@ -782,12 +788,13 @@ def test_start_conversation_accepts_acp_agent_settings( assert response.status_code == 201 request = mock_conversation_service.start_conversation.call_args.args[0] - assert request.agent.kind == "ACPAgent" - assert request.agent.acp_command == ["echo", "settings"] - assert request.agent.acp_args == ["--verbose"] - assert request.agent.acp_model == "acp-test-model" - assert request.agent.acp_session_mode == "bypassPermissions" - assert request.agent.acp_prompt_timeout == 123.0 + agent = validate_agent_settings(request.agent_settings).create_agent() + assert agent.kind == "ACPAgent" + assert agent.acp_command == ["echo", "settings"] + assert agent.acp_args == ["--verbose"] + assert agent.acp_model == "acp-test-model" + assert agent.acp_session_mode == "bypassPermissions" + assert agent.acp_prompt_timeout == 123.0 finally: client.app.dependency_overrides.clear() @@ -796,8 +803,9 @@ def test_start_conversation_accepts_acp_agent_settings( @pytest.mark.parametrize( "agent_settings", [ - {"agent_kind": "invalid"}, "not-a-settings-object", + ["agent_kind"], + 7, ], ) def test_start_conversation_rejects_invalid_agent_settings( @@ -822,6 +830,34 @@ def test_start_conversation_rejects_invalid_agent_settings( client.app.dependency_overrides.clear() +def test_start_conversation_defers_agent_settings_validation( + client, mock_conversation_service +): + """A well-formed payload the launch cannot resolve is a 422 from the service.""" + mock_conversation_service.start_conversation.side_effect = AgentLaunchError( + "Invalid agent_settings: unknown agent_kind" + ) + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service + ) + + try: + response = client.post( + "/api/conversations", + json={ + "agent_settings": {"agent_kind": "invalid"}, + "workspace": {"working_dir": "/tmp/test"}, + }, + ) + + assert response.status_code == 422 + assert response.json()["detail"]["code"] == "invalid_agent_launch" + request = mock_conversation_service.start_conversation.call_args.args[0] + assert request.agent_settings == {"agent_kind": "invalid"} + finally: + client.app.dependency_overrides.clear() + + def test_start_conversation_agent_takes_precedence_over_agent_settings( client, mock_conversation_service ): diff --git a/tests/sdk/profiles/test_launch.py b/tests/sdk/profiles/test_launch.py new file mode 100644 index 0000000000..2df1f6f860 --- /dev/null +++ b/tests/sdk/profiles/test_launch.py @@ -0,0 +1,496 @@ +"""Tests for ``prepare_agent_launch`` — the one function that builds a launch. + +Covers the runtime-dependent pieces (browser, streaming, ACP skill sourcing, +memory), per-launch additions, provenance, the structured dangling-reference +error, and the deprecated ``agent_settings`` launch, which becomes an inline +profile resolved by the same function (#5141). +""" + +from datetime import datetime +from pathlib import Path + +import pytest +from pydantic import SecretStr + +from openhands.sdk.agent import ACPAgent, Agent +from openhands.sdk.context import AgentContext +from openhands.sdk.llm import LLM +from openhands.sdk.llm.llm_profile_store import LLMProfileStore +from openhands.sdk.mcp.config import MCPServer, coerce_mcp_config +from openhands.sdk.profiles import ( + ACPAgentProfile, + AgentLaunchAdditions, + AgentLaunchCatalog, + AgentLaunchError, + AgentLaunchRuntime, + OpenHandsAgentProfile, + UnresolvedProfileReferences, + agent_settings_launch_source, + prepare_agent_launch, +) +from openhands.sdk.secret import StaticSecret +from openhands.sdk.settings.model import ( + ACPAgentSettings, + OpenHandsAgentSettings, + validate_agent_settings, +) +from openhands.sdk.skills import Skill +from openhands.sdk.tool import Tool + + +_LLM_SECRET = "sk-LLM-SECRET-SHOULD-NOT-LEAK" +_CRITIC_SECRET = "sk-CRITIC-SECRET" +_SUFFIX_APPEND = ( + "\n* Automation: http://localhost:18001\n" # noqa: E501 +) + + +@pytest.fixture +def llm_store(tmp_path: Path) -> LLMProfileStore: + store = LLMProfileStore(base_dir=tmp_path / "llm") + store.save( + "default", + LLM(model="gpt-4o", api_key=SecretStr(_LLM_SECRET), usage_id="x"), + include_secrets=True, + ) + store.save( + "picked", + LLM(model="claude-opus-5", api_key=SecretStr(_LLM_SECRET), usage_id="x"), + include_secrets=True, + ) + return store + + +@pytest.fixture +def mcp_config() -> dict[str, MCPServer]: + return coerce_mcp_config( + {"mcpServers": {"fetch": {"url": "https://fetch.test"}}}, + ) + + +def _catalog( + llm_store: LLMProfileStore, + *, + mcp_config: dict[str, MCPServer] | None = None, + skills: list[Skill] | None = None, +) -> AgentLaunchCatalog: + return AgentLaunchCatalog( + llm_store=llm_store, + mcp_config=mcp_config if mcp_config is not None else {}, + skills=skills, + ) + + +def _openhands_profile(**kwargs) -> OpenHandsAgentProfile: + return OpenHandsAgentProfile(name="oh", llm_profile_ref="default", **kwargs) + + +# --------------------------------------------------------------------------- # +# Runtime-dependent pieces +# --------------------------------------------------------------------------- # + + +def test_default_tools_get_browser_only_when_the_runtime_has_it( + llm_store: LLMProfileStore, +) -> None: + profile = _openhands_profile() + assert profile.tools is None + + with_browser = prepare_agent_launch( + profile, + catalog=_catalog(llm_store), + runtime=AgentLaunchRuntime(browser_available=True), + ) + without = prepare_agent_launch( + profile, + catalog=_catalog(llm_store), + runtime=AgentLaunchRuntime(browser_available=False), + ) + + assert with_browser.agent is not None and without.agent is not None + assert "browser_tool_set" in [t.name for t in with_browser.agent.tools] + assert "browser_tool_set" not in [t.name for t in without.agent.tools] + # The resolved view materializes the tool list, so a preview can show it. + assert isinstance(with_browser.settings, OpenHandsAgentSettings) + assert with_browser.settings.tools is not None + + +def test_explicit_tools_are_never_amended(llm_store: LLMProfileStore) -> None: + profile = _openhands_profile(tools=[Tool(name="terminal")]) + plan = prepare_agent_launch( + profile, + catalog=_catalog(llm_store), + runtime=AgentLaunchRuntime(browser_available=True), + ) + assert plan.agent is not None + assert [t.name for t in plan.agent.tools] == ["terminal"] + + +def test_streaming_is_forced_for_openhands_profiles( + llm_store: LLMProfileStore, +) -> None: + plan = prepare_agent_launch( + _openhands_profile(), + catalog=_catalog(llm_store), + runtime=AgentLaunchRuntime(stream=True), + ) + assert isinstance(plan.agent, Agent) + assert plan.agent.llm.stream is True + + +def test_current_datetime_is_computed_at_launch(llm_store: LLMProfileStore) -> None: + before = datetime.now().astimezone() + plan = prepare_agent_launch(_openhands_profile(), catalog=_catalog(llm_store)) + assert isinstance(plan.settings, OpenHandsAgentSettings) + stamped = plan.settings.agent_context.current_datetime + assert isinstance(stamped, datetime) + assert stamped >= before + + +def test_acp_launch_keeps_no_timestamp(llm_store: LLMProfileStore) -> None: + plan = prepare_agent_launch( + ACPAgentProfile(name="acp", acp_server="claude-code"), + catalog=_catalog(llm_store), + ) + assert isinstance(plan.settings, ACPAgentSettings) + assert plan.settings.agent_context is not None + assert plan.settings.agent_context.current_datetime is None + + +def test_acp_profile_gets_the_catalog_only_under_managed_sourcing( + llm_store: LLMProfileStore, +) -> None: + profile = ACPAgentProfile(name="acp", acp_server="claude-code") + catalog_skills = [Skill(name="a", content="x")] + + native = prepare_agent_launch( + profile, + catalog=_catalog(llm_store, skills=catalog_skills), + runtime=AgentLaunchRuntime(acp_skill_sourcing="native"), + ) + managed = prepare_agent_launch( + profile, + catalog=_catalog(llm_store, skills=catalog_skills), + runtime=AgentLaunchRuntime(acp_skill_sourcing="openhands_managed"), + ) + + assert native.settings is not None and native.settings.agent_context is not None + assert managed.settings is not None and managed.settings.agent_context is not None + assert native.settings.agent_context.skills == [] + assert [s.name for s in managed.settings.agent_context.skills] == ["a"] + + +def test_disabled_skills_deny_the_catalog(llm_store: LLMProfileStore) -> None: + plan = prepare_agent_launch( + _openhands_profile(disabled_skills=["b"]), + catalog=_catalog( + llm_store, + skills=[Skill(name="a", content="x"), Skill(name="b", content="y")], + ), + ) + assert isinstance(plan.settings, OpenHandsAgentSettings) + assert [s.name for s in plan.settings.agent_context.skills] == ["a"] + # The deny-list rides the context so lazily-loaded project skills honor it. + assert plan.settings.agent_context.disabled_skills == ["b"] + + +def test_global_memory_preference_is_stamped(llm_store: LLMProfileStore) -> None: + plan = prepare_agent_launch( + _openhands_profile(), + catalog=_catalog(llm_store), + runtime=AgentLaunchRuntime(load_memory=True), + ) + assert isinstance(plan.settings, OpenHandsAgentSettings) + assert plan.settings.agent_context.load_memory is True + + +# --------------------------------------------------------------------------- # +# Additions and provenance +# --------------------------------------------------------------------------- # + + +def test_additions_append_to_the_profile_suffix(llm_store: LLMProfileStore) -> None: + plan = prepare_agent_launch( + _openhands_profile(system_message_suffix="PROFILE_BASELINE"), + catalog=_catalog(llm_store), + additions=AgentLaunchAdditions( + system_message_suffix_append=f" {_SUFFIX_APPEND} " + ), + ) + assert isinstance(plan.settings, OpenHandsAgentSettings) + suffix = plan.settings.agent_context.system_message_suffix + assert suffix == f"PROFILE_BASELINE\n\n{_SUFFIX_APPEND}" + + +def test_additions_cannot_widen_a_profiles_scope( + llm_store: LLMProfileStore, mcp_config: dict[str, MCPServer] +) -> None: + profile = _openhands_profile( + tools=[Tool(name="terminal")], + mcp_server_refs=[], + secret_refs=["ALLOWED"], + disabled_skills=["a"], + ) + catalog = _catalog( + llm_store, mcp_config=mcp_config, skills=[Skill(name="a", content="x")] + ) + additions = AgentLaunchAdditions( + system_message_suffix_append=_SUFFIX_APPEND, llm_profile_ref="picked" + ) + + plain = prepare_agent_launch(profile, catalog=catalog) + added = prepare_agent_launch(profile, catalog=catalog, additions=additions) + + assert isinstance(plain.settings, OpenHandsAgentSettings) + assert isinstance(added.settings, OpenHandsAgentSettings) + assert added.settings.tools == plain.settings.tools + assert added.settings.mcp_config == plain.settings.mcp_config == {} + assert added.settings.agent_context.skills == plain.settings.agent_context.skills + assert added.allowed_secrets == plain.allowed_secrets == frozenset({"ALLOWED"}) + + +def test_llm_profile_override_swaps_the_llm_and_is_recorded( + llm_store: LLMProfileStore, +) -> None: + profile = _openhands_profile() + plan = prepare_agent_launch( + profile, + catalog=_catalog(llm_store), + additions=AgentLaunchAdditions(llm_profile_ref="picked"), + ) + assert isinstance(plan.agent, Agent) + assert plan.agent.llm.model == "claude-opus-5" + assert plan.launched_profile is not None + assert plan.launched_profile.llm_profile_ref == "picked" + assert plan.launched_profile.agent_profile_id == profile.id + + +def test_llm_profile_override_rejects_non_openhands_sources( + llm_store: LLMProfileStore, +) -> None: + additions = AgentLaunchAdditions(llm_profile_ref="picked") + with pytest.raises(AgentLaunchError, match="ACP"): + prepare_agent_launch( + ACPAgentProfile(name="acp", acp_server="claude-code"), + catalog=_catalog(llm_store), + additions=additions, + ) + with pytest.raises(AgentLaunchError, match="Agent Profile"): + prepare_agent_launch( + Agent(llm=LLM(model="gpt-4o", usage_id="agent"), tools=[]), + additions=additions, + ) + + +def test_provenance_marks_an_inline_draft(llm_store: LLMProfileStore) -> None: + stored = prepare_agent_launch( + _openhands_profile(secret_refs=[]), + catalog=_catalog(llm_store), + profile_origin="stored", + ) + inline = prepare_agent_launch( + _openhands_profile(secret_refs=[]), + catalog=_catalog(llm_store), + profile_origin="inline", + ) + assert stored.launched_profile is not None + assert inline.launched_profile is not None + assert stored.launched_profile.inline is False + assert inline.launched_profile.inline is True + assert stored.allowed_secrets == frozenset() + + +def test_no_provenance_is_recorded_when_not_asked(llm_store: LLMProfileStore) -> None: + plan = prepare_agent_launch( + _openhands_profile(), catalog=_catalog(llm_store), profile_origin=None + ) + assert plan.launched_profile is None + + +# --------------------------------------------------------------------------- # +# Raw agents +# --------------------------------------------------------------------------- # + + +def test_raw_agent_gets_memory_and_additions_but_keeps_its_own_llm() -> None: + agent = Agent( + llm=LLM(model="gpt-4o", usage_id="agent", stream=False), + tools=[], + agent_context=AgentContext(system_message_suffix="BASE"), + ) + plan = prepare_agent_launch( + agent, + runtime=AgentLaunchRuntime( + load_memory=True, stream=True, browser_available=True + ), + additions=AgentLaunchAdditions(system_message_suffix_append=_SUFFIX_APPEND), + ) + assert isinstance(plan.agent, Agent) + context = plan.agent.agent_context + assert context is not None + assert context.load_memory is True + assert context.system_message_suffix == f"BASE\n\n{_SUFFIX_APPEND}" + # A raw agent is the explicit low-level option: nothing else is imposed. + assert plan.agent.llm.stream is False + assert plan.agent.tools == [] + assert plan.settings is None + assert plan.launched_profile is None + + +def test_raw_acp_agent_loses_managed_skills_under_native_sourcing() -> None: + agent = ACPAgent( + acp_command=["echo", "acp"], + agent_context=AgentContext( + skills=[Skill(name="a", content="x")], load_user_skills=True + ), + ) + plan = prepare_agent_launch( + agent, runtime=AgentLaunchRuntime(acp_skill_sourcing="native") + ) + assert isinstance(plan.agent, ACPAgent) + assert plan.agent.agent_context is not None + assert plan.agent.agent_context.skills == [] + assert plan.agent.agent_context.load_user_skills is False + + +def test_an_agent_profile_launch_needs_a_catalog() -> None: + with pytest.raises(TypeError, match="catalog"): + prepare_agent_launch(_openhands_profile()) + + +# --------------------------------------------------------------------------- # +# Dangling references +# --------------------------------------------------------------------------- # + + +def test_every_dangling_reference_is_reported_at_once( + llm_store: LLMProfileStore, mcp_config: dict[str, MCPServer] +) -> None: + profile = OpenHandsAgentProfile( + name="oh", llm_profile_ref="missing", mcp_server_refs=["fetch", "gone"] + ) + with pytest.raises(UnresolvedProfileReferences) as exc_info: + prepare_agent_launch( + profile, catalog=_catalog(llm_store, mcp_config=mcp_config) + ) + + error = exc_info.value + assert error.llm_profile_ref == "missing" + assert error.mcp_server_refs == ["gone"] + detail = error.to_detail() + assert detail["code"] == "unresolved_profile_references" + assert detail["dangling_llm_profile_ref"] == "missing" + assert detail["dangling_mcp_server_refs"] == ["gone"] + assert "missing" in detail["message"] + + +def test_an_overridden_llm_ref_that_dangles_names_the_override( + llm_store: LLMProfileStore, +) -> None: + with pytest.raises(UnresolvedProfileReferences) as exc_info: + prepare_agent_launch( + _openhands_profile(), + catalog=_catalog(llm_store), + additions=AgentLaunchAdditions(llm_profile_ref="gone"), + ) + assert exc_info.value.llm_profile_ref == "gone" + + +# --------------------------------------------------------------------------- # +# Deprecated ``agent_settings`` launches +# --------------------------------------------------------------------------- # + + +def _agent_settings_payload() -> OpenHandsAgentSettings: + settings = validate_agent_settings( + { + "agent_kind": "openhands", + "llm": { + "model": "gpt-4o", + "usage_id": "agent", + "api_key": _LLM_SECRET, + }, + "tools": [{"name": "terminal"}], + "tool_concurrency_limit": 3, + "enable_switch_llm_tool": False, + "mcp_config": {"fetch": {"url": "https://fetch.test"}}, + "verification": { + "critic_enabled": True, + "critic_api_key": _CRITIC_SECRET, + }, + "agent_context": AgentContext( + skills=[Skill(name="a", content="x"), Skill(name="b", content="y")], + disabled_skills=["b"], + system_message_suffix="CLIENT_SUFFIX", + user_message_suffix="USER_SUFFIX", + secrets={"CONTEXT_SECRET": StaticSecret(value=SecretStr("v"))}, + current_datetime="2020-01-01T00:00", + ).model_dump(), + } + ) + assert isinstance(settings, OpenHandsAgentSettings) + return settings + + +def test_agent_settings_launch_resolves_what_the_client_sent() -> None: + settings = _agent_settings_payload() + profile, catalog = agent_settings_launch_source(settings) + plan = prepare_agent_launch( + profile, + catalog=catalog, + runtime=AgentLaunchRuntime(stream=True), + profile_origin=None, + ) + + assert isinstance(plan.settings, OpenHandsAgentSettings) + resolved = plan.settings + assert [t.name for t in (resolved.tools or [])] == ["terminal"] + assert resolved.tool_concurrency_limit == 3 + assert resolved.enable_switch_llm_tool is False + assert list(resolved.mcp_config) == ["fetch"] + assert isinstance(resolved.llm.api_key, SecretStr) + assert resolved.llm.api_key.get_secret_value() == _LLM_SECRET + assert resolved.llm.stream is True + context = resolved.agent_context + assert context.system_message_suffix == "CLIENT_SUFFIX" + assert [s.name for s in context.skills] == ["a"] + # Fields no Agent Profile models are carried through, not dropped. + assert context.user_message_suffix == "USER_SUFFIX" + assert context.secrets is not None and "CONTEXT_SECRET" in context.secrets + critic_key = resolved.verification.critic_api_key + assert isinstance(critic_key, SecretStr) + assert critic_key.get_secret_value() == _CRITIC_SECRET + # The stale saved timestamp is replaced at launch (#5141). + assert isinstance(context.current_datetime, datetime) + # No stored profile launched this conversation. + assert plan.launched_profile is None + assert plan.allowed_secrets is None + + +def test_agent_settings_acp_launch_keeps_non_profile_fields() -> None: + settings = validate_agent_settings( + { + "agent_kind": "acp", + "acp_server": "claude-code", + "acp_startup_timeout": 120.0, + "acp_isolate_data_dir": True, + "agent_context": AgentContext( + skills=[Skill(name="a", content="x")], load_user_skills=True + ).model_dump(), + } + ) + profile, catalog = agent_settings_launch_source(settings) + plan = prepare_agent_launch( + profile, + catalog=catalog, + runtime=AgentLaunchRuntime(acp_skill_sourcing="native"), + profile_origin=None, + ) + + assert isinstance(plan.settings, ACPAgentSettings) + assert plan.settings.acp_startup_timeout == 120.0 + assert plan.settings.acp_isolate_data_dir is True + context = plan.settings.agent_context + assert context is not None + assert context.skills == [] + assert context.load_user_skills is False From 67857494656c065b9cdbd453a6e7de4caed0dd88 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:13:11 -0400 Subject: [PATCH 2/4] fix: narrow types in launch tests for pyright Co-authored-by: openhands Co-Authored-By: Claude Opus 5 (1M context) --- tests/agent_server/docker_runtime/test_mediation.py | 5 +++-- tests/agent_server/test_conversation_router.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/agent_server/docker_runtime/test_mediation.py b/tests/agent_server/docker_runtime/test_mediation.py index 7ccf38df13..87c055cbb2 100644 --- a/tests/agent_server/docker_runtime/test_mediation.py +++ b/tests/agent_server/docker_runtime/test_mediation.py @@ -149,8 +149,9 @@ async def test_profile_launch_scopes_secrets_and_stamps_provenance( finished = await _finish(prepared) assert finished.agent_profile_id is None assert set(finished.secrets) == {"ALLOWED"} - assert finished.agent.llm.api_key is not None - assert finished.agent.llm.api_key.get_secret_value() == "model-key" + api_key = finished.agent.llm.api_key + assert isinstance(api_key, SecretStr) + assert api_key.get_secret_value() == "model-key" @pytest.mark.asyncio diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index b9077668ad..65a2bb84a4 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -789,7 +789,7 @@ def test_start_conversation_accepts_acp_agent_settings( assert response.status_code == 201 request = mock_conversation_service.start_conversation.call_args.args[0] agent = validate_agent_settings(request.agent_settings).create_agent() - assert agent.kind == "ACPAgent" + assert isinstance(agent, ACPAgent) assert agent.acp_command == ["echo", "settings"] assert agent.acp_args == ["--verbose"] assert agent.acp_model == "acp-test-model" From 3218829d9de62d70956dc28368d384c2d040414c Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:31:00 -0400 Subject: [PATCH 3/4] fix: assert the encrypted agent_settings payload, not a pre-resolved agent Co-authored-by: openhands Co-Authored-By: Claude Opus 5 (1M context) --- tests/agent_server/test_conversation_service.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index a5e1d2efb1..1faf726d73 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -300,12 +300,10 @@ async def test_start_conversation_decrypts_encrypted_agent_settings_mcp_env( confirmation_policy=NeverConfirm(), secrets_encrypted=True, ) - assert ( - dump_mcp_config(request.agent.mcp_config)["github"]["env"][ - "GITHUB_PERSONAL_ACCESS_TOKEN" - ] - == encrypted_mcp_token - ) + # The payload rides the request untouched; the server decrypts it when it + # resolves the launch. + assert request.agent is None + assert request.agent_settings is not None captured: dict[str, Any] = {} From 42344313ef8c0e42c777a1b1e0c37f61953bb722 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:32:20 -0400 Subject: [PATCH 4/4] refactor: tidy the ACP base-settings branch in the launch resolver Co-authored-by: openhands Co-Authored-By: Claude Opus 5 (1M context) --- openhands-sdk/openhands/sdk/profiles/resolver.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/profiles/resolver.py b/openhands-sdk/openhands/sdk/profiles/resolver.py index 56bc070ba2..918b8423f3 100644 --- a/openhands-sdk/openhands/sdk/profiles/resolver.py +++ b/openhands-sdk/openhands/sdk/profiles/resolver.py @@ -431,12 +431,12 @@ def _build_acp_settings( "acp_args": list(profile.acp_args) if profile.acp_args else [], "mcp_config": mcp_config, } - base_context = None if base is None else base.agent_context if base is not None and base.agent_kind == "acp": + base_context = base.agent_context context = ( - base_context.model_copy(update=context_fields) - if base_context is not None - else AgentContext(**context_fields) + AgentContext(**context_fields) + if base_context is None + else base_context.model_copy(update=context_fields) ) return base.model_copy(update={**fields, "agent_context": context}) return validate_agent_settings( @@ -662,6 +662,8 @@ def load(self, name: str, *, cipher: Cipher | None = None) -> LLM: # noqa: ARG0 return self.llm +# The inline profile a deprecated ``agent_settings`` launch becomes. Its LLM +# "reference" resolves against a one-entry loader holding the payload's own LLM. _AGENT_SETTINGS_PROFILE_NAME = "agent_settings"