-
Notifications
You must be signed in to change notification settings - Fork 217
fix(agent-server/sdk): subagents missing in remote workspace #2275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
a49c80c
26e8137
53ff442
766a018
51e31ba
fc95bbb
f22586f
f8bd111
d8ba8e8
dbadf04
cd1d7ca
528ffc2
991b684
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,8 @@ def create_security_expert(llm): | |
| from threading import RLock | ||
| from typing import TYPE_CHECKING, NamedTuple | ||
|
|
||
| from pydantic import SecretStr | ||
|
|
||
| from openhands.sdk.logger import get_logger | ||
| from openhands.sdk.subagent.load import ( | ||
| load_project_agents, | ||
|
|
@@ -44,10 +46,10 @@ def create_security_expert(llm): | |
|
|
||
|
|
||
| class AgentFactory(NamedTuple): | ||
| """Simple container for an agent factory function and its description.""" | ||
| """Simple container for an agent factory function and its definition.""" | ||
|
|
||
VascoSch92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| factory_func: Callable[["LLM"], "Agent"] | ||
| description: str | ||
| definition: AgentDefinition | ||
VascoSch92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| # Global registry for user-registered agent factories | ||
|
|
@@ -71,19 +73,34 @@ def register_agent( | |
| Raises: | ||
| ValueError: If an agent with the same name already exists | ||
| """ | ||
| try: | ||
| from openhands.sdk.llm.llm import LLM as _LLM | ||
|
|
||
| _model_placeholder = "__introspect__" | ||
| agent = factory_func(_LLM(model=_model_placeholder, api_key=SecretStr("n/a"))) | ||
| definition = AgentDefinition.from_agent( | ||
| agent, name=name, description=description | ||
| ) | ||
| # If the model was our placeholder, the factory didn't set one explicitly | ||
|
||
| if definition.model == _model_placeholder: | ||
| definition = definition.model_copy(update={"model": "inherit"}) | ||
VascoSch92 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| except Exception: | ||
| logger.debug(f"Could not introspect factory for agent '{name}'") | ||
| definition = AgentDefinition(name=name, description=description) | ||
|
|
||
| with _registry_lock: | ||
| if name in _agent_factories: | ||
| raise ValueError(f"Agent '{name}' already registered") | ||
|
|
||
| _agent_factories[name] = AgentFactory( | ||
| factory_func=factory_func, description=description | ||
| factory_func=factory_func, | ||
| definition=definition, | ||
| ) | ||
|
|
||
|
|
||
| def register_agent_if_absent( | ||
| name: str, | ||
| factory_func: Callable[["LLM"], "Agent"], | ||
| description: str, | ||
| definition: AgentDefinition, | ||
VascoSch92 marked this conversation as resolved.
Show resolved
Hide resolved
VascoSch92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) -> bool: | ||
| """ | ||
| Register a custom agent if no agent with that name exists yet. | ||
|
|
@@ -93,20 +110,19 @@ def register_agent_if_absent( | |
| with programmatically registered agents. | ||
VascoSch92 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| Args: | ||
| name: Unique name for the agent | ||
| factory_func: Function that takes an LLM and returns an Agent | ||
| description: Human-readable description of what this agent does | ||
| definition: AgentDefinition describing the agent. | ||
|
|
||
| Returns: | ||
| True if the agent was registered, False if an agent with that name | ||
| already existed. | ||
| """ | ||
| with _registry_lock: | ||
| if name in _agent_factories: | ||
| if definition.name in _agent_factories: | ||
| return False | ||
|
|
||
| _agent_factories[name] = AgentFactory( | ||
| factory_func=factory_func, description=description | ||
| _agent_factories[definition.name] = AgentFactory( | ||
| factory_func=factory_func, definition=definition | ||
| ) | ||
| return True | ||
|
|
||
|
|
@@ -200,11 +216,11 @@ def register_file_agents(work_dir: str | Path) -> list[str]: | |
| registered: list[str] = [] | ||
| for agent_def in deduplicated: | ||
| factory = agent_definition_to_factory(agent_def) | ||
| was_registered = register_agent_if_absent( | ||
| name=agent_def.name, | ||
| factory_func=factory, | ||
| description=agent_def.description or f"File-based agent: {agent_def.name}", | ||
| ) | ||
| if not agent_def.description: | ||
| agent_def = agent_def.model_copy( | ||
| update={"description": f"File-based agent: {agent_def.name}"} | ||
| ) | ||
| was_registered = register_agent_if_absent(factory, agent_def) | ||
| if was_registered: | ||
| registered.append(agent_def.name) | ||
| logger.info( | ||
|
|
@@ -232,11 +248,11 @@ def register_plugin_agents(agents: list[AgentDefinition]) -> list[str]: | |
| registered: list[str] = [] | ||
| for agent_def in agents: | ||
| factory = agent_definition_to_factory(agent_def) | ||
| was_registered = register_agent_if_absent( | ||
| name=agent_def.name, | ||
| factory_func=factory, | ||
| description=agent_def.description or f"Plugin agent: {agent_def.name}", | ||
| ) | ||
| if not agent_def.description: | ||
| agent_def = agent_def.model_copy( | ||
| update={"description": f"Plugin agent: {agent_def.name}"} | ||
| ) | ||
| was_registered = register_agent_if_absent(factory, agent_def) | ||
| if was_registered: | ||
| registered.append(agent_def.name) | ||
| logger.info(f"Registered plugin agent '{agent_def.name}'") | ||
|
|
@@ -294,11 +310,17 @@ def get_factory_info() -> str: | |
| return "\n".join(info_lines) | ||
|
|
||
| for name, factory in sorted(user_factories.items()): | ||
| info_lines.append(f"- **{name}**: {factory.description}") | ||
| info_lines.append(f"- **{name}**: {factory.definition.description}") | ||
|
|
||
| return "\n".join(info_lines) | ||
|
|
||
|
|
||
| def get_registered_agent_definitions() -> list[AgentDefinition]: | ||
| """Return stored AgentDefinitions for forwarding to remote servers.""" | ||
| with _registry_lock: | ||
| return [f.definition for f in _agent_factories.values()] | ||
|
|
||
|
|
||
| def _reset_registry_for_tests() -> None: | ||
| """Clear the registry for tests to avoid cross-test contamination.""" | ||
| with _registry_lock: | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a new test to tests/cross/test_remote_conversation_live_server.py that spawns up a REAL agent server with the real sub-agent requests, and make sure it works?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found that I didn't see that before because the machinery to load subagents is using I solved this by adding a Having this as a classmethod on Why this solution? This keeps Everything is tested, and also cross tested. |
Uh oh!
There was an error while loading. Please reload this page.