feat: add configurable skills loading paths with YAML configuration - #4
Closed
Leoyzen wants to merge 2 commits into
Closed
feat: add configurable skills loading paths with YAML configuration#4Leoyzen wants to merge 2 commits into
Leoyzen wants to merge 2 commits into
Conversation
Introduce skills configuration section to manifest allowing users to specify custom skill directories and control default path inclusion. Skills are discovered from configured paths with "first path wins" semantics for conflict resolution. Key changes: - Add SkillsConfig model with paths and include_default fields - Update AgentsManifest to support skills configuration - Enhance SkillsManager to use configured paths for discovery - Implement relative path resolution against config file location - Support remote filesystems (s3://, github://) for skill discovery - Add comprehensive tests for path resolution and backward compatibility The implementation ensures existing configurations without skills section continue to work exactly as before, with default paths (~/.claude/skills/, .claude/skills/) automatically included unless explicitly disabled.
Summary of ChangesHello @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 flexibility of skill discovery within AgentPool by introducing a configurable 'skills' section in the YAML manifest. Previously limited to hard-coded default locations, users can now specify custom local or remote paths for their skills, resolve relative paths based on the manifest's location, and control the inclusion of default skill directories. This change empowers users to manage project-specific skill collections, share libraries, and precisely define their operational environment, moving towards a more adaptable and user-centric skill management system. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for configuring skill loading paths via YAML, which greatly improves flexibility and project-specific skill management. The implementation is well-thought-out, including a new Pydantic model for configuration, updates to the SkillsManager to use this configuration, and comprehensive unit and integration tests that cover various scenarios like relative/absolute paths, priority resolution, and backward compatibility. The code is clear and the changes are well-documented in the PR description.
I have one suggestion for a minor refactoring to improve code clarity by removing a small piece of redundant code. Overall, this is an excellent contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
3 tasks
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
May 27, 2026
Change ToolCallLocation.line from int = Field(default=0, ge=0) to int | None = Field(default=None, ge=0) to align with the spec which treats line as optional (omitted when not applicable).
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.
Leoyzen
added a commit
that referenced
this pull request
Jul 28, 2026
… StreamCompleteEvent RunErrorEvent was always a terminal event. Adding a trailing StreamCompleteEvent created a double-terminal problem patched with _run_error_emitted guard flags — unnecessary complication. Revert to simpler design: - NativeTurn.execute() path #7b: yield RunErrorEvent only (terminal) - _execute_turn(): restore break on RunErrorEvent - Remove _run_error_emitted guard from ACPEventConverter and EventProcessor - Remove _MAX_EVENTS_AFTER_ERROR defensive guard - Remove guard test files, update terminal event and execute_turn tests Cancellation paths (#4/#6/#9) still yield StreamCompleteEvent(cancelled=True) — these don't have RunErrorEvent and need a terminal event.
Leoyzen
added a commit
that referenced
this pull request
Jul 28, 2026
…289) * fix: default request_limit to None (unlimited) for native agents PydanticAI's UsageLimits defaults request_limit to 50, which is too low for agents with many tool calls. When no usage_limits are explicitly configured, default to request_limit=None (unlimited). * feat: EnqueuedMessagesEvent mapping + terminal event standardization - Upgrade pydantic-ai from 2.9.0 to >=2.12.0 (resolved to 2.18.0) - Refactor EventMapper to hook-based dispatch with handle_* methods - Add handle_enqueued_messages() mapping EnqueuedMessagesEvent → UserMessageInsertedEvent - Add StepErrorMetadata dataclass + step_error field on RunErrorEvent - Standardize terminal events: all 9 NativeTurn.execute() exit paths now yield exactly one terminal event - Fix CancelledError path (#4) to yield StreamCompleteEvent(cancelled=True) - Fix RunErrorEvent path (#7b) to yield trailing StreamCompleteEvent(cancelled=True) - Fix RuntimeError/GeneratorExit path (#6) to yield StreamCompleteEvent(cancelled=True) - Fix belt-and-suspenders path (#9) to yield StreamCompleteEvent(cancelled=True) - Migrate RunHandle.followup() from session.prompt_queue to agent_run.enqueue(priority='when_idle') - Remove _schedule_user_message_emission() from steer/followup when EnqueuedMessagesEvent available - Fix _execute_turn() to not break on RunErrorEvent — continue consuming trailing StreamCompleteEvent - Add defensive guard: break after 3 events without StreamCompleteEvent following RunErrorEvent - Add _run_error_emitted guard to ACPEventConverter and OpenCodeEventProcessor to prevent double terminal signals - 53 new tests across 6 new test files - 3521 unit tests pass, ruff clean, mypy clean * fix: UserMessageInsertedEvent dedup via FIFO message_id queue + converter _displayed_message_ids - Revert steer()/followup() emission condition: always emit fire-and-forget when emit_user_message=True - Add _pending_enqueue_message_ids FIFO queue to AgentRunContext (shared between RunHandle and NativeTurn) - steer()/followup() append message_id before agent_run.enqueue() — handle_enqueued_messages() pops to reuse same message_id - Add _displayed_message_ids: set[str] to ACPEventConverter — skips duplicate UserMessageInsertedEvent by message_id - Add displayed_message_ids: set[str] to EventProcessorContext — same dedup for OpenCode - Dedup sets persist per-session (not cleared in reset()) - Fixes CI failure: test_steer_to_acp_converter_pipeline now passes (fire-and-forget restored) - ACP compatible: no agent_run.enqueue() for ACP → no FIFO queue populated → no EnqueuedMessagesEvent → only one event source - 30 tests pass (8 new dedup tests + 3 FIFO queue tests + existing tests updated) * refactor: Replace _steer_received heuristic with precise EnqueuedMessagesEvent split trigger - Remove _steer_received flag from EventProcessorContext - Remove PartStartEvent + _steer_received two-step heuristic from opencode_event_bridge.py - Replace with direct one-step trigger: UserMessageInsertedEvent(source='internal', delivery='steer') - source='internal' fires at drain time (EnqueuedMessagesEvent), not receive time - source='protocol' (receive time) does NOT trigger split — correct behavior - delivery='followup' does NOT trigger split — only steer splits the logical turn - Eliminates false split race: PartStartEvent could fire before steer was actually drained - 11 turn split tests pass (6 updated + 5 new) * refactor: RunErrorEvent is terminal — remove guard flags and trailing StreamCompleteEvent RunErrorEvent was always a terminal event. Adding a trailing StreamCompleteEvent created a double-terminal problem patched with _run_error_emitted guard flags — unnecessary complication. Revert to simpler design: - NativeTurn.execute() path #7b: yield RunErrorEvent only (terminal) - _execute_turn(): restore break on RunErrorEvent - Remove _run_error_emitted guard from ACPEventConverter and EventProcessor - Remove _MAX_EVENTS_AFTER_ERROR defensive guard - Remove guard test files, update terminal event and execute_turn tests Cancellation paths (#4/#6/#9) still yield StreamCompleteEvent(cancelled=True) — these don't have RunErrorEvent and need a terminal event. * fix: Add RunErrorEvent handling to OpenCode event bridge session status RunErrorEvent is now terminal (no trailing StreamCompleteEvent), but the OpenCode event bridge session status match block didn't handle it. This left the TUI session stuck in 'busy' state after an agent error. Add RunErrorEvent case that mirrors RunFailedEvent cleanup: - Set session status to 'idle' - Register assistant message if unregistered (C3 fallback) - Finalize assistant time - Set MessageAbortedError on assistant message - Persist assistant message and context for resume - Do NOT broadcast SessionErrorEvent (EventProcessor already does this) 8 new tests in test_run_error_session_status.py. * refactor: simplify source field to Literal["enqueued", "internal"] source="enqueued": EnqueuedMessagesEvent mapping (model processing time) → display + steer split source="internal": fire-and-forget emission (send time, fallback) → display only, no split Changes: - events.py: source Literal simplified to ["enqueued", "internal"] - event_mapper.py: handle_enqueued_messages() uses source="enqueued" - run.py: steer()/followup() skip _schedule_user_message_emission() when _enqueued_messages_available AND active_agent_run is not None - session_controller_runs.py: source="team"/"protocol" → "internal" - session_pool_messaging.py: source="background_task" → "internal" - opencode_event_bridge.py: split triggers on source="enqueued" (was "internal") - 7 test files updated for new source values 108 tests pass, ruff clean * fix: update tests for source=enqueued refactor - test_route_message_event: source assertions protocol→internal - test_p2_protocol_channel_routing: source=protocol→internal, update ProtocolChannel routing test to expect internal source routed through channel - test_steer_event_pipeline: set _enqueued_messages_available=False to force fire-and-forget path (mock agent_run doesn't trigger EnqueuedMessagesEvent) - test_user_message_inserted_integration: background_task→internal (12 occ), add delivery=steer filter to distinguish from initial prompt events (both now source=internal), fix protocol→internal in dedup test - test_steer_background_acp_integration: background_task→internal, add delivery=steer filter - test_run_error_session_status: ruff format 26 tests pass across all modified files
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds configurable skill loading paths to AgentPool, enabling users to define custom skill directories via YAML configuration and control whether default skill paths are included.
Motivation
Previously, skill discovery was limited to hard-coded default paths (
~/.claude/skills/and.claude/skills/). This made it difficult for users to:Changes
New Configuration
Added
skillssection to manifest configuration:Implementation Details
agentpool_config/skills.py): Pydantic model definingpathsandinclude_defaultfields withget_effective_paths()method for path resolutionskills: SkillsConfigfield toAgentsManifestBackward Compatibility
skillssection continue to work unchangedinclude_defaultis true (default)skills: {}includes only default pathsTesting
Added comprehensive test coverage:
Example Usage