Skip to content

Commit 22172db

Browse files
refactor: simplify agent update logic with clearer conditionals
Refactor _ensure_plugins_loaded to use explicit has_mcp_config flag and direct self._plugin_specs checks instead of the agent_update_needed flag. This provides clearer intent while maintaining correctness and proper type safety. The agent update is now conditionally executed only when there are plugins or MCP config to process, avoiding unnecessary model_copy calls that could interfere with agent initialization. Also adds comments explaining the keyword-only * syntax (PEP 3102) in expand_mcp_variables and load_mcp_config functions to prevent ambiguous positional boolean arguments. Per review feedback from @neubig.
1 parent 5c7ab97 commit 22172db

2 files changed

Lines changed: 18 additions & 12 deletions

File tree

openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,9 @@ def _ensure_plugins_loaded(self) -> None:
332332
merged_context = self.agent.agent_context
333333
merged_mcp = dict(self.agent.mcp_config) if self.agent.mcp_config else {}
334334

335+
# Track whether we have plugins or MCP config to process
336+
has_mcp_config = bool(merged_mcp)
337+
335338
# Load plugins if specified
336339
if self._plugin_specs:
337340
logger.info(f"Loading {len(self._plugin_specs)} plugin(s)...")
@@ -359,6 +362,7 @@ def _ensure_plugins_loaded(self) -> None:
359362
# Merge plugin contents
360363
merged_context = plugin.add_skills_to(merged_context)
361364
merged_mcp = plugin.add_mcp_config_to(merged_mcp)
365+
has_mcp_config = has_mcp_config or bool(merged_mcp)
362366

363367
# Collect hooks
364368
if plugin.hooks and not plugin.hooks.is_empty():
@@ -387,17 +391,19 @@ def _ensure_plugins_loaded(self) -> None:
387391
else:
388392
logger.debug("Applied MCP config defaults (no secrets provided)")
389393

390-
# Update agent with merged content (context from plugins, expanded MCP config)
391-
self.agent = self.agent.model_copy(
392-
update={
393-
"agent_context": merged_context,
394-
"mcp_config": merged_mcp,
395-
}
396-
)
394+
# Update agent with merged content only if we have plugins or MCP config
395+
# Skip update when nothing changed to avoid unnecessary agent state mutations
396+
if self._plugin_specs or has_mcp_config:
397+
self.agent = self.agent.model_copy(
398+
update={
399+
"agent_context": merged_context,
400+
"mcp_config": merged_mcp,
401+
}
402+
)
397403

398-
# Also update the agent in _state so API responses reflect loaded plugins
399-
with self._state:
400-
self._state.agent = self.agent
404+
# Also update the agent in _state so API responses reflect loaded plugins
405+
with self._state:
406+
self._state.agent = self.agent
401407

402408
# Register file-based agents defined in plugins
403409
if all_plugin_agents:

openhands-sdk/openhands/sdk/skills/utils.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ def expand_mcp_variables(
6969
config: dict,
7070
variables: dict[str, str],
7171
secrets: dict[str, str] | None = None,
72-
*,
72+
*, # no positional booleans (PEP 3102)
7373
expand_defaults: bool = True,
7474
) -> dict:
7575
"""Expand variables in MCP configuration.
@@ -128,7 +128,7 @@ def load_mcp_config(
128128
mcp_json_path: Path,
129129
skill_root: Path | None = None,
130130
secrets: dict[str, str] | None = None,
131-
*,
131+
*, # no positional booleans (PEP 3102)
132132
expand_defaults: bool = True,
133133
) -> dict:
134134
"""Load and parse .mcp.json with variable expansion.

0 commit comments

Comments
 (0)