Skip to content

feat: implement Unified Model Selection Configuration - #5

Closed
Leoyzen wants to merge 1 commit into
mainfrom
feature/model_varient_selection
Closed

feat: implement Unified Model Selection Configuration#5
Leoyzen wants to merge 1 commit into
mainfrom
feature/model_varient_selection

Conversation

@Leoyzen

@Leoyzen Leoyzen commented Feb 13, 2026

Copy link
Copy Markdown
Collaborator

Unified Model Selection Configuration

Summary

This PR implements unified model selection support across AgentPool's protocol servers. Users can now define custom model variants in their manifest configuration, which will be exposed through both OpenCode and ACP server endpoints.

What's New

Shared Model Utilities (src/agentpool_server/shared/)

  • New shared module for model-related helper functions
  • extract_provider(): Extract provider names from any model configuration type (OpenAI, Anthropic, Gemini, etc.)
  • build_providers_from_tokonomics(): Convert tokonomics discovery results into server-compatible provider objects
  • apply_configured_variants(): Merge user-defined model variants with auto-discovered models

OpenCode Server Updates (opencode_server/routes/config_routes.py)

  • Implements intelligent fallback hierarchy for model discovery:
    1. User-defined model_variants from manifest (preferred)
    2. Dynamic discovery via tokonomics
    3. Agent-specific modes (Codex/Claude thought levels)
    4. Empty list with warning (last resort)
  • Removed hardcoded dummy provider fallback
  • Configured variants take precedence over discovered models with identical IDs

ACP Server Updates (acp_server/acp_agent.py)

  • get_session_model_state(): Now includes configured variants in session state responses
  • set_session_model(): Validates model IDs against both tokonomics discovery and configured variants
  • User-defined variants override auto-discovered models with matching IDs

Example Configuration

agents:
  my_agent:
    type: native
    model: "openai:gpt-4o"

model_variants:
  fast:
    type: string
    identifier: "openai:gpt-4o-mini"
  creative:
    type: anthropic
    identifier: "claude-sonnet-4"
  reliable:
    type: fallback
    models:
      - type: openai
        identifier: "gpt-4o"
      - type: anthropic  
        identifier: "claude-opus-4"

Key Behaviors

  • Precedence: Configured variants override discovered models with the same ID
  • Fallback: When no variants are configured, servers gracefully fall back to automatic discovery
  • Backward Compatible: Existing configurations continue to work without changes
  • Type-Safe: Full pattern matching implementation without runtime attribute access

Testing

  • 29 comprehensive unit tests covering all model configuration types
  • Tests for provider extraction, provider building, and variant merging
  • Integration tests for end-to-end workflows

Files Changed: 5
Additions: 788 lines
Deletions: 71 lines

@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello @Leoyzen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the AgentPool's model selection capabilities by introducing a unified configuration system. It allows users to define and prioritize custom model variants through manifest files, which are then seamlessly integrated and respected across both OpenCode and ACP server endpoints. This change streamlines model management, improves flexibility, and ensures that user-defined preferences take precedence over dynamically discovered models, all while maintaining backward compatibility.

Highlights

  • Unified Model Selection: Implemented a unified system for model selection across AgentPool's protocol servers (OpenCode and ACP), allowing users to define custom model variants in their manifest configurations.
  • Precedence Logic: Established a clear precedence hierarchy where user-defined model variants override auto-discovered models with identical IDs, ensuring custom configurations are respected.
  • Shared Model Utilities: Introduced a new shared module (src/agentpool_server/shared/model_utils.py) containing reusable functions for model provider extraction, tokonomics integration, and variant merging, promoting code reuse and consistency.
  • OpenCode Server Enhancements: Updated the OpenCode server to use an intelligent fallback mechanism for model discovery, prioritizing configured variants, then tokonomics, and finally agent-specific modes, removing previous hardcoded dummy providers.
  • ACP Server Integration: Modified the ACP server to incorporate configured variants into session model state retrieval and validation during model setting, ensuring consistency with the new unified model selection.
  • Backward Compatibility: Ensured that existing configurations continue to function without requiring any changes, minimizing disruption for current users.
  • Comprehensive Testing: Added 29 unit tests covering the new model configuration types and logic, ensuring robustness and correctness of the unified model selection system.
Changelog
  • src/agentpool_server/acp_server/acp_agent.py
    • Updated get_session_model_state to fetch and merge configured model variants from the manifest, giving them precedence over tokonomics-discovered models.
    • Modified set_session_model to validate requested model IDs against both dynamically discovered models and user-defined variants in the manifest.
  • src/agentpool_server/opencode_server/routes/config_routes.py
    • Imported logging, Any, AgentsManifest, and new shared model utility functions.
    • Added new asynchronous functions (_get_configured_variants, _build_providers_from_configured, _build_providers_from_variants, _build_providers_with_fallback) to implement the model discovery fallback hierarchy.
    • Refactored get_providers and list_providers endpoints to utilize the new _build_providers_with_fallback logic, removing the previous dummy provider fallback.
    • Removed the _get_dummy_providers function.
  • src/agentpool_server/shared/init.py
    • Created an __init__.py file to define the shared directory as a Python package.
  • src/agentpool_server/shared/model_utils.py
    • Introduced helper functions: _extract_provider_from_identifier to parse provider names from model IDs, _extract_provider to determine provider from various model config types, _build_providers_from_tokonomics to convert tokonomics data into provider objects, and _apply_configured_variants to merge user-defined variants into provider lists.
  • tests/agentpool_server/shared/test_model_utils.py
    • Added new test file with comprehensive unit tests for _extract_provider_from_identifier, _extract_provider, _build_providers_from_tokonomics, and _apply_configured_variants functions.
Activity
  • No human activity (comments, reviews, etc.) has been recorded on this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a unified model selection configuration, which is a significant and well-executed feature. The introduction of a shared model_utils module is a great step towards better code organization and reuse. The new fallback hierarchy for model discovery in the OpenCode server is a robust improvement over the previous implementation that relied on dummy data. The changes in the ACP server are consistent and correctly apply the new unified logic. The comprehensive test suite for the new utilities is also commendable.

I have a couple of suggestions regarding hardcoded values that are duplicated across files. Addressing these would further improve the maintainability of the new code. Additionally, the refactoring seems to have left the _apply_variants_to_providers function in config_routes.py unused, which could be removed.

Comment thread src/agentpool_server/opencode_server/routes/config_routes.py Outdated
Comment thread src/agentpool_server/shared/model_utils.py Outdated
Implement unified model selection across AgentPool protocol servers (ACP, OpenCode)
to respect model_variants configuration from manifest.yml.

Changes:
- Add shared model utilities (src/agentpool_server/shared/model_utils.py)
  - _extract_provider(): Extract provider from AnyModelConfig
  - _build_providers_from_tokonomics(): Build providers from discovery
  - _apply_configured_variants(): Merge configured with discovered models

- Update OpenCode server (config_routes.py)
  - Implement 4-tier fallback: config → tokonomics → agent modes → empty
  - Remove hardcoded _get_dummy_providers()
  - Add _get_configured_variants() and _build_providers_with_fallback()

- Update ACP server (acp_agent.py)
  - get_session_model_state(): Include configured variants in session state
  - set_session_model(): Validate against both tokonomics and configured variants

- Add comprehensive tests (test_model_utils.py, 29 tests)

Configured variants take precedence over discovered models with the same ID.
Backward compatible: existing configs work unchanged.
@Leoyzen
Leoyzen force-pushed the feature/model_varient_selection branch from 02de7e5 to 6eef318 Compare February 14, 2026 07:11
@Leoyzen Leoyzen closed this Apr 30, 2026
Leoyzen added a commit that referenced this pull request May 27, 2026
- _swap_session_agent: Update session agent registry after swap (review #5)
- get_agent_role_config_option: Use display_name with type-safe fallback, add description (review #7)
- list_modes: Use mode.id for programmatic identifiers, add explicit None guard (review #4, #8)
- Update tests to match new behavior
Leoyzen added a commit that referenced this pull request May 27, 2026
* docs(rfc): add RFC-0034 ACP Session Config Options 统一化

新增 RFC-0034,提案升级 AgentPool ACP Server 的 Session Config Options
透出逻辑,使 Zed 等 ACP 兼容 IDE 能够选择模型和切换 Agent Role。

主要内容:
- 识别 4 个 GAP:Agent Role 未透出(P0)、ACP/OpenCode model list
  数据来源不一致(P1)、/mode 路由硬编码(P1)、get_session_mode_state
  过滤过严(P2)
- 分析 3 个方案,推荐选项 2(三阶段统一化)
- 技术设计:build_model_state_for_acp()、get_agent_role_config_option()、
  _swap_session_agent() 及 OpenCode /mode 路由动态修复

🤖 Generated with [Qoder][https://qoder.com]

* docs(rfc): 根据 review 反馈修正 RFC-0034

- 修正 model fallback 逻辑:strict fallback(configured 存在时只用 configured)
- 修正 agent_role current_value:使用 agent.name 而非 pool.main_agent.name
- 重写 _swap_session_agent():委托 session.switch_active_agent() + _session_agent_locks 保护
- 增加 session._task_lock 协调:拒绝 active prompt 期间的 swap
- 增加 pool.manifest null check 保护
- 修正 list_modes() null safety:state.agent 为 None 时返回默认值
- 明确开放问题 Q2/Q3 的决策:对话历史不继承、current_value 已修复
- 更新决策记录:增加 session mutation 复用、锁保护、task_lock 协调、对话历史决策
- 更新 Phase 2 实施计划:增加 Zed 预验证、并发测试、current_value 测试
- 调整工作量估算:~260 行 → ~240 行

* docs(rfc): RFC-0034 新增 Phase 0 — ACP Configurable LLM Providers 适配

ACP PR #648 (Configurable LLM Providers) 已 MERGED,引入
providers/list、providers/set、providers/disable 三个方法族,
允许客户端发现和覆盖 agent 的 LLM 请求路由。

主要更新:
- 新增 GAP 5 (P0): providers/* 完全未实现
- 新增目标 G7: 实现 ACP providers/* 协议方法
- 新增 Phase 0: ProviderRouter 实现 + schema 类型定义 +
  ACP 请求处理器 + AgentCapabilities.providers 声明
- 修订 Phase 1: build_model_state_for_acp() 接受 provider_router
  参数,过滤被禁用 provider 下的模型
- 更新架构概览图: 传输层(providers)与应用层(session config)分层
- 更新里程碑: 四阶段实施,Phase 0 优先于 Phase 1
- 新增开放问题 6/7/8: providers 对已运行 session 的影响、
  provider 路由覆盖与 agent 初始化兼容、SessionModelState
  中是否携带 provider 关联信息
- 新增决策记录: providers/set 保守策略、从 model_variants
  派生 ProviderInfo、provider_router 参数解耦

🤖 Generated with [Qoder][https://qoder.com]

* docs(rfc): 优化 RFC-0034 — 补充 Zed 源码级兼容性分析

基于 Zed 源码调研(crates/agent_ui/src/config_options.rs、profile_selector.rs、
agent_servers/src/acp.rs)的关键发现:

1. Zed 渲染所有 config_options 为独立 UI 按钮,agent_role 可正确显示和点击
2. first_config_option_id() 仅返回同 category 的第一个 option,键盘快捷键
   可能冲突 — 标记为已知限制(NG7)
3. Zed ProfileSelector 完全独立于 ACP,使用本地 AgentSettings.profiles
4. Zed 当前完全不支持 providers/* 协议(Phase 0 暂无 Zed UI 入口)

RFC 更新内容:
- 新增 Zed IDE 渲染行为小节(源码级证据)
- 新增 Zed 兼容性分析总结表
- 更新非目标 NG7:键盘快捷键冲突为已知限制
- 更新开放问题 5/6/7/8/9,标记 Zed 调研结论
- 更新 Phase 2 预验证:明确键盘限制和排序建议
- 更新向后兼容保证表:添加 category 冲突行
- 更新决策记录:补充 Zed 调研证据

🤖 Generated with [Qoder][https://qoder.com]

* feat(acp): implement RFC-0034 ACP Session Config Options unification

Phase 0: ACP Configurable LLM Providers
- Add providers/* protocol methods (providers/list, providers/set, providers/disable)
- Add ProviderRouter with override/disable/capability tracking
- Add providers field to AgentCapabilities and InitializeResponse

Phase 1: Shared Model List Logic
- Add build_model_state_for_acp() with configured-first, tokonomics-fallback
- Invert get_session_model_state() to use configured variants first

Phase 2: Agent Role Config Option
- Add get_agent_role_config_option() exposing pool.all_agents
- Add _swap_session_agent() with lock protection
- Extend set_session_config_option() with agent_role handling

Phase 3: OpenCode /mode Route Fix
- Dynamic /mode route using agent.get_modes()

Also includes RFC-0033 MCP over ACP support:
- Add AcpMcpServer type and acp field to McpCapabilities
- Add acp_mcp_servers parameter to AgentCapabilities.create()

Tests:
- 35 new tests across provider_router, model_state, agent_role,
  config_routes, and cross-protocol integration
- Snapshot tests re-baselined

* chore: remove RFC-0033 code from RFC-0034 branch

Remove accidentally included RFC-0033 MCP-over-ACP implementation:
- Delete acp_mcp_manager.py, acp_mcp_transport.py
- Delete RFC-0033 tests (test_mcp.py, test_acp_mcp_*, test_mcp_integration)
- Remove AcpMcpServer from mcp.py
- Remove acp field from McpCapabilities
- Remove acp_mcp_servers parameter from AgentCapabilities.create()
- Remove acp_mcp_servers parameter from InitializeResponse.create()
- Remove RFC-0033 handler code from acp_agent.py

Keep RFC-0034 changes intact:
- providers/* protocol methods
- ProviderRouter with override/disable
- build_model_state_for_acp() configured-first logic
- agent_role config option and swap
- Dynamic /mode route

* fix: address PR review comments for RFC-0034

- _swap_session_agent: Update session agent registry after swap (review #5)
- get_agent_role_config_option: Use display_name with type-safe fallback, add description (review #7)
- list_modes: Use mode.id for programmatic identifiers, add explicit None guard (review #4, #8)
- Update tests to match new behavior

* fix(agent): use model_variants in get_modes() instead of tokonomics

Agent.get_modes() was calling get_available_models() which returns
all tokonomics-discovered models (2000+). Now it checks configured
model_variants first and only falls back to tokonomics when no
variants are configured.

Fixes the issue where config_options model selector showed thousands
of models instead of the configured variants.

* fix(agent): track model variant name to fix Zed Unknown display

When using model_variants, get_modes() returned variant names as option ids
but current_mode_id was the raw model identifier (e.g. openai:svc/glm-4.7).
This caused Zed to display 'Unknown' because current_mode_id didn't match
any available mode id.

Fix: Add _current_model_variant field to Agent. When _set_mode() is called
with a variant name, store it. get_modes() now uses _current_model_variant
as current_mode_id so it matches the option ids.

* fix(agent): set _current_model_variant on init when model is variant name

Agent.__init__ resolves model string via _resolve_model_string(), but
was not setting _current_model_variant. This caused get_modes() to fall
back to self.model_name (the raw model identifier) on initial load,
showing 'Unknown' in Zed until _set_mode() was called.

Fix: Also track variant name in __init__ when model string matches a
model_variants key.

* fix(agent): use actual model identifier as mode id, variant name as display name

Redesign model config option to use actual model identifiers:
- id/value: actual model identifier (e.g. openai:svc/glm-4.7)
- name: variant name (e.g. glm47) for display
- current_mode_id: actual model identifier

This ensures currentValue matches option values in Zed's config option
selector, fixing the 'Unknown' display issue.

_set_mode() now supports both actual model identifiers and variant names
by reverse-lookup from manifest model_variants.

* fix(agent): align get_modes() id format with model_name

Use config.get_model().system:model_name for option ids instead of
config.identifier, ensuring currentValue matches option values.

Root cause: model_name returns pydantic-ai system:model_name format
(e.g., 'openai:svc/glm-4.7') while config.identifier returns full
provider format (e.g., 'openai-chat:svc/glm-4.7'), causing mismatch
in Zed's model selector dropdown.

* fix: address PR #37 review comments (round 2)

- providers/set & providers/disable: 兼容 id 字段(Comment #12, #13)
- provider_router: 防御性初始化 + 未知 provider 静默禁用(Comment #14, #15)
- model_utils: 先过滤 raw toko_models(更准确),current_model 不在列表时插入(Comment #16)
- .gitignore: 添加 .omo/(Comment #18
Leoyzen added a commit that referenced this pull request Jul 1, 2026
…t exception

TDD-driven fixes for 3 issues found by Gemini Code Assist:

1. _signal_shutdown_locked race condition (CRITICAL):
   - stdio connections were not popped from _connections until after
     owner task exit (up to 10s window). Concurrent get_transport()
     could retrieve a dying connection.
   - Fix: always pop from _connections in _signal_shutdown_locked,
     regardless of transport type. Remove redundant pop in release()
     and _evict_if_needed().

2. HTTP/SSE ref count imbalance (HIGH):
   - get_transport() reused HTTP/SSE entry without incrementing
     ref_count, but release() decremented it. Multiple releases
     could drive ref_count negative.
   - Verified: current code pops HTTP/SSE in _signal_shutdown_locked
     so negative ref_count doesn't cause issues, but semantics
     are now correct with unified pop.

3. _build_toolset silent exception (MEDIUM):
   - get_tools() exceptions were silently swallowed, returning None
     with no logging. Operators had no visibility into missing tools.
   - Fix: add logger.warning() with provider name and exc_info=True.

Verified: #5 (Agent import in session.py) is NOT a bug — import
exists at line 28.

Tests: 5 new TDD tests in test_global_pool_review_fixes.py
(139 total pass, ruff clean)
Leoyzen added a commit that referenced this pull request Jul 1, 2026
…t exception

TDD-driven fixes for 3 issues found by Gemini Code Assist:

1. _signal_shutdown_locked race condition (CRITICAL):
   - stdio connections were not popped from _connections until after
     owner task exit (up to 10s window). Concurrent get_transport()
     could retrieve a dying connection.
   - Fix: always pop from _connections in _signal_shutdown_locked,
     regardless of transport type. Remove redundant pop in release()
     and _evict_if_needed().

2. HTTP/SSE ref count imbalance (HIGH):
   - get_transport() reused HTTP/SSE entry without incrementing
     ref_count, but release() decremented it. Multiple releases
     could drive ref_count negative.
   - Verified: current code pops HTTP/SSE in _signal_shutdown_locked
     so negative ref_count doesn't cause issues, but semantics
     are now correct with unified pop.

3. _build_toolset silent exception (MEDIUM):
   - get_tools() exceptions were silently swallowed, returning None
     with no logging. Operators had no visibility into missing tools.
   - Fix: add logger.warning() with provider name and exc_info=True.

Verified: #5 (Agent import in session.py) is NOT a bug — import
exists at line 28.

Tests: 5 new TDD tests in test_global_pool_review_fixes.py
(139 total pass, ruff clean)
Leoyzen added a commit that referenced this pull request Jul 2, 2026
…t exception

TDD-driven fixes for 3 issues found by Gemini Code Assist:

1. _signal_shutdown_locked race condition (CRITICAL):
   - stdio connections were not popped from _connections until after
     owner task exit (up to 10s window). Concurrent get_transport()
     could retrieve a dying connection.
   - Fix: always pop from _connections in _signal_shutdown_locked,
     regardless of transport type. Remove redundant pop in release()
     and _evict_if_needed().

2. HTTP/SSE ref count imbalance (HIGH):
   - get_transport() reused HTTP/SSE entry without incrementing
     ref_count, but release() decremented it. Multiple releases
     could drive ref_count negative.
   - Verified: current code pops HTTP/SSE in _signal_shutdown_locked
     so negative ref_count doesn't cause issues, but semantics
     are now correct with unified pop.

3. _build_toolset silent exception (MEDIUM):
   - get_tools() exceptions were silently swallowed, returning None
     with no logging. Operators had no visibility into missing tools.
   - Fix: add logger.warning() with provider name and exc_info=True.

Verified: #5 (Agent import in session.py) is NOT a bug — import
exists at line 28.

Tests: 5 new TDD tests in test_global_pool_review_fixes.py
(139 total pass, ruff clean)
Leoyzen added a commit that referenced this pull request Jul 2, 2026
… scoping (#88)

* fix(mcp): MCP provider lifecycle architecture — three-tier connection scoping

Complete overhaul of MCP provider lifecycle to fix subagent MCP tool
inheritance, cross-task CancelScope errors, and streaming adapter hangs.

Closes #70

- Frozen dataclass capturing pool/agent/session/skill MCP configs
- Child sessions inherit parent's session_configs at creation time
- Eliminates race condition between receive_request() and session/load

- Owner-task pattern for stdio (dedicated asyncio.Task enters/exits CM)
- _SharedSessionTransport wrapper: yields shared ClientSession without
  duplicate connect_session() calls
- HTTP/SSE: fresh transport per call (no stream contention)
- LRU eviction (MAX_SESSIONS=256), ref counting, threading.Lock

- (client_id, skill_name) keying for skill MCP isolation
- Owner-task for stdio, add_transport() for pre-created ACP transports
- copy_pre_created_transports(): child inherits parent's ACP transport

- Fresh MCPToolset per agentlet from snapshot + connection pools
- Bypasses agent.tools.providers for MCP (uses snapshot path)
- native=False to force local MCP tool fallback

- Per-session stream pairs in AcpMcpConnection (register_session())
- send_to_acp() routes response to caller's stream (no JSON-RPC id
  tracking — synchronous send_to_client naturally correlates)
- broadcast_to_sessions() for server-initiated notifications
- connect_session() finally only cleans up own pair, not connection's

- Moved yield outside anyio.create_task_group()
- Producer as asyncio.ensure_future(), consumer in main coroutine

- to_transport() on all config classes (Stdio/SSE/StreamableHTTP)
- Removed all to_pydantic_ai() methods
- as_capability() creates fresh MCPToolset per call (no caching)

- SkillMcpServerConfig.to_mcp_server_config() bridge method
- SkillCapability.get_toolset() reads from snapshot + SessionConnectionPool
- on_run_ended() uses isinstance instead of getattr

- 134 MCP server tests (45 unit + 42 integration + 47 existing)
- 3 transport reuse tests (concurrent connect_session, one-exit, 4-way)
- 3 GlobalConnectionPool sharing tests (HTTP, stdio, shared stdio)
- 17 cross-task lifecycle integration tests
- ruff clean, mypy clean on changed files

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(mcp): MCP provider lifecycle architecture — three-tier connection scoping

Complete overhaul of MCP provider lifecycle to fix subagent MCP tool
inheritance, cross-task CancelScope errors, and streaming adapter hangs.

Closes #70

- Frozen dataclass capturing pool/agent/session/skill MCP configs
- Child sessions inherit parent's session_configs at creation time
- Eliminates race condition between receive_request() and session/load

- Owner-task pattern for stdio (dedicated asyncio.Task enters/exits CM)
- _SharedSessionTransport wrapper: yields shared ClientSession without
  duplicate connect_session() calls
- HTTP/SSE: fresh transport per call (no stream contention)
- LRU eviction (MAX_SESSIONS=256), ref counting, threading.Lock

- (client_id, skill_name) keying for skill MCP isolation
- Owner-task for stdio, add_transport() for pre-created ACP transports
- copy_pre_created_transports(): child inherits parent's ACP transport

- Fresh MCPToolset per agentlet from snapshot + connection pools
- Bypasses agent.tools.providers for MCP (uses snapshot path)
- native=False to force local MCP tool fallback

- Per-session stream pairs in AcpMcpConnection (register_session())
- send_to_acp() routes response to caller's stream (no JSON-RPC id
  tracking — synchronous send_to_client naturally correlates)
- broadcast_to_sessions() for server-initiated notifications
- connect_session() finally only cleans up own pair, not connection's

- Moved yield outside anyio.create_task_group()
- Producer as asyncio.ensure_future(), consumer in main coroutine

- to_transport() on all config classes (Stdio/SSE/StreamableHTTP)
- Removed all to_pydantic_ai() methods
- as_capability() creates fresh MCPToolset per call (no caching)

- SkillMcpServerConfig.to_mcp_server_config() bridge method
- SkillCapability.get_toolset() reads from snapshot + SessionConnectionPool
- on_run_ended() uses isinstance instead of getattr

- 134 MCP server tests (45 unit + 42 integration + 47 existing)
- 3 transport reuse tests (concurrent connect_session, one-exit, 4-way)
- 3 GlobalConnectionPool sharing tests (HTTP, stdio, shared stdio)
- 17 cross-task lifecycle integration tests
- ruff clean, mypy clean on changed files

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix: ruff lint errors and remove debug trace logging

- Fix 8 ruff errors: TC003/TC002 (move imports to TYPE_CHECKING),
  E501 (line length), SIM105 (use contextlib.suppress), I001 (import sort)
- Remove ~20 debug logger.debug() trace calls from turn.py, run.py,
  core.py that were added during subagent hang diagnosis
- Keep exception-handling debug logs (RunAbortedError, tool kind map,
  usage extraction) — these are useful for production debugging

* fix: address Gemini code review — race condition, ref counting, silent exception

TDD-driven fixes for 3 issues found by Gemini Code Assist:

1. _signal_shutdown_locked race condition (CRITICAL):
   - stdio connections were not popped from _connections until after
     owner task exit (up to 10s window). Concurrent get_transport()
     could retrieve a dying connection.
   - Fix: always pop from _connections in _signal_shutdown_locked,
     regardless of transport type. Remove redundant pop in release()
     and _evict_if_needed().

2. HTTP/SSE ref count imbalance (HIGH):
   - get_transport() reused HTTP/SSE entry without incrementing
     ref_count, but release() decremented it. Multiple releases
     could drive ref_count negative.
   - Verified: current code pops HTTP/SSE in _signal_shutdown_locked
     so negative ref_count doesn't cause issues, but semantics
     are now correct with unified pop.

3. _build_toolset silent exception (MEDIUM):
   - get_tools() exceptions were silently swallowed, returning None
     with no logging. Operators had no visibility into missing tools.
   - Fix: add logger.warning() with provider name and exc_info=True.

Verified: #5 (Agent import in session.py) is NOT a bug — import
exists at line 28.

Tests: 5 new TDD tests in test_global_pool_review_fixes.py
(139 total pass, ruff clean)

* test: fix acp_server tests for new AcpMcpConnection API

Rewrite 5 test files to use the new per-session stream API:
- register_session() / unregister_session() instead of open()
- send_to_acp(message, response_stream) instead of send_to_client(message)
- pair.to_session_receive.receive() instead of conn.to_session.receive()
- handle_client_message broadcasts to all registered sessions
- Removed 6 tests for deleted properties (to_session, from_session, etc.)

46 tests pass (was 29 failed + 8 errors).

* chore: remove deployment config, add review comment annotations

- Remove xeno-agent/config/diag-agent-ng.yaml (deployment config,
  not framework code — contains private model endpoints)
- Add threading.Lock boundary comment in GlobalConnectionPool
- Add HTTP/SSE ref_count TODO in GlobalConnectionPool
- Add cleanup boundary comment in SessionConnectionPool.cleanup()
  (pre-created ACP transports are managed by AcpMcpConnectionManager)

* refactor(mcp): remove ref counting and LRU eviction from GlobalConnectionPool

Shared stdio connections now live for the pool lifetime — no ref
counting, no LRU eviction, no MAX_SESSIONS cap. Pool-level MCP servers
are typically few (<10) and long-lived, making per-connection tracking
unnecessary overhead.

Changes:
- Remove ref_count field, release() method, _evict_if_needed()
- Remove MAX_SESSIONS constant and LRU eviction logic
- Remove _signal_shutdown_locked() (inlined into shutdown_all())
- HTTP/SSE transports no longer cached in _connections dict
- Replace OrderedDict with plain dict (no LRU ordering needed)
- Remove _PooledConnection.is_stdio field (only stdio is cached)
- Add warning log when cached connections exceed 50
- Remove stale TODO about HTTP/SSE ref_count imbalance

Net: -145 lines from global_pool.py

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(mcp): update GlobalConnectionPool tests for simplified architecture

Remove tests for deleted APIs (release(), ref_count, MAX_SESSIONS,
_evict_if_needed, LRU eviction). Update remaining tests to reflect
that HTTP/SSE transports are no longer cached in _connections.

- test_global_pool.py: Remove 8 tests for release()/LRU, update
  HTTP/SSE tests to assert no caching, add fresh-transport test
- test_global_pool_review_fixes.py: Remove TestReleasePopsDyingConnection
  and TestHTTPRefCountBalance classes (-169 lines), keep
  TestBuildToolsetLogsWarning
- test_global_pool_sharing.py: Remove unused imports

Net: -441 lines across test files

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(mcp): use server.timeout for owner-task ready wait instead of hardcoded 30s

The owner-task ready wait timeout was hardcoded to 30s, ignoring the
server's configured timeout. Now uses config.timeout (default 30s) so
servers with longer startup times (e.g. npx wrappers) are not killed
prematurely.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(skills): extract session_id from deps directly when AgentContext not available

_ build_mcp_toolsets and on_run_ended were hardcoded to default or
returning early when ctx.deps was not an AgentContext instance.
This broke tests that use FakeDeps(session_id=...).

Fix: use getattr(ctx.deps, "session_id", "default") as fallback
before defaulting to "default" / returning early.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(mcp): remove dead stdio connections from pool on owner-task exit

If a stdio subprocess crashes, the owner task exits but the dead
connection was never removed from _connections. Subsequent
get_transport() calls would find the dead entry, detect
owner_task.exception() != None, and raise RuntimeError every
time — permanently poisoning the pool.

Fix: remove the connection from _connections in _run_session's
finally block (under lock, identity-checked against the pool's
current entry to avoid races with shutdown_all()).

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(skills): use AsyncMock for as_capability in skill capability tests

PR #88 changed as_capability() to async def, but test mocks still used MagicMock.return_value which returns a list instead of a coroutine. Switch to AsyncMock to properly await the call.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(delegation): remove deprecated _warn parameter from MCPManager init

PR #88 removed the _warn parameter from MCPManager.__init__(), but test_graph_teams.py still passed it. Drop the argument.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(opencode): mock MCP server iteration for session integration tests

PR #88 added new code paths in get_or_create_session_agent() that iterate pool.mcp.servers and cfg.get_mcp_servers(). The mock AgentPool didn't configure these attributes, causing TypeError: 'Mock' object is not iterable. Set both to empty lists.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(compat): remove deprecated _warn parameter from MCPManager compat test

PR #88 removed the _warn parameter from MCPManager.__init__(), but the backward compatibility test still passed it.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(delegation): use agent_name instead of agent in ACPSessionManager.create_session

PR #88 changed ACPSessionManager.create_session() to accept agent_name: str instead of agent: Agent. Update the test call accordingly.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* style(mcp): fix ruff format in changed files

Fix formatting issues in manager.py, session_pool.py, and capability.py introduced by PR #88 changes.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(agentlet): add AsyncMock for provider.get_tools in capability tests

Mock providers were missing get_tools as AsyncMock, causing 'object MagicMock can't be used in await expression' errors during get_agentlet(). The errors were caught but generated noise and could trigger 'Event loop is closed' on CI.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* style: fix ruff lint and format errors in PR #88 changed files

Fix all remaining ruff check and ruff format errors in files changed by PR #88 after rebasing onto develop/agentic (which includes lint fix #90). Errors fixed: G004 (f-string logging), E501 (line length), D205 (docstring format), SIM102/SIM117 (if nesting), PERF401 (async comprehension), TRY300/TRY301 (else block), TRY004 (TypeError vs RuntimeError), and pragmatic noqa for PLR0915/PLR0911/BLE001 on complex functions.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix: resolve mypy type errors and fix test_session_manager_with_mcp on Linux

Fix 43 mypy errors across 9 files: missing type annotations in session_stream_pair.py, incompatible types in core.py, GraphRun generic args in streaming_adapter.py, and BaseAgent attribute access patterns. Also fix test_session_manager_with_mcp which was skipped on macOS but failed on Linux CI due to AgentPool() created without main_agent_name.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(test): register agent config in runtime registry for MCP integration tests

test_session_manager_with_mcp and test_session_with_mcp_servers were skipped on macOS but failed on Linux CI. AgentPool() was created without any agents in the manifest, and create_session() could not find the agent config. Register a NativeAgentConfig in the runtime registry to fix. Also fix ruff format.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(test): access runtime_registry via session_pool after async with

runtime_registry is on SessionPool, not AgentPool. Move register() call inside 'async with agent_pool:' block where session_pool is initialized. Remove unnecessary register() from test_session_with_mcp_servers which creates ACPSession directly.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(test): access runtime_registry via session_pool.sessions

runtime_registry is on SessionController (SessionPool.sessions), not SessionPool directly.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

---------

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Leoyzen added a commit that referenced this pull request Jul 7, 2026
…apability

Review round 2 fixes:

Fix #4 (Critical): Wire _acp_mcp_manager in ACPSession.__post_init__
- MCPManager._acp_mcp_manager was initialized to None and never set
- cleanup_session() could never delegate to AcpMcpConnectionManager
- Per-session ACP stream pairs and reverse-index entries leaked
- Fix: wire agent.mcp._acp_mcp_manager = acp_agent._mcp_manager in __post_init__

Fix #5 (Medium): Identity check after acquiring cleanup lock
- Concurrent cleanup_session() callers could do redundant work
- All ops were idempotent but wasteful (clearing empty dicts, etc.)
- Fix: check if self._session_contexts.get(session_id) is not ctx after lock

Fix #6 (Medium): Consolidate duplicated fallback in as_capability()
- Three identical 'for server in self.servers:' loops consolidated to one
- Pure readability refactor, zero behavior change

TDD: 3 new tests (2 RED before fix, 3 GREEN after)
- test_cleanup_session_delegates_to_acp_mcp_manager (unit)
- test_acp_session_wires_acp_mcp_manager (integration)
- test_cleanup_session_identity_check_prevents_redundant_work (unit)

213 tests pass, ruff clean.
Leoyzen added a commit that referenced this pull request Jul 8, 2026
* spec: MCP session lifecycle fix — Phase 1

Add OpenSpec change for fixing stale MCP toolset cache and session-scoped
resource lifecycle bugs (#121). Includes:

- proposal.md: What & why (6 lifecycle fixes, no config changes)
- design.md: 8 design decisions (D1-D8) with Oracle + Momus review
- specs/mcp-session-lifecycle: 7 requirements, 14 scenarios
- specs/session-orchestration: Modified requirements for close path
- specs/unified-session-lifecycle: WebSocket disconnect hook
- tasks.md: 7 task groups, 46 tasks (P1a-P1f + E2E)
- tests/mcp_server/test_stale_mcp_connection.py: 5 reproduction tests

Reviewed by Momus (PASS) and Oracle (PASS) after 2 revision cycles.

Closes #121 (spec phase)

* spec: address Gemini Code Assist review comments

4 accepted fixes from dialectical analysis with Oracle:

1. Task 3.1/3.2/3.3: Change _session_connections to
   dict[str, set[tuple[str, int]]] — store (connection_id,
   session_key) pairs so AcpMcpConnectionManager.cleanup_session()
   can look up SessionStreamPair via session_key

2. Task 5.2 (D6): Two-layer cleanup on resume — call both
   SessionController.close_session() (RunHandle lifecycle) AND
   ACPSession.close() (ACP env/signals/prompts). Neither alone
   is sufficient.

3. Task 6.4 (D7): Same two-layer cleanup for WebSocket disconnect

4. Task 2.8: Use try/finally or fixture teardown for test cleanup

Rejected comments (2):
- hasattr(self.agent, 'mcp'): violates AGENTS.md, mcp always set
- hasattr(agent, 'mcp'): same, agent is not None check exists

Already addressed (2):
- Concurrency re-verify after lock: spec's lock-on-context design
  handles this implicitly
- await on_disconnect: type signature makes it obvious

* feat(mcp): add _SessionContext dataclass and session connection tracking

- Add _SessionContext dataclass to MCPManager with per-session state
  (connection_pool, toolset_cache, snapshot, acp_connection_ids, _cleanup_lock)
- Add _session_contexts dict to MCPManager.__init__
- Add _session_connections reverse index to AcpMcpConnectionManager
- Add register_session_connection() method for tracking session→connection mappings

Implements T1 and T6 of fix-mcp-session-lifecycle plan.

* feat(mcp): add session lifecycle methods and ACP cleanup

- get_or_create_session() and update_session_snapshot() on MCPManager (T2)
- add_acp_transport() on MCPManager for session-scoped ACP tracking (T3)
- register_session() returns tuple[SessionStreamPair, int] (T7, GAP-1)
- has_active_sessions() on AcpMcpConnection (T7)
- cleanup_session() with _cleanup_lock on AcpMcpConnectionManager (T7, GAP-12)
- Updated all callers of register_session() to unpack tuple return

* feat(mcp): cleanup_session on MCPManager and wire register_session_connection

- cleanup_session() with per-session _cleanup_lock on MCPManager (T4)
- _acp_mcp_manager field added for ACP cleanup delegation
- connect_acp_mcp_server() gains session_id parameter (T8, GAP-5)
- Returns tuple[str, int] (connection_id, session_key)
- Call site in session.py passes session_id and calls add_acp_transport
- All test callers updated for new signature

* test(mcp): add session lifecycle and ACP cleanup unit tests (T5+T9)

* refactor(mcp): change as_capability to session_id-based API (T10)

- Change as_capability(snapshot=, session_pool=) to as_capability(session_id=)
- Parameterize _make_capability with toolset_cache dict parameter (GAP-7)
- Split _process_snapshot into _process_global_configs and _process_session_configs
- GAP-11: KeyError fallback for concurrent cleanup_session race
- Backward compat: session_id=None processes self.servers with self._toolset_cache

* refactor(agent): update get_agentlet to use as_capability(session_id) (T12)

- Replace as_capability(snapshot=, session_pool=) with as_capability(session_id=)
- GAP-4: Use run_ctx.session_id from AgentRunContext instead of self._session_id
- Remove if/else branching on _mcp_snapshot — as_capability handles internally
- Keep _mcp_snapshot and _session_connection_pool field declarations for compat

* test(mcp): update caching+provider tests for session_id API (T13)

- Update 6 tests in test_mcpmanager_caching.py for new as_capability(session_id) API
- Update 15 failing tests in test_mcp_provider_lifecycle.py to use session context
- Fix static source assertion in test_no_dedup_hack_in_get_agentlet
- All 48 tests pass

* test(mcp): flip stale connection tests to verify fix (T14)

- Rename test_session_resume_returns_stale_toolset → _returns_fresh_toolset
- Rename test_multiple_acp_servers_all_go_stale → _get_fresh_toolsets
- Rename test_disconnect_all_clears_cache → test_cleanup_session_clears_per_session_cache
- All 5 tests now verify the fix instead of documenting the bug
- All tests pass with new session_id API

* feat(session): wire cleanup_session into ACPSession.close and SessionController (T15)

* feat(agent): wire get_or_create_session in SessionController agent creation (T16)

* test(mcp): integration tests for session close lifecycle (T17+T18+T19)

* fix(acp): resume_session close-then-recreate instead of early-return (T20)

* test(acp): resume_session lifecycle tests - close, reconnect, active run (T21+T22+T23)

* feat(acp): add on_disconnect callback to websocket handler (T24)

- Add on_disconnect: Callable[[AgentSideConnection], Awaitable[None]] | None parameter
- Generate UUID4 connection_id on AgentSideConnection at accept time (GAP-3)
- Call on_disconnect in ConnectionClosed handler before conn.close()
- Backward compatible: on_disconnect defaults to None

* feat(acp): implement close_all_sessions_for_connection (T25)

- Add _connection_sessions reverse index on ACPSessionManager
- Add connection_id parameter to create_session() and resume_session()
- Implement close_all_sessions_for_connection() for WebSocket disconnect cleanup
- Idempotent: pops connection_id, iterates sessions, closes via SessionController + ACPSession.close()

* feat(acp): wire on_disconnect to close_all_sessions_for_connection (T26)

- Add on_disconnect parameter to serve(), _serve_websocket(), _serve_streamable_http()
- Wire on_disconnect callback in ACPServer._start_async() closure
- Add session_manager field to AgentPoolACPAgent for shared session tracking
- Create shared ACPSessionManager in ACPServer for cross-connection session tracking
- Add disconnect detection in _serve_streamable_http via recv_task completion
- Fix test_resume_session_is_idempotent -> test_resume_session_closes_old_and_recreates
  (T20 changed resume_session from idempotent to close-then-recreate)

* test(acp): websocket disconnect closes sessions and preserves others (T27+T28)

- T27: test_websocket_disconnect_closes_all_sessions — 2 sessions same conn, disconnect, both closed
- T27: test_websocket_disconnect_preserves_other_connections — 2 conns, disconnect one, other survives
- T28: test_websocket_disconnect_during_run — active run cancelled with 2s timeout on disconnect

* fix(acp): resolve mypy union-attr errors with cast (T32) + add e2e session lifecycle test (T33)

- T32: Use cast() to type session_manager field as ACPSessionManager (not | None) for mypy
- T33: test_e2e_session_lifecycle — full lifecycle: connect→session→MCP→disconnect→reconnect→resume→verify fresh

* fix: resolve CI ruff format and lint errors

- ruff format: reformat 5 files (manager.py, session_controller.py, session.py, test_session_lifecycle.py, test_stale_mcp_connection.py)
- ruff check: shorten docstring in test_acp_session_resume.py (E501)

* fix(mcp): address review — _connection_sessions cleanup, get_or_create leaks

- Fix #1 (Critical): resume_session() now removes session_id from
  _connection_sessions before closing old session. Prevents stale
  connection disconnect from closing the newly resumed session.
- Fix #2 (High): as_capability() uses _session_contexts.get() instead of
  get_or_create_session(). Prevents memory leak when context was already
  cleaned up. Removes dead try/except KeyError code.
- Fix #3 (Medium): cleanup_session() uses _session_contexts.get() and
  returns early if None. Avoids creating throwaway SessionConnectionPool.
- TDD: 3 tests in test_review_fixes.py verify all fixes.

* fix(mcp): wire _acp_mcp_manager, add identity check, consolidate as_capability

Review round 2 fixes:

Fix #4 (Critical): Wire _acp_mcp_manager in ACPSession.__post_init__
- MCPManager._acp_mcp_manager was initialized to None and never set
- cleanup_session() could never delegate to AcpMcpConnectionManager
- Per-session ACP stream pairs and reverse-index entries leaked
- Fix: wire agent.mcp._acp_mcp_manager = acp_agent._mcp_manager in __post_init__

Fix #5 (Medium): Identity check after acquiring cleanup lock
- Concurrent cleanup_session() callers could do redundant work
- All ops were idempotent but wasteful (clearing empty dicts, etc.)
- Fix: check if self._session_contexts.get(session_id) is not ctx after lock

Fix #6 (Medium): Consolidate duplicated fallback in as_capability()
- Three identical 'for server in self.servers:' loops consolidated to one
- Pure readability refactor, zero behavior change

TDD: 3 new tests (2 RED before fix, 3 GREEN after)
- test_cleanup_session_delegates_to_acp_mcp_manager (unit)
- test_acp_session_wires_acp_mcp_manager (integration)
- test_cleanup_session_identity_check_prevents_redundant_work (unit)

213 tests pass, ruff clean.

* test(mcp): add 20 integration tests for session wiring lifecycle

Categories A-D from Oracle integration test plan:
- A (4): Cross-component wiring — cleanup delegation, __post_init__ wiring,
  full close chain, close_all_sessions_for_connection
- B (7): Lifecycle edge cases — full create/cleanup, close/recreate,
  shared connection isolation, WebSocket disconnect, resume, concurrent cleanup
- C (4): State consistency — registry consistency after cleanup/close/resume,
  stream pair unregistration
- D (5): Error paths — ACP manager raises, session close raises,
  MCP cleanup raises, resume old close raises, pool cleanup raises

These tests would have caught the _acp_mcp_manager wiring bug (round 2
review comment #1) that unit tests missed due to component isolation.

* fix(acp): wire connection_id through create_session/resume_session call sites

- Declare connection_id: str | None on AgentSideConnection (replaces monkey-patch)
- Remove # type: ignore[attr-defined] from transports.py connection_id assignments
- Add _get_connection_id() helper on AgentPoolACPAgent using isinstance check
- Wire connection_id= into all 5 create_session/resume_session call sites:
  new_session, load_session, fork_session, resume_session, handler.py
- Fix misleading GAP-11 comment: dict.get() returns None, never raises KeyError

Without this fix, _connection_sessions dict was never populated, making
close_all_sessions_for_connection() always return immediately — the entire
WebSocket disconnect cleanup feature was dead code.

* test(mcp): add 13 E2E integration tests for full MCP session lifecycle

Covers all 13 gap areas identified by Oracle analysis:
- G1: Full create_session → get_or_create_session_agent → MCPManager chain
- G2: as_capability with non-empty ACP snapshot → real MCPToolset
- G3: initialize_mcp_servers → connect_acp_mcp_server → AcpMcpTransport
- G4: Full tool execution through as_capability → MCPToolset → AcpMcpTransport
- G5: SessionController.close_session with real agent + real MCP resources
- G6: resume_session with real ACPSession (not patched)
- G7: Full on_disconnect → close_all_sessions_for_connection chain
- G8: connection_id propagation: create_session populates _connection_sessions
- G9: as_capability during concurrent cleanup (GAP-11 race)
- G10: ACP transport failure during tool execution + cleanup
- G11: Multiple sessions on same connection with real ACPSessions
- G12: Child session inherits parent's ACP transports
- G13: Pool shutdown cleans all session MCP resources

* fix: resolve CI mypy and unit test failures

- server.py: Remove unused type: ignore, use None guard for connection_id
- test_acp_session_resume.py: Add connection_id to expected resume_session call args

* chore(openspec): archive fix-mcp-session-lifecycle and sync specs

- Mark all 46 tasks as complete in tasks.md
- Sync 3 delta specs to main specs:
  - mcp-session-lifecycle (new)
  - session-orchestration (updated)
  - unified-session-lifecycle (updated)
- Archive to openspec/changes/archive/2026-07-07-fix-mcp-session-lifecycle/

* fix: parent session memory leak + on_disconnect in finally (review r3)

- session_controller.py: Replace get_or_create_session() with
  _session_contexts.get() when reading parent snapshot/pool. Prevents
  phantom _SessionContext creation when parent was already cleaned up.
- transports.py: Move on_disconnect callback from except ConnectionClosed
  to finally block. Ensures callback fires on any exception path.
- 3 TDD tests: leak detection, regression guard, disconnect coverage.

* fix(mcp): wire child session ACP manager, add transport callback, fix toolset __aexit__

Three fixes for child session ACP transport registration gaps:

1. Wire _acp_mcp_manager on child agent from parent (session_controller.py)
   - Child sessions created via get_or_create_session_agent() don't go
     through ACPSession.__post_init__, so _acp_mcp_manager stayed None.
     Now copied from parent after copy_pre_created_transports().

2. Add on_session_registered callback to AcpMcpTransport (acp_mcp_transport.py)
   - Optional callback invoked after register_session() with (connection_id,
     session_key). Enables callers to register ACP connections for cleanup
     tracking via register_session_connection().

3. Fix toolset_cache.clear() to call __aexit__ first (manager.py)
   - cleanup_session() called .clear() without closing MCPToolset instances,
     leaking stream pairs and forwarder tasks. Now mirrors disconnect_all()
     pattern: iterate values, call __aexit__(None, None, None) with
     contextlib.suppress(ValueError), then clear.

TDD: 3 tests in test_child_session_acp_fix.py (all GREEN).
252 MCP+ACP tests pass, 0 regressions, ruff clean.
Million-mo added a commit that referenced this pull request Aug 3, 2026
#1 tool_call_id: QuestionCapability._question already uses replace() to
propagate tool_name/tool_call_id/tool_input — verified with L2 test.
#2 telemetry: add @logfire.instrument to QuestionCapability._question;
background_task modules already instrumented.
#3 state cleanup: after_run() evicts _session_states and _ephemeral_states,
shuts down batcher and task manager.
#5 queued cancel: pending cancel path fires on_completed before
completion_event.set().
#6 flush exception: _flush catches broad Exception, marks delivered
regardless of success/failure.
#7 timeout message: CancelledError handler checks task.status ==
'timed_out' before choosing message.
#8 private API: guard pydantic_ai._agent_graph import with try/except
and helpful error message.
#9 L2 test: add test_question_tool_propagates_tool_call_id_to_agent_context.
#10 error contract: _build_definition raises ValueError with descriptive
message for missing name or non-dict input.
Nit: remove DEBUG_TASK_MGR prefix, type coro as Coroutine, fix test names.
Million-mo added a commit that referenced this pull request Aug 4, 2026
…Capability from xeno-agent (#346)

* feat(capabilities): add QuestionCapability with YAML schema override support

Move question tools into a proper AbstractCapability that accepts
args.schemas and args.enabled_tools, mirroring BackgroundTaskCapability.
Replaces the bare QuestionTools entry point so consumers can customize
LLM-facing parameter descriptions via YAML schema files without writing
their own capability wrapper.

* test(question): migrate question tool tests from xeno-agent

Move 51 question tool unit tests (40 for question_for_user + 11 for
ask_followup_question) from xeno-agent to agentpool. Tests now import
from agentpool_toolsets.builtin.question_tools instead of xeno_agent.

Two assertions adjusted to match agentpool's actual implementation:
- Error message regex: 'questionnaire' → 'questions' (agentpool naming)
- ask_followup_question metadata: dropped suggestion_attributes check
  (agentpool's _format_followup_response doesn't emit this field)

Also add RUF001 per-file ignore for CJK fullwidth punctuation in test data.

* feat(question): merge simple question tool into QuestionCapability

Add a 'question' tool to QuestionCapability that replicates the legacy
QuestionTool behavior (simple prompt + optional response_schema via MCP
Elicit). This unifies all user-interaction tools under one capability:
  - question_for_user: XML multi-question questionnaire
  - ask_followup_question: single question with <suggest> options
  - question: simplest single-question (replaces QuestionTool)

Mark QuestionToolConfig (tools: [{type: question}]) as deprecated,
directing users to capabilities: [{type: question}].

Add 3 new tests covering the question tool: default-enabled, enabled
alone, enabled via schemas.

* feat(capabilities): add BackgroundTaskCapability migrated from xeno-agent

Migrate the complete BackgroundTaskCapability implementation to agentpool:
- capability.py: full lifecycle management (task, background_output,
  background_cancel, steer_task tools)
- manager.py: BackgroundTaskManager with concurrent task execution,
  cleanup, and session isolation
- notification.py: NotificationBatcher for debounced completion notifications
- types.py: BackgroundTask, SessionTaskState dataclasses
- utils/tool_schema.py: YAML schema loading and LLM-facing schema override

Includes 277 tests (unit + integration + resource provider) covering
lifecycle, concurrency, error propagation, notification batching,
history isolation, and cancellation regression.

Config schema files (task.yaml, background_output.yaml,
background_cancel.yaml, steer_task.yaml) provide default LLM-facing
parameter descriptions.

Entry point 'background_task' registered in pyproject.toml.

* fix(ci): resolve ruff format, mypy, and flaky test failures

Three CI failures fixed:

1. ruff format: question.py ternary expression reformatted

2. mypy (16 errors → 0):
   - tool_schema.py: cast yaml.safe_load/json.loads results to
     OpenAIFunctionDefinition, construct TypedDict with explicit
     key-value pairs instead of ** unpacking
   - question.py: add ToolResult return annotations to tool wrappers
   - manager.py: re-read task_model.status into locals after await
     to prevent mypy narrowing from concurrent status changes
   - capability.py: type session_pool as SessionPool | None,
     fix delivered bool assignment from followup() str|None return,
     remove dead config.type == 'team' comparison (agents dict never
     contains team configs), rename shadowed task_model variable

3. Flaky tests (8 failures): replace fixed asyncio.sleep(0.8) with
   _wait_until_called() polling helper in 3 test files. The fixed
   sleep was too tight under CI load — debounce timers fire late
   when the event loop is busy with parallel workers.

* fix(ci): replace anyio TaskGroup with asyncio.ensure_future in NotificationBatcher

Root cause: loop.call_later callbacks run in a separate contextvars
context where anyio's sniffio async-library detection fails with
AsyncLibraryNotFoundError. This prevented _schedule_flush from
calling tg.start_soon, so _flush never executed and deliver_callback
was never invoked — all 25 batcher tests + 8 notification tests
failed in CI.

Fix: replace anyio.create_task_group/start_soon with
asyncio.ensure_future for scheduling flush coroutines. Keep
anyio.CancelScope and anyio.fail_after for timeout protection
(these don't require sniffio context). Track flush tasks in a
set[asyncio.Task] and cancel/await them in shutdown.

Also restore source_type team detection by checking config.type
via str() cast (mypy-safe for AnyAgentConfig union that doesn't
include team types at the type level, but mocks provide type='team'
at runtime).

* refactor(question): unify question tools into single question tool

Merge ask_followup_question, question_for_user, and question into one
unified question tool per reviewer feedback. The question_for_user
implementation (richest, supports multi-question XML with enum/multi/
input types) is retained as the canonical implementation, renamed to
question. ask_followup_question (legacy compat) and the simple question
tool are removed.

Changes:
- question_tools.py: remove ask_followup_question + _format_followup_response,
  rename question_for_user to question
- question.py: simplify QuestionCapability to expose only question
- Update all tests, docs, and tool name references

* fix(review): address opencode-agent review findings

#1 tool_call_id: QuestionCapability._question already uses replace() to
propagate tool_name/tool_call_id/tool_input — verified with L2 test.
#2 telemetry: add @logfire.instrument to QuestionCapability._question;
background_task modules already instrumented.
#3 state cleanup: after_run() evicts _session_states and _ephemeral_states,
shuts down batcher and task manager.
#5 queued cancel: pending cancel path fires on_completed before
completion_event.set().
#6 flush exception: _flush catches broad Exception, marks delivered
regardless of success/failure.
#7 timeout message: CancelledError handler checks task.status ==
'timed_out' before choosing message.
#8 private API: guard pydantic_ai._agent_graph import with try/except
and helpful error message.
#9 L2 test: add test_question_tool_propagates_tool_call_id_to_agent_context.
#10 error contract: _build_definition raises ValueError with descriptive
message for missing name or non-dict input.
Nit: remove DEBUG_TASK_MGR prefix, type coro as Coroutine, fix test names.

* chore: remove list_available_nodes tool (legacy, Leoyzen feedback #345)

- src/agentpool_toolsets/builtin/subagent_tools.py: remove
  list_available_nodes method + create_tool registration
- src/agentpool_config/toolsets.py: SubagentToolName Literal now
  only accepts 'task'; docstring updated
- tests/toolsets/test_tool_filtering.py: update assertions
- tests/toolsets/builtin/test_as_capability.py: update assertions
- tests/servers/acp_server/test_claude_acp_toolset_integration.py:
  update assertion
- tests/tools/test_runcontext.py: remove prompt referencing
  list_available_nodes (test was already xfail)
- docs/how-to/advanced/acp-integration.md: update docs
- docs/how-to/servers/mcp-server.md: update docs

The tool was legacy code; Leoyzen noted agents list is now
injected directly into system prompt.

* fix: ruff format toolsets.py (single-entry Literal syntax)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant