diff --git a/.pr/design.md b/.pr/design.md new file mode 100644 index 0000000000..7c72a272af --- /dev/null +++ b/.pr/design.md @@ -0,0 +1,139 @@ +# Per-call `llm_profile` on the task tool + +## Problem + +The task tool can delegate work to a subagent, but an inherited subagent always +uses the parent conversation's LLM. A parent that wants a mixed-model workflow +must either switch its own model or rely on a static `model:` value in an agent +definition. Neither supports choosing a worker profile for an individual task +while the parent keeps its current model. + +## User-facing behavior + +`TaskAction` gains an optional `llm_profile` field naming a saved LLM profile. + +An inherited agent uses the task call's `llm_profile` when supplied and otherwise +uses the parent model. Supplying `llm_profile` for an agent definition that already +selects a model is a configuration conflict and fails explicitly; neither choice +is silently discarded. + +For resumable tasks, a bare resume retains the task's effective per-call profile; +an explicit profile on a resume replaces it. Resume remains scoped to the current +in-memory `TaskManager` lifecycle. + +Unknown profiles fail loudly and become an error `TaskObservation`. The task is +not partially registered and the worker never silently falls back to the parent +model. + +## Shared profile-loading boundary + +`LocalConversation.load_profile_llm()` is the common cipher-aware loading +primitive for conversation-owned profile operations. It selects the +conversation's default store or a caller-supplied profile directory, then calls +`LLMProfileStore.load(profile_name, cipher=self._cipher)`. + +The operation only loads. It does not activate the returned LLM, add it to the +parent conversation's registry, or bind parent conversation context. + +The existing higher-level methods add their own behavior: + +- `switch_profile()` loads through the primitive, assigns the canonical + `profile:` usage ID, and activates the LLM; +- `get_or_create_profile_llm()` returns a registry hit or loads through the + primitive, assigns the caller's usage ID, registers the LLM, and binds the + conversation context; +- `TaskManager` loads through the primitive, resets metrics, and passes the LLM + into the worker factory. The worker `LocalConversation` then owns registration + and context binding. + +Using `get_or_create_profile_llm()` directly for a worker would register an +otherwise unused template LLM in the parent conversation and give it the wrong +metrics/context owner. Adding a mode flag to that method would also make its +registry-oriented contract ambiguous. + +`LLMProfileStore.load()` already performs persisted subscription restoration +through `LLM.from_persisted()`. The loading primitive therefore returns the +runtime subscription LLM while still avoiding parent registration and +activation. + +The store's native error contract is preserved: a missing profile raises +`FileNotFoundError` with the available profile files, while invalid or corrupted +profiles raise `ValueError`. There is no separate list-before-load operation, so +validation and reading happen through the store's locked load path. + +## Worker construction and metrics + +The selected LLM is injected before `factory_func` runs. This matters because a +factory may derive a default condenser from its input LLM; swapping afterward +would leave the condenser on the parent model. + +Subscription-backed LLMs retain the factory-created condenser. Subscription +completion dispatch supports condenser calls, matching top-level agent creation +and profile switching. + +The loaded LLM gets a fresh metrics object before factory construction. It is +never registered in the parent's `llm_registry`. The worker conversation tracks +its own agent and condenser usage, and the existing task completion path copies +the worker's combined metrics into the parent under `task:` exactly +once. The parent's active model is unchanged. + +## Profile discovery and confidentiality + +The task tool description lists saved profile names using the same public +`get_llm_profile_names()` and `format_llm_profiles()` helpers as `switch_llm`. +It exposes names only: profile model IDs, provider URLs, API keys, and persisted +JSON are not included. When no profiles exist, the section is omitted. + +The list is a creation-time snapshot of the default profile store. When any +registered agent specifies a custom `profile_store_dir`, the task tool omits the +list rather than advertising names from a store that the selected agent may not +use. + +## Persistence and compatibility + +`Task.llm_profile` stores the effective per-call profile for resume. Existing task +calls omit the optional field and continue inheriting the parent model. + +The additive action field has the repository's normal version-skew caveat: an old +SDK that forbids unknown action fields cannot deserialize a new event containing +`llm_profile`. + +## Verification matrix + +Focused tests cover: + +- two sequential tasks choosing different saved profiles; +- parent model unchanged and no worker profile entry in the parent registry; +- independent worker metrics and `task:` merge-back; +- encrypted-secret loading through the conversation cipher; +- persisted subscription restoration; +- subscription workers retain their supported LLM-backed condenser; +- custom profile directories; +- native missing-profile errors with no partial task state; +- explicit conflicts between definition-owned models and per-call profiles; +- bare-resume retention and explicit resume replacement; +- a clear missing-profile failure when a stored profile is removed before resume; +- inherited behavior when no override is supplied; +- pre-factory injection and `stream=False` worker behavior; +- profile names present while model IDs, provider URLs, and API keys remain absent + from the tool description; +- omission of the profile section when no profiles exist. +- omission of default-store profile names when a registered agent uses a custom + profile store. + +Current verification on the rebased branch: + +- 235 task, subagent, profile-switch, and conversation-switch tests pass; +- pre-commit passes on every revised file, including Pyright; +- a live encrypted-profile delegation completed with a MiniMax M3 parent and a + MiMo v2.5 Pro worker. The parent remained on MiniMax, its registry contained no + worker profile entry, its only metrics key was `task:task_00000001`, and a bare + resume retained the encrypted worker profile and MiMo model. + +## Out of scope + +- changing the `switch_llm` or subagent defaults; +- enabling parallel task execution by default; +- adding frontend worker-model reporting; +- changing definition-level profile loading; +- changing the conversation-spawn workflow. diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 65545b8818..0918e14b31 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -1685,7 +1685,7 @@ def switch_profile(self, profile_name: str) -> None: try: cached = self.llm_registry.get(usage_id) except KeyError: - loaded = self._profile_store.load(profile_name, cipher=self._cipher) + loaded = self.load_profile_llm(profile_name) cached = loaded.model_copy(update={"usage_id": usage_id}) self.switch_llm(cached) @@ -1700,13 +1700,40 @@ def get_or_create_profile_llm(self, profile_name: str, usage_id: str) -> LLM: try: return self.llm_registry.get(usage_id) except KeyError: - loaded = self._profile_store.load(profile_name, cipher=self._cipher) + loaded = self.load_profile_llm(profile_name) llm = loaded.model_copy(update={"usage_id": usage_id}) llm = create_subscription_llm_from_config(llm) self.llm_registry.add(llm) self._bind_conversation_context(llm) return llm + def load_profile_llm( + self, profile_name: str, profile_store_dir: str | None = None + ) -> LLM: + """Load a saved profile LLM without registering or activating it. + + The returned LLM is not added to ``llm_registry`` or made active, so the + caller owns its lifecycle and metrics attribution. Persisted subscription + profiles are restored by :class:`LLMProfileStore` during the load. + Secrets are decrypted with the conversation's cipher. + + Args: + profile_name: Name of a profile previously saved via + LLMProfileStore (a ``.json`` suffix is tolerated). + profile_store_dir: Optional override for the store directory. When + None, the conversation's default store is used. + + Raises: + FileNotFoundError: If the profile does not exist. + ValueError: If the profile is corrupted or invalid. + """ + store = ( + self._profile_store + if profile_store_dir is None + else LLMProfileStore(profile_store_dir) + ) + return store.load(profile_name, cipher=self._cipher) + def switch_acp_model(self, model: str) -> None: """Switch the model on an ACP conversation. diff --git a/openhands-sdk/openhands/sdk/subagent/AGENTS.md b/openhands-sdk/openhands/sdk/subagent/AGENTS.md index bd18357299..6f8e310c2d 100644 --- a/openhands-sdk/openhands/sdk/subagent/AGENTS.md +++ b/openhands-sdk/openhands/sdk/subagent/AGENTS.md @@ -111,6 +111,8 @@ Supported YAML frontmatter keys (see `AgentDefinition.load` in `schema.py`): - `skills` (default: `[]`): a comma-separated string or a list of skill names - `model` (default: `inherit`): `inherit` reuses the parent LLM; another value is loaded as an LLM profile name from `profile_store_dir` or the default profile store + - the task tool's per-call `llm_profile` applies only to `model: inherit` + definitions; supplying it for a definition that selects a model is an error - `color` (optional) - `max_iteration_per_run` (optional, positive integer) - `max_budget_per_run` (optional, positive number in USD) diff --git a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py index 99e016f5f0..7921baf783 100644 --- a/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py +++ b/openhands-sdk/openhands/sdk/tool/builtins/switch_llm.py @@ -88,7 +88,8 @@ def get_llm_profile_names() -> list[str]: return [summary["name"] for summary in LLMProfileStore().list_summaries()] -def _format_profiles(profile_names: Sequence[str]) -> str: +def format_llm_profiles(profile_names: Sequence[str]) -> str: + """Format saved LLM profile names as a bullet list for tool descriptions.""" if not profile_names: return "- No saved LLM profiles are currently available." return "\n".join(f"- {name}" for name in sorted(profile_names)) @@ -164,7 +165,7 @@ def create( return [ cls( description=_DESCRIPTION_TEMPLATE.format( - profiles=_format_profiles(profile_names) + profiles=format_llm_profiles(profile_names) ), action_type=SwitchLLMAction, observation_type=SwitchLLMObservation, diff --git a/openhands-tools/openhands/tools/task/definition.py b/openhands-tools/openhands/tools/task/definition.py index f1a2d335f3..f8120c4b3a 100644 --- a/openhands-tools/openhands/tools/task/definition.py +++ b/openhands-tools/openhands/tools/task/definition.py @@ -26,6 +26,10 @@ ToolDefinition, register_tool, ) +from openhands.sdk.tool.builtins.switch_llm import ( + format_llm_profiles, + get_llm_profile_names, +) if TYPE_CHECKING: @@ -48,6 +52,14 @@ class TaskAction(Action): default="general-purpose", description="The type of specialized agent to use for this task.", ) + llm_profile: str | None = Field( + default=None, + description=( + "Saved LLM profile for this subagent. If omitted, the subagent " + "inherits the parent model. Do not supply this when the selected " + "agent definition already specifies a model profile." + ), + ) resume: str | None = Field( default=None, description="Task ID of the task to resume from.", @@ -117,7 +129,7 @@ def to_llm_content(self) -> Sequence[TextContent | ImageContent]: Available agent types and the tools they have access to: {agent_types_info} -When NOT to use the task tool: +{llm_profiles_section}When NOT to use the task tool: - A single grep, find, or cat command would answer your question — just run it yourself - You are making a file edit (use file_editor directly) - You already have the context needed @@ -235,13 +247,29 @@ def create( agent_types_info = get_factory_info() - registered = {d.name for d in get_registered_agent_definitions()} + agent_definitions = get_registered_agent_definitions() + has_custom_profile_store = any( + definition.profile_store_dir is not None for definition in agent_definitions + ) + profile_names = [] if has_custom_profile_store else get_llm_profile_names() + llm_profiles_section = "" + if profile_names: + llm_profiles_section = ( + "To run a subagent on a different model than your own, pass " + "llm_profile with one of these saved LLM profiles:\n" + f"{format_llm_profiles(profile_names)}\n" + "This affects only the delegated subagent; " + "your own model is unchanged.\n\n" + ) + + registered = {definition.name for definition in agent_definitions} task_tool_examples = "\n".join( ex for name, ex in TASK_TOOL_EXAMPLES.items() if name in registered ) task_description = TASK_TOOL_DESCRIPTION.format( agent_types_info=agent_types_info, + llm_profiles_section=llm_profiles_section, task_tool_examples=task_tool_examples, ) diff --git a/openhands-tools/openhands/tools/task/impl.py b/openhands-tools/openhands/tools/task/impl.py index eeee58c023..e15bc2cb59 100644 --- a/openhands-tools/openhands/tools/task/impl.py +++ b/openhands-tools/openhands/tools/task/impl.py @@ -35,6 +35,7 @@ def __call__( description=action.description, resume=action.resume, conversation=conversation, + llm_profile=action.llm_profile, ) match task.status: case TaskStatus.COMPLETED: diff --git a/openhands-tools/openhands/tools/task/manager.py b/openhands-tools/openhands/tools/task/manager.py index a8b52a1d60..a98c01d81b 100644 --- a/openhands-tools/openhands/tools/task/manager.py +++ b/openhands-tools/openhands/tools/task/manager.py @@ -48,6 +48,11 @@ _SUBAGENTS_DIR: Final[str] = "subagents" +def _definition_has_model_profile(factory: "AgentFactory") -> bool: + """Return whether the agent definition selects its own model profile.""" + return bool(factory.definition.model) and factory.definition.model != "inherit" + + class TaskStatus(StrEnum): """Represents the lifecycle states of a task.""" @@ -73,6 +78,11 @@ class Task(BaseModel): ) result: str | None = Field(default=None, description="Result of the task.") error: str | None = Field(default=None, description="Error if task failed.") + llm_profile: str | None = Field( + default=None, + description="Effective per-call LLM profile the task's worker runs on. " + "A bare resume keeps it; an explicit resume-time override replaces it.", + ) conversation: LocalConversation | None = Field( default=None, exclude=True, @@ -168,6 +178,7 @@ def start_task( resume: str | None = None, description: str | None = None, conversation: LocalConversation | None = None, + llm_profile: str | None = None, ) -> Task: """Start a blocking sub-agent task. @@ -177,6 +188,11 @@ def start_task( resume: Task ID to resume (continues existing conversation). description: Short label for the task. conversation: Parent conversation (set on first call). + llm_profile: Optional saved LLM profile to run the sub-agent on + instead of inheriting the parent's model. This conflicts with + a ``model:`` pin in the sub-agent definition. On resume, + omitting it keeps the profile the task was created with; + passing one explicitly switches the resumed worker to it. Returns: TaskState with the final result. @@ -188,11 +204,13 @@ def start_task( task = self._resume_task( resume=resume, subagent_type=subagent_type, + llm_profile=llm_profile, ) else: task = self._create_task( subagent_type=subagent_type, description=description, + llm_profile=llm_profile, ) return self._run_task( @@ -200,8 +218,16 @@ def start_task( prompt=prompt, ) - def _resume_task(self, resume: str, subagent_type: str) -> Task: - """Resume a sub-agent task.""" + def _resume_task( + self, resume: str, subagent_type: str, llm_profile: str | None = None + ) -> Task: + """Resume a sub-agent task. + + A None llm_profile keeps the profile the task was created with when the + selected definition inherits its model. Definitions that pin a model + discard the stored profile. An explicit profile overrides the stored + one, but conflicts with a definition-pinned model. + """ with self._tasks_lock: if resume not in self._tasks: raise ValueError( @@ -209,8 +235,14 @@ def _resume_task(self, resume: str, subagent_type: str) -> Task: f"Available tasks: {', '.join(sorted(self._tasks))}" ) + stored = self._tasks[resume] factory = get_agent_factory(subagent_type) - worker_agent = self._get_sub_agent_from_factory(factory) + requested_profile = llm_profile + if requested_profile is None and not _definition_has_model_profile(factory): + requested_profile = stored.llm_profile + worker_agent = self._get_sub_agent_from_factory( + factory, llm_profile=requested_profile + ) conversation_id = self._tasks[resume].conversation_id with detached_delegate_context() as link: conversation = LocalConversation( @@ -231,10 +263,11 @@ def _resume_task(self, resume: str, subagent_type: str) -> Task: factory.definition.get_confirmation_policy(), ) - self._tasks[resume] = self._tasks[resume].model_copy( + self._tasks[resume] = stored.model_copy( update={ "conversation": conversation, "status": TaskStatus.RUNNING, + "llm_profile": requested_profile, } ) @@ -244,6 +277,7 @@ def _create_task( self, subagent_type: str, description: str | None, + llm_profile: str | None = None, ) -> Task: """Create a fresh task. @@ -252,7 +286,9 @@ def _create_task( 2. The parent conversation's ``max_iteration_per_run`` """ factory = get_agent_factory(subagent_type) - worker_agent = self._get_sub_agent_from_factory(factory) + worker_agent = self._get_sub_agent_from_factory( + factory, llm_profile=llm_profile + ) effective_max_iter = ( factory.definition.max_iteration_per_run @@ -289,6 +325,7 @@ def _create_task( conversation_id=conversation_id, conversation=sub_conversation, status=TaskStatus.RUNNING, + llm_profile=llm_profile, ) return self._tasks[task_id] @@ -349,25 +386,45 @@ def _delegate_observability_metadata( **link, } - def _get_sub_agent(self, subagent_type: str) -> Agent: + def _get_sub_agent( + self, subagent_type: str, llm_profile: str | None = None + ) -> Agent: """Return the subagent assigned to the task. Raises: ValueError: If the subagent type is invalid. """ factory = get_agent_factory(subagent_type) - return self._get_sub_agent_from_factory(factory) + return self._get_sub_agent_from_factory(factory, llm_profile=llm_profile) - def _get_sub_agent_from_factory(self, factory: "AgentFactory") -> Agent: + def _get_sub_agent_from_factory( + self, factory: "AgentFactory", llm_profile: str | None = None + ) -> Agent: """Create a sub-agent from an AgentFactory.""" parent = self.parent_conversation parent_llm = parent.agent.llm - llm_updates: dict = {"stream": False} - sub_agent_llm = parent_llm.model_copy(update=llm_updates) - # Reset metrics such that the sub-agent has its own - # Metrics object - sub_agent_llm.reset_metrics() + definition_has_model = _definition_has_model_profile(factory) + if llm_profile is not None and definition_has_model: + raise ValueError( + f"Agent '{factory.definition.name}' already selects model profile " + f"'{factory.definition.model}'; remove llm_profile or choose an " + "agent definition that inherits the parent model." + ) + + if llm_profile is not None: + # Injected pre-factory so the factory derives the default condenser + # LLM from the selected profile. + sub_agent_llm = parent.load_profile_llm( + llm_profile, + profile_store_dir=factory.definition.profile_store_dir, + ) + sub_agent_llm.reset_metrics() + else: + sub_agent_llm = parent_llm.model_copy(update={"stream": False}) + # Reset metrics such that the sub-agent has its own + # Metrics object + sub_agent_llm.reset_metrics() sub_agent = factory.factory_func(sub_agent_llm) diff --git a/tests/sdk/conversation/test_switch_model.py b/tests/sdk/conversation/test_switch_model.py index 17386b5924..c701ed6eba 100644 --- a/tests/sdk/conversation/test_switch_model.py +++ b/tests/sdk/conversation/test_switch_model.py @@ -594,6 +594,107 @@ def test_switch_profile_decrypts_with_cipher(tmp_path, monkeypatch): assert api_key.get_secret_value() == "plaintext-secret" +def test_load_profile_llm_decrypts_with_cipher(tmp_path, monkeypatch): + """load_profile_llm decrypts secrets with the conversation's cipher (like + switch_profile) but does NOT register the LLM in llm_registry — the caller + (e.g. the task tool) owns metrics attribution. + """ + profile_dir = tmp_path / "profiles" + profile_dir.mkdir() + monkeypatch.setattr(llm_profile_store, "_DEFAULT_PROFILE_DIR", profile_dir) + + cipher = Cipher("test-key-for-load-profile-llm") + store = LLMProfileStore(base_dir=profile_dir) + store.save( + "encrypted", + LLM( + model="gpt-4o", + usage_id="encrypted", + api_key=SecretStr("plaintext-secret"), + ), + include_secrets=True, + cipher=cipher, + ) + + conv = LocalConversation( + agent=Agent( + llm=_make_llm("default-model", "test-llm"), + tools=[], + ), + workspace=Path.cwd(), + cipher=cipher, + ) + + llm = conv.load_profile_llm("encrypted") + + assert isinstance(llm.api_key, SecretStr) + assert llm.api_key.get_secret_value() == "plaintext-secret" + assert conv.agent.llm.model == "default-model" + with pytest.raises(KeyError): + conv.llm_registry.get(llm.usage_id) + + +def test_load_profile_llm_unknown_lists_available(profile_store): + """Unknown profile uses the store's native error and lists its profiles.""" + conv = _make_conversation() + with pytest.raises(FileNotFoundError, match="not found") as excinfo: + conv.load_profile_llm("missing") + assert "fast" in str(excinfo.value) + + +def test_load_profile_llm_custom_store_dir(tmp_path, monkeypatch): + """profile_store_dir resolves from that dir instead of the default store.""" + monkeypatch.setattr( + llm_profile_store, "_DEFAULT_PROFILE_DIR", tmp_path / "default-profiles" + ) + custom_dir = tmp_path / "custom-profiles" + store = LLMProfileStore(base_dir=custom_dir) + store.save("custom", _make_llm("custom-model", "custom"), include_secrets=True) + + conv = _make_conversation() + + llm = conv.load_profile_llm("custom", profile_store_dir=str(custom_dir)) + assert llm.model == "custom-model" + + with pytest.raises(FileNotFoundError, match="not found"): + conv.load_profile_llm("custom") + + +def test_load_profile_llm_restores_subscription_profile(tmp_path, monkeypatch): + """Persisted subscription profiles are restored during the store load.""" + profile_dir = tmp_path / "profiles" + monkeypatch.setattr(llm_profile_store, "_DEFAULT_PROFILE_DIR", profile_dir) + store = LLMProfileStore(base_dir=profile_dir) + store.save( + "subscription", + LLM( + model="gpt-5.2-codex", + usage_id="subscription", + auth_type="subscription", + subscription_vendor="openai", + ), + ) + + restored: list[LLM] = [] + + def _restore_subscription(llm: LLM) -> LLM: + restored.append(llm) + return llm.model_copy(update={"model": "runtime-subscription-model"}) + + monkeypatch.setattr( + "openhands.sdk.llm.auth.openai.create_subscription_llm_from_config", + _restore_subscription, + ) + + conv = _make_conversation() + llm = conv.load_profile_llm("subscription") + + assert len(restored) == 1 + assert restored[0].auth_type == "subscription" + assert llm.model == "runtime-subscription-model" + assert conv.agent.llm.model == "default-model" + + def test_switch_profile_delegates_to_switch_llm(profile_store, monkeypatch): """switch_profile loads from disk and delegates to switch_llm; the LLM handed off carries the canonical ``profile:{name}`` usage_id. diff --git a/tests/tools/task/conftest.py b/tests/tools/task/conftest.py new file mode 100644 index 0000000000..902866a7f9 --- /dev/null +++ b/tests/tools/task/conftest.py @@ -0,0 +1,21 @@ +import pytest + +from openhands.sdk.llm import llm_profile_store +from openhands.sdk.subagent import registry + + +@pytest.fixture(autouse=True) +def _isolate_llm_profile_store(tmp_path, monkeypatch): + """Redirect the default LLM profile dir so tests never touch the real + ~/.openhands/profiles (TaskToolSet.create lists saved profiles). + + The registry's profile-store getter is lru_cached, so it is cleared before + and after each test: a store cached under an earlier test's (now-deleted) + tmp dir must not leak into the next test. + """ + registry._get_profile_store.cache_clear() + monkeypatch.setattr( + llm_profile_store, "_DEFAULT_PROFILE_DIR", tmp_path / "profiles" + ) + yield + registry._get_profile_store.cache_clear() diff --git a/tests/tools/task/test_task_manager.py b/tests/tools/task/test_task_manager.py index e459f42ebe..66bae18141 100644 --- a/tests/tools/task/test_task_manager.py +++ b/tests/tools/task/test_task_manager.py @@ -7,9 +7,11 @@ from pydantic import SecretStr from openhands.sdk import LLM, Agent +from openhands.sdk.context.condenser import LLMSummarizingCondenser from openhands.sdk.conversation.impl.local_conversation import LocalConversation from openhands.sdk.conversation.state import ConversationExecutionStatus from openhands.sdk.hooks.config import HookConfig, HookDefinition, HookMatcher +from openhands.sdk.llm.llm_profile_store import LLMProfileStore from openhands.sdk.subagent.registry import ( _reset_registry_for_tests, register_agent, @@ -31,6 +33,18 @@ def _make_llm() -> LLM: ) +def _make_profile_store(base_dir: Path, profiles: dict[str, str]) -> LLMProfileStore: + """Save one LLM profile per name -> model mapping into a store.""" + store = LLMProfileStore(base_dir=base_dir / "profiles") + for name, model in profiles.items(): + store.save( + name, + LLM(model=model, api_key=SecretStr(f"{name}-key"), usage_id=name), + include_secrets=True, + ) + return store + + def _make_parent_conversation( tmp_path: Path, persistence_dir: str | Path | None = None, @@ -525,6 +539,298 @@ def test_delegate_metadata_includes_parent_span_link(self, tmp_path): } +class TestTaskManagerLLMProfile: + """Tests for the per-call llm_profile override on task creation/resume. + + The conftest redirects the default profile-store dir to ``tmp_path``, so + ``_make_profile_store(tmp_path, ...)`` writes exactly where the parent + conversation's store (and the registry's cached getter) resolves. + """ + + def setup_method(self): + _reset_registry_for_tests() + + def teardown_method(self): + _reset_registry_for_tests() + + def test_create_task_with_llm_profile(self, tmp_path): + """A saved profile's LLM replaces the parent-inherited one.""" + manager, parent = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store(tmp_path, {"fast": "fast-model"}) + parent_model = parent.agent.llm.model + parent_metrics = parent.agent.llm.metrics + + task = manager._create_task( + subagent_type="general-purpose", + description=None, + llm_profile="fast", + ) + + assert task.llm_profile == "fast" + assert task.conversation is not None + assert task.conversation.agent.llm.model == "fast-model" + assert task.conversation.agent.llm.stream is False + assert task.conversation.agent.llm.metrics is not parent_metrics + assert parent.agent.llm.model == parent_model + assert "profile:fast" not in parent.llm_registry.list_usage_ids() + + def test_create_task_unknown_profile_raises(self, tmp_path): + """Unknown profiles raise before any task state is created.""" + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store(tmp_path, {"fast": "fast-model"}) + + with pytest.raises(FileNotFoundError, match="not found") as excinfo: + manager._create_task( + subagent_type="general-purpose", + description=None, + llm_profile="missing", + ) + + assert "fast" in str(excinfo.value) # available profiles are listed + assert len(manager._tasks) == 0 + + def test_definition_model_conflicts_with_llm_profile(self, tmp_path): + """A definition-owned model must not silently ignore ``llm_profile``.""" + from openhands.sdk.subagent.registry import agent_definition_to_factory + + agent_def = AgentDefinition( + name="pinned_agent", + description="Agent with its own model profile", + model="pinned", + tools=[], + system_prompt="You are pinned.", + ) + register_agent( + name="pinned_agent", + factory_func=agent_definition_to_factory(agent_def), + description=agent_def, + ) + + manager, _ = _manager_with_parent(tmp_path) + _make_profile_store(tmp_path, {"pinned": "pinned-model"}) + + with pytest.raises(ValueError, match="already selects model profile 'pinned'"): + manager._create_task( + subagent_type="pinned_agent", + description=None, + llm_profile="typo-does-not-exist", + ) + + assert len(manager._tasks) == 0 + + def test_subscription_profile_keeps_worker_condenser(self, tmp_path): + """Subscription workers retain the factory's supported condenser.""" + manager, parent = _manager_with_parent(tmp_path) + register_builtins_agents() + subscription_llm = _make_llm() + subscription_llm.is_subscription = True + + with patch.object(parent, "load_profile_llm", return_value=subscription_llm): + task = manager._create_task( + subagent_type="general-purpose", + description=None, + llm_profile="subscription", + ) + + assert task.conversation is not None + assert task.conversation.agent.llm.is_subscription is True + condenser = task.conversation.agent.condenser + assert isinstance(condenser, LLMSummarizingCondenser) + assert condenser.llm.is_subscription is True + + def test_resume_with_llm_profile(self, tmp_path): + """Resuming with llm_profile rebuilds the worker on that profile.""" + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store(tmp_path, {"fast": "fast-model"}) + + task = manager._create_task(subagent_type="general-purpose", description=None) + original_id = task.id + manager._evict_task(task) + + resumed = manager._resume_task( + resume=original_id, + subagent_type="general-purpose", + llm_profile="fast", + ) + + assert resumed.id == original_id + assert resumed.llm_profile == "fast" + assert resumed.conversation is not None + assert resumed.conversation.agent.llm.model == "fast-model" + assert resumed.conversation.agent.llm.stream is False + + def test_resume_definition_model_conflicts_with_llm_profile(self, tmp_path): + """Resume reports a definition/profile conflict without mutating state.""" + from openhands.sdk.subagent.registry import agent_definition_to_factory + + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store( + tmp_path, + {"fast": "fast-model", "pinned": "pinned-model"}, + ) + task = manager._create_task( + subagent_type="general-purpose", + description=None, + llm_profile="fast", + ) + manager._evict_task(task) + + agent_def = AgentDefinition( + name="pinned_agent", + description="Agent with its own model profile", + model="pinned", + tools=["terminal", "file_editor", "task_tracker"], + system_prompt="You are pinned.", + ) + register_agent( + name="pinned_agent", + factory_func=agent_definition_to_factory(agent_def), + description=agent_def, + ) + + with pytest.raises(ValueError, match="already selects model profile 'pinned'"): + manager._resume_task( + resume=task.id, + subagent_type="pinned_agent", + llm_profile="ignored-missing-profile", + ) + + assert manager._tasks[task.id].llm_profile == "fast" + assert manager._tasks[task.id].conversation is None + + def test_resume_definition_model_discards_stored_profile(self, tmp_path): + """A pinned definition replaces a stored inherited-model profile.""" + from openhands.sdk.subagent.registry import agent_definition_to_factory + + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store( + tmp_path, + {"fast": "fast-model", "pinned": "pinned-model"}, + ) + task = manager._create_task( + subagent_type="general-purpose", + description=None, + llm_profile="fast", + ) + manager._evict_task(task) + + agent_def = AgentDefinition( + name="pinned_agent", + description="Agent with its own model profile", + model="pinned", + tools=["terminal", "file_editor", "task_tracker"], + system_prompt="You are pinned.", + ) + register_agent( + name="pinned_agent", + factory_func=agent_definition_to_factory(agent_def), + description=agent_def, + ) + + resumed = manager._resume_task( + resume=task.id, + subagent_type="pinned_agent", + ) + + assert resumed.llm_profile is None + assert resumed.conversation is not None + assert resumed.conversation.agent.llm.model == "pinned-model" + + def test_resume_without_profile_keeps_original(self, tmp_path): + """A bare resume keeps the profile the task was created with.""" + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store(tmp_path, {"fast": "fast-model"}) + + task = manager._create_task( + subagent_type="general-purpose", description=None, llm_profile="fast" + ) + original_id = task.id + manager._evict_task(task) + + resumed = manager._resume_task( + resume=original_id, subagent_type="general-purpose" + ) + + assert resumed.llm_profile == "fast" + assert resumed.conversation is not None + assert resumed.conversation.agent.llm.model == "fast-model" + + def test_resume_after_profile_removed_returns_clear_error(self, tmp_path): + """A bare resume fails clearly if its stored profile was removed.""" + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + store = _make_profile_store(tmp_path, {"fast": "fast-model"}) + + task = manager._create_task( + subagent_type="general-purpose", description=None, llm_profile="fast" + ) + manager._evict_task(task) + store.delete("fast") + + with pytest.raises(FileNotFoundError, match="Profile `fast` not found"): + manager._resume_task( + resume=task.id, + subagent_type="general-purpose", + ) + + assert manager._tasks[task.id].llm_profile == "fast" + assert manager._tasks[task.id].conversation is None + + def test_resume_with_explicit_profile_overrides_stored(self, tmp_path): + """An explicit resume-time profile replaces the stored one for later + bare resumes.""" + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + _make_profile_store(tmp_path, {"fast": "fast-model", "slow": "slow-model"}) + + task = manager._create_task( + subagent_type="general-purpose", description=None, llm_profile="fast" + ) + original_id = task.id + manager._evict_task(manager._tasks[original_id]) + + resumed = manager._resume_task( + resume=original_id, + subagent_type="general-purpose", + llm_profile="slow", + ) + assert resumed.llm_profile == "slow" + assert resumed.conversation is not None + assert resumed.conversation.agent.llm.model == "slow-model" + + manager._evict_task(manager._tasks[original_id]) + resumed_again = manager._resume_task( + resume=original_id, subagent_type="general-purpose" + ) + assert resumed_again.llm_profile == "slow" + assert resumed_again.conversation is not None + assert resumed_again.conversation.agent.llm.model == "slow-model" + + def test_resume_after_inherit_created_still_inherits(self, tmp_path): + """A task created without llm_profile keeps inheriting the parent + model on bare resumes.""" + manager, _ = _manager_with_parent(tmp_path) + register_builtins_agents() + + task = manager._create_task(subagent_type="general-purpose", description=None) + original_id = task.id + manager._evict_task(task) + + resumed = manager._resume_task( + resume=original_id, subagent_type="general-purpose" + ) + + assert resumed.llm_profile is None + assert resumed.conversation is not None + assert resumed.conversation.agent.llm.model == "gpt-4o" + + def _make_task_with_mock_conv(task_id: str, **conv_kwargs) -> Task: """Create a Task with a MagicMock conversation, bypassing Pydantic validation.""" mock_conv = MagicMock(**conv_kwargs) diff --git a/tests/tools/task/test_task_tool_set.py b/tests/tools/task/test_task_tool_set.py index d12e16f397..ef38d3001c 100644 --- a/tests/tools/task/test_task_tool_set.py +++ b/tests/tools/task/test_task_tool_set.py @@ -1,10 +1,15 @@ import json +from unittest.mock import patch -from openhands.sdk import Agent, Conversation, LocalConversation, Tool +from pydantic import SecretStr + +from openhands.sdk import LLM, Agent, Conversation, LocalConversation, Tool from openhands.sdk.conversation.state import ConversationExecutionStatus from openhands.sdk.event.llm_convertible.observation import ObservationEvent from openhands.sdk.llm import Message, MessageToolCall, TextContent +from openhands.sdk.llm.llm_profile_store import LLMProfileStore from openhands.sdk.subagent.registry import _reset_registry_for_tests, register_agent +from openhands.sdk.subagent.schema import AgentDefinition from openhands.sdk.testing import TestLLM from openhands.tools.task import TaskToolSet from openhands.tools.task.definition import TASK_TOOL_EXAMPLES, TaskObservation @@ -17,6 +22,7 @@ def _task_tool_call( subagent_type: str = "test_agent", description: str | None = None, resume: str | None = None, + llm_profile: str | None = None, ) -> Message: """Build a Message whose only tool call is the task tool.""" args: dict = { @@ -27,6 +33,8 @@ def _task_tool_call( args["description"] = description if resume is not None: args["resume"] = resume + if llm_profile is not None: + args["llm_profile"] = llm_profile return Message( role="assistant", @@ -152,6 +160,77 @@ def test_two_sequential_tasks(self, tmp_path): assert observations[0].subagent == "agent_a" assert observations[1].subagent == "agent_b" + def test_two_sequential_tasks_with_llm_profiles(self, tmp_path): + """Two sequential tasks can each run on a different saved LLM profile.""" + # The conftest redirects the default profile dir to tmp_path / "profiles", + # which is where the parent conversation's profile store resolves. + store = LLMProfileStore(base_dir=tmp_path / "profiles") + for name, model in (("fast", "fast-model"), ("slow", "slow-model")): + store.save( + name, + LLM(model=model, api_key=SecretStr(f"{name}-key"), usage_id=name), + include_secrets=True, + ) + + factory_llms: list[LLM] = [] + + def make_factory(sub_llm: TestLLM): + def factory(llm: LLM) -> Agent: + factory_llms.append(llm) + return Agent(llm=sub_llm, tools=[]) + + return factory + + register_agent( + name="agent_a", + factory_func=make_factory( + TestLLM.from_messages([_text_message("first result")]) + ), + description="Test agent: agent_a", + ) + register_agent( + name="agent_b", + factory_func=make_factory( + TestLLM.from_messages([_text_message("second result")]) + ), + description="Test agent: agent_b", + ) + + parent_llm = TestLLM.from_messages( + [ + _task_tool_call( + "call_1", + prompt="Task A", + subagent_type="agent_a", + llm_profile="fast", + ), + _task_tool_call( + "call_2", + prompt="Task B", + subagent_type="agent_b", + llm_profile="slow", + ), + _text_message("Both tasks done."), + ] + ) + + agent = Agent(llm=parent_llm, tools=[Tool(name=TaskToolSet.name)]) + conversation = Conversation( + agent=agent, workspace=str(tmp_path), visualizer=None + ) + + conversation.send_message("Run two tasks") + conversation.run() + + assert ( + conversation.state.execution_status == ConversationExecutionStatus.FINISHED + ) + observations = _get_task_observations(conversation) + assert len(observations) == 2 + assert observations[0].text == "first result" + assert observations[1].text == "second result" + assert [llm.model for llm in factory_llms] == ["fast-model", "slow-model"] + def test_task_resume_across_turns(self, tmp_path): """A task can be launched, then resumed by passing the task_id.""" # Sub-agent for the first call @@ -239,6 +318,40 @@ def test_unknown_agent_type_returns_error_observation(self, tmp_path): assert obs.is_error is True assert "nonexistent_agent" in obs.text or "Unknown agent" in obs.text + def test_unknown_llm_profile_returns_error_observation(self, tmp_path): + """An unknown llm_profile yields an error TaskObservation naming it.""" + sub_llm = TestLLM.from_messages([_text_message("never reached")]) + _register_simple_agent("test_agent", sub_llm) + + parent_llm = TestLLM.from_messages( + [ + _task_tool_call( + "call_1", + prompt="Do something", + subagent_type="test_agent", + llm_profile="nonexistent_profile", + ), + _text_message("Oops."), + ] + ) + + agent = Agent(llm=parent_llm, tools=[Tool(name=TaskToolSet.name)]) + conversation = Conversation( + agent=agent, workspace=str(tmp_path), visualizer=None + ) + + conversation.send_message("Do something") + conversation.run() + + assert ( + conversation.state.execution_status == ConversationExecutionStatus.FINISHED + ) + observations = _get_task_observations(conversation) + assert len(observations) == 1 + obs = observations[0] + assert obs.is_error is True + assert "nonexistent_profile" in obs.text + def test_sub_agent_exception_returns_error_observation(self, tmp_path): """When the sub-agent's LLM raises, the task reports an error.""" sub_llm = TestLLM.from_messages( @@ -387,6 +500,81 @@ def test_no_matching_agent_example_excluded(self, tmp_path): for name, example_text in TASK_TOOL_EXAMPLES.items(): assert example_text.strip() not in description + def test_description_lists_llm_profiles(self, tmp_path): + """The tool description lists saved LLM profiles for llm_profile.""" + with patch( + "openhands.tools.task.definition.get_llm_profile_names", + return_value=["fast", "slow"], + ): + tools = TaskToolSet.create( + conv_state=None, # type: ignore[arg-type] + ) + description = tools[0].description + assert "llm_profile" in description + assert "- fast" in description + assert "- slow" in description + + def test_description_exposes_profile_names_only(self, tmp_path): + """Profile configuration and secrets are excluded from the description.""" + store = LLMProfileStore() + store.save( + "safe-name", + LLM( + model="sentinel-private-model", + base_url="https://sentinel-private.example", + api_key=SecretStr("sentinel-private-key"), + ), + include_secrets=True, + ) + + description = TaskToolSet.create( + conv_state=None, # type: ignore[arg-type] + )[0].description + + assert "- safe-name" in description + assert "sentinel-private-model" not in description + assert "sentinel-private.example" not in description + assert "sentinel-private-key" not in description + + def test_description_without_profiles_omits_section(self, tmp_path): + """With no saved profiles, the llm_profile section is omitted entirely + rather than rendering an incoherent "one of these: none" list.""" + with patch( + "openhands.tools.task.definition.get_llm_profile_names", + return_value=[], + ): + tools = TaskToolSet.create( + conv_state=None, # type: ignore[arg-type] + ) + description = tools[0].description + assert "llm_profile" not in description + assert "saved LLM profiles" not in description + + def test_description_omits_profiles_for_custom_store_agent(self, tmp_path): + """Do not advertise default-store names when an agent resolves elsewhere.""" + agent_def = AgentDefinition( + name="custom_store_agent", + description="Agent backed by a custom profile store", + model="inherit", + profile_store_dir=str(tmp_path / "custom-profiles"), + ) + register_agent( + name=agent_def.name, + factory_func=lambda llm: Agent(llm=llm, tools=[]), + description=agent_def, + ) + + with patch( + "openhands.tools.task.definition.get_llm_profile_names", + return_value=["default-only"], + ): + description = TaskToolSet.create( + conv_state=None, # type: ignore[arg-type] + )[0].description + + assert "default-only" not in description + assert "llm_profile" not in description + def test_only_registered_examples_included(self, tmp_path): """Only examples for registered agents appear; others are excluded.""" keys = list(TASK_TOOL_EXAMPLES.keys())