Feature/bump deps main 0402 - #6
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates to the agentpool codebase, primarily focusing on the integration of the opencode_sdk and the implementation of a new elicitation system. Key changes include the migration to opencode_sdk models, the addition of a MessageReconstructor to handle event stream reconstruction, and the introduction of a SandboxBashTool using bashkit. Additionally, the PR refines the ACP server's session management and configuration handling. My review identified several critical issues regarding type safety in mode validation, incorrect mapping of permission results, and unnecessary type coercion that could break boolean configuration options. Please address these issues to ensure robust and type-safe behavior.
| valid_ids = {str(m.value) for m in matching_category.available_modes} | ||
| if mode_id not in valid_ids: | ||
| raise UnknownModeError(mode_id, sorted(valid_ids)) |
There was a problem hiding this comment.
The validation logic for mode_id is not type-safe for boolean modes. Since mode_id can be a bool and valid_ids is a set of strings (due to str(m.value)), the membership check mode_id not in valid_ids will always be true for boolean values (e.g., True != "True").
if matching_category := next((c for c in available_modes if c.id == category_id), None):
valid_values = {m.value for m in matching_category.available_modes}
if mode_id not in valid_values:
raise UnknownModeError(mode_id, sorted(str(v) for v in valid_values))| mapping: dict[ConfirmationResult, Literal["allow"]] = { | ||
| "allow": "allow", | ||
| "skip": "allow", | ||
| "abort_run": "allow", | ||
| "abort_chain": "allow", | ||
| } |
There was a problem hiding this comment.
Mapping skip, abort_run, and abort_chain to "allow" is incorrect and potentially dangerous. If a user chooses to skip a tool or abort the run, the agent should not be given permission to execute the tool. These results should map to "deny".
mapping: dict[ConfirmationResult, Literal["allow", "deny"]] = {
"allow": "allow",
"skip": "deny",
"abort_run": "deny",
"abort_chain": "deny",
}| config_options = await get_session_config_options(self.agent) | ||
| # Update the changed option's current_value | ||
| if opt := next((i for i in config_options if i.id == config_id), None): | ||
| assert isinstance(value_id, str) |
| if not isinstance(config_opt, SelectSessionConfigOption): | ||
| continue |
| name=config_opt.name, | ||
| available_modes=mode_infos, | ||
| current_mode_id=config_opt.current_value, | ||
| current_mode_id=str(config_opt.current_value), |
There was a problem hiding this comment.
Unnecessary string coercion of current_value. Since ModeCategory.current_mode_id supports str | bool, the value should be passed as-is to preserve type information, especially for boolean config options.
| current_mode_id=str(config_opt.current_value), | |
| current_mode_id=config_opt.current_value, |
| await self._agent.update_state( | ||
| config_id=str(opt.id), | ||
| value_id=str(opt.current_value), | ||
| ) |
There was a problem hiding this comment.
Coercing value_id to a string here is unnecessary and loses type information for boolean configuration options. Agent.update_state and the underlying ConfigOptionChanged signal both support str | bool values.
| await self._agent.update_state( | |
| config_id=str(opt.id), | |
| value_id=str(opt.current_value), | |
| ) | |
| await self._agent.update_state( | |
| config_id=opt.id, | |
| value_id=opt.current_value, | |
| ) |
- Store task references in _consumer_tasks dict to prevent GC - Store asyncio.Task objects from ensure_future() - This ensures background tasks don't get collected prematurely - Addresses issue #6 from code review
…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.
* 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.
… 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.
…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
#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.
…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)
20260402合并phi65主分支