diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 45d788c12..9233ed6f9 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -69,9 +69,11 @@ jobs: uses: anomalyco/opencode/github@latest env: DEEPSEEK_API_KEY: ${{ secrets.DEEPSEEK_API_KEY }} + GITHUB_TOKEN: ${{ github.token }} with: agent: review-lead model: deepseek/deepseek-v4-flash + use_github_token: true prompt: | Review this pull request. Read the AGENTS.md file first for project conventions, then analyze the diff and delegate to your specialist diff --git a/changelog/unreleased/2026-08-21-pr384-review-fixes.md b/changelog/unreleased/2026-08-21-pr384-review-fixes.md new file mode 100644 index 000000000..b480e9d81 --- /dev/null +++ b/changelog/unreleased/2026-08-21-pr384-review-fixes.md @@ -0,0 +1,18 @@ +# Post-merge review fixes for PR #384 (tool-schema-overlap capability) + +Fixes five issues identified during code review of the merged tool-schema-overlap +capability PR: + +- **P0 — Resource leak**: `BaseAgent.__aexit__` now calls `__aexit__` on + `_LifecycleCapable` instances in `_external_capabilities`, closing direct + `MCPClient` connections created by the `_ensure_client` fallback that were + previously never cleaned up. +- **P1 — Internal URL**: Replaced the hardcoded internal Sany URL in + `examples/kb_diag_agent.yaml` with `https://mcp.example.com/…`. +- **P1 — Unhandled exception**: `_get_mcp_server_info` now catches and logs + errors from `cap.list_tools()` on config-defined MCP capabilities instead + of crashing the entire status endpoint when a client disconnects. +- **P2 — Enum type limitation**: `ParamOverride.enum` relaxed from + `list[str]` to `list[Any]` so integer/number enums are configurable. +- **P3 — Code duplication**: Added "intentional duplication" comments to + `_AliasLoader` in all 10 `agentpool*` shim modules. diff --git a/examples/kb_diag_agent.yaml b/examples/kb_diag_agent.yaml index bdeb36d67..20e05760d 100644 --- a/examples/kb_diag_agent.yaml +++ b/examples/kb_diag_agent.yaml @@ -18,7 +18,7 @@ agents: config: type: streamable-http name: knowledge_diag - url: https://sprouts-dev-app-frontend.sany.com.cn/api-mcp/knowledge_diag/mcp + url: https://mcp.example.com/knowledge_diag/mcp # 去掉 search_kb 接口:若仍需保留该工具,删除下一行即可 disabled_tools: - search_kb diff --git a/src/agentpool/__init__.py b/src/agentpool/__init__.py index ac56391f8..a7686bb27 100644 --- a/src/agentpool/__init__.py +++ b/src/agentpool/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_bot/__init__.py b/src/agentpool_bot/__init__.py index bc79094b5..df68ec7ed 100644 --- a/src/agentpool_bot/__init__.py +++ b/src/agentpool_bot/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_bot, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_cli/__init__.py b/src/agentpool_cli/__init__.py index ff972f2b6..30be548f1 100644 --- a/src/agentpool_cli/__init__.py +++ b/src/agentpool_cli/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_cli, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_commands/__init__.py b/src/agentpool_commands/__init__.py index 1855dd2d6..61018741e 100644 --- a/src/agentpool_commands/__init__.py +++ b/src/agentpool_commands/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_commands, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_config/__init__.py b/src/agentpool_config/__init__.py index e6bcfdaf8..924bdd305 100644 --- a/src/agentpool_config/__init__.py +++ b/src/agentpool_config/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_config, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_prompts/__init__.py b/src/agentpool_prompts/__init__.py index 9bce7da7d..5b6da2397 100644 --- a/src/agentpool_prompts/__init__.py +++ b/src/agentpool_prompts/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_prompts, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_server/__init__.py b/src/agentpool_server/__init__.py index 697849d32..badf140b2 100644 --- a/src/agentpool_server/__init__.py +++ b/src/agentpool_server/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_server, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_storage/__init__.py b/src/agentpool_storage/__init__.py index 487f79047..95b89f14f 100644 --- a/src/agentpool_storage/__init__.py +++ b/src/agentpool_storage/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_storage, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_sync/__init__.py b/src/agentpool_sync/__init__.py index 2579bf634..2bb6c83e6 100644 --- a/src/agentpool_sync/__init__.py +++ b/src/agentpool_sync/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_sync, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/agentpool_toolsets/__init__.py b/src/agentpool_toolsets/__init__.py index ecbfd8efe..82e226612 100644 --- a/src/agentpool_toolsets/__init__.py +++ b/src/agentpool_toolsets/__init__.py @@ -34,6 +34,11 @@ def __getattr__(name: str) -> object: return getattr(wolfharness_toolsets, name) +# NOTE: _AliasLoader is intentionally duplicated across all agentpool* shim +# modules. These shims redirect to wolfharness* and must work before +# wolfharness is imported, so the loader cannot be shared from a common +# utility module. This duplication will be removed when the deprecated +# shims are dropped. class _AliasLoader(abc.Loader): """Loader that returns an already-imported target module as-is. diff --git a/src/wolfharness/agents/base_agent.py b/src/wolfharness/agents/base_agent.py index 61af960ca..61fa85975 100644 --- a/src/wolfharness/agents/base_agent.py +++ b/src/wolfharness/agents/base_agent.py @@ -40,6 +40,7 @@ from collections.abc import AsyncGenerator from contextvars import Token from datetime import datetime + from types import TracebackType from evented_config import EventConfig from exxec import ExecutionEnvironment @@ -576,11 +577,20 @@ async def _get_mcp_server_info(self) -> dict[str, MCPServerStatus]: # Only check cap.client (no lazy connection) to avoid # triggering _ensure_client() during status reporting. if cap.client is not None: - tool_entries = await cap.list_tools() - tools = [t.name for t in tool_entries] - info = cap.client.server_info - server_name = info.get("name") if info else None - server_version = info.get("version") if info else None + try: + tool_entries = await cap.list_tools() + tools = [t.name for t in tool_entries] + info = cap.client.server_info + server_name = info.get("name") if info else None + server_version = info.get("version") if info else None + except Exception: + logger.exception( + "Failed to list tools from config-defined MCP capability", + extra={"capability": cap}, + ) + tools = [] + server_name = None + server_version = None else: tools = [] server_name = None @@ -717,6 +727,37 @@ async def _with_session_capabilities( finally: self._session_capabilities.clear() + async def __aexit__( + self, + exc_type: type[BaseException] | None, + exc_val: BaseException | None, + exc_tb: TracebackType | None, + ) -> None: + """Clean up external capabilities before base cleanup. + + Config-defined capabilities (e.g. ``type: mcp`` created via + ``EntryPointCapabilityConfig.build()``) are stored in + ``_external_capabilities`` and never registered with an + ``MCPManager``. Their async context manager ``__aexit__`` is + called here so that resources like direct ``MCPClient`` + connections (created by the ``_ensure_client`` fallback) are + properly closed instead of leaking on process exit. + """ + from wolfharness.capabilities.combined_toolset import _LifecycleCapable + + for cap in self._external_capabilities: + if not isinstance(cap, _LifecycleCapable): + continue + try: + await cap.__aexit__(exc_type, exc_val, exc_tb) + except Exception: + logger.warning( + "Error during capability cleanup", + exc_info=True, + extra={"capability": cap}, + ) + await super().__aexit__(exc_type, exc_val, exc_tb) + @asynccontextmanager async def _temporary_tools( self, diff --git a/src/wolfharness/capabilities/tool_schema_overlap_config.py b/src/wolfharness/capabilities/tool_schema_overlap_config.py index a6566548a..53015ca10 100644 --- a/src/wolfharness/capabilities/tool_schema_overlap_config.py +++ b/src/wolfharness/capabilities/tool_schema_overlap_config.py @@ -119,7 +119,7 @@ class ParamOverride(BaseModel): name: NonEmptyStr | None = None description: str | None = None type: str | None = None - enum: list[str] | None = None + enum: list[Any] | None = None required: bool | None = None default: Any = UNDEFINED