Skip to content

feat(capabilities): add QuestionCapability and migrate BackgroundTaskCapability from xeno-agent - #346

Merged
Million-mo merged 11 commits into
mainfrom
feat/question-capability-v2
Aug 4, 2026
Merged

feat(capabilities): add QuestionCapability and migrate BackgroundTaskCapability from xeno-agent#346
Million-mo merged 11 commits into
mainfrom
feat/question-capability-v2

Conversation

@Million-mo

Copy link
Copy Markdown
Collaborator

Summary

Migrate question and background_task capabilities from xeno-agent into agentpool as built-in AbstractCapability subclasses with YAML schema override support.

QuestionCapability (src/agentpool/capabilities/question.py)

  • Provides a unified question tool with multi-question XML questionnaire support (enum/multi/input types)
  • Accepts schemas: dict[str, str] for YAML schema file paths (resolved relative to CONFIG_DIR)
  • Accepts enabled_tools: list[str] to control which tools are exposed
  • Tool implementation sourced from agentpool_toolsets.builtin.question_tools.QuestionTools
  • ask_followup_question merged into question per reviewer feedback — single tool, simpler API

BackgroundTaskCapability (src/agentpool/capabilities/background_task/capability.py)

  • Migrated from xeno-agent with full lifecycle support
  • Tools: task, background_output, background_cancel, steer_task
  • load_skills, expected_output, title parameters on task
  • NotificationBatcher for debounced batch completion notifications
  • force_retrieval mode (disabled/tool_choice/directive) to ensure pending task results are retrieved
  • SessionTaskState with concurrency control, timeout, and cleanup
  • after_node_run hook for completion notification injection
  • Schema override support mirroring QuestionCapability

Bug Fixes

  • Replace anyio.TaskGroup with asyncio.ensure_future in NotificationBatcher to fix Python 3.13 compatibility
  • Resolve ruff format, mypy, and flaky test failures

Changes

  • 6 commits on top of main
  • All 12 CI checks passing

…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.
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.
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.
…gent

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.
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.
…cationBatcher

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).
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
@opencode-agent

opencode-agent Bot commented Aug 3, 2026

Copy link
Copy Markdown

No GitHub token is available for direct gh posting, so my synthesized review will be captured and posted as the PR comment by the opencode infrastructure. Here is the review:


Review: feat/capabilities: add QuestionCapability and migrate BackgroundTaskCapability from xeno-agent

Reviewed the full 41-file, ~15k-line diff against main, with specialist deep-dives on the background_task core, the question capability/config wiring, and documentation. CI checks reportedly pass; the findings below are correctness/consistency issues that passing CI does not catch.

Blocking

1. QuestionCapability._question drops tool_call_id/tool_name — breaks durable-elicitation correlation (src/agentpool/capabilities/question.py:147-156)

The old question_for_user flowed through wrap_tool_for_pydantic_ai, which calls the tool with replace(ctx.deps, tool_name=ctx.tool_name, tool_call_id=ctx.tool_call_id, tool_input=kwargs.copy()) (src/agentpool/tools/tool_wrapping.py:74-79). The new _question passes ctx.deps raw into QuestionTools.question, so AgentContext.tool_call_id stays None. handle_elicitation() then computes handle = self.tool_call_id or run_ctx.run_id (src/agentpool/agents/context.py:380) and keys the pending-deferred/crash-recovery cache under run_id instead of the real tool call id. On resume this mismatches ToolCallPart.tool_call_id, breaking (a) the crash-recovery cache at context.py:308-317 and (b) checkpoint/elicitation-bridge correlation — the exact failure tool_wrapping.py:103-108 warns about. The non-durable path still works, which is why no test catches it.

Fix: mirror the wrapper — agent_ctx = replace(ctx.deps, tool_name=ctx.tool_name, tool_call_id=ctx.tool_call_id, tool_input={"questions": questions}). And add a test that asserts the propagated tool_call_id (see #9).

2. Zero telemetry on a mandatory-instrumented critical path

None of the new files (background_task/*, question.py, utils/tool_schema.py) use @logfire.instrument or logfire.span. Root and capabilities AGENTS.md both make span instrumentation of capability critical paths mandatory, and "never asyncio.create_task() without an active span." manager.py:107,418 (create_task), notification.py:160,174 (ensure_future), and manager.py:377-378 are all bare. create_task inherits the caller's contextvars so runs started inside a tool-call span aren't orphaned — but the entire minutes-long background path (_execute_task, _run_and_stream, batched notification delivery) runs under a stale, already-ended parent span, and tasks spawned after the run's span closes (e.g. manager.py:418 cleanup timers, notification.py:160 auto-start) are orphan traces. Compare the established pattern in orchestrator/run.py:892 which wraps its fire-and-forget emission in logfire.span("event.user_message_inserted.emit"). Every other capability in the tree (e.g. resource_capability.py, subagent_capability.py) is instrumented.

3. Unbounded per-run state accumulation (src/agentpool/capabilities/background_task/capability.py:248-257, 286-335)

_session_states is keyed by run_ctx.run_id (regenerated each run, agents/context.py:96) with no after_run/on_run_ended eviction — for_run returns self, so the dict grows one full BackgroundTaskManager + live-TimerHandle NotificationBatcher per run, forever. Worse, _ephemeral_states is keyed by id(ctx): values are never evicted, and CPython reuses id() after GC, so an unrelated later context can inherit a stale SessionTaskState (cross-context correctness hazard, not just a leak). Need a run-end cleanup hook that removes both the run_id entry and the promoted ephemeral key.

Significant

4. DEBUG_TASK_MGR debug scaffolding at logger.error in production paths (manager.py:166-213) — logger.error("... on_completed returned for task_id=%s"), logger.error("... _execute_task finished ..."), "DEBUG_TASK_MGR: Task exception:". Success-path notifications at ERROR level will flood error channels/alerting. Only the on_completed FAILED branch belongs at error; the rest should be debug/info and the DEBUG_TASK_MGR: prefix removed.

5. Cancelling a queued task never runs on_completed (manager.py:232-240) — the pending branch marks cancelled and sets completion_event but doesn't call handle.on_completed(), and doesn't cancel the parked coroutine. on_completed (which pops+sets run_ctx.child_done_events, capability.py:1111-1114) only fires later when the semaphore is eventually acquired — possibly never under saturation or if shutdown() intervenes. A parent awaiting child_done_events.wait() after background_cancel of a queued task can hang indefinitely. The other two cancel paths (manager.py:122-131, :184-208) do invoke it — all three should behave identically.

6. NotificationBatcher._flush only guards TimeoutError/CancelledError (notification.py:182-222) — any other exception from deliver_callback (e.g. a RuntimeError from RunHandle.followup, capability.py:410) escapes the ensure_future'd task ("Task exception was never retrieved") and _delivered is never marked, so the batch is re-delivered on the next flush → duplicate lead-agent notifications. Wrap the deliver call in a broader handler and mark delivered regardless.

7. Timed-out tasks write # Task Cancelled to the output file (manager.py:156-159 + capability.py:1079-1082) — wait_for cancels the inner coroutine, _execute_task records timed_out, but the coroutine's CancelledError handler writes "Task ... was cancelled". background_output then reports a timed_out status whose file content claims cancellation.

8. Runtime import of private pydantic-ai API (capability.py:15, from pydantic_ai._agent_graph import ModelRequestNode) — the only runtime import of a private pydantic_ai module in the codebase; ModelRequestNode isn't re-exported on the pinned >=2.12.0 line, so it breaks silently on a minor upgrade. At minimum pin the exact version and add a compile guard; _instructions is already correctly confined to TYPE_CHECKING.

9. Missing L2 coverage for QuestionCapabilitytests/AGENTS.md requires L1 + L2 for new capabilities. test_question_capability.py is L1 shape-only; there is no test that _question adapts RunContext → AgentContext, no assertion of tool_call_id/tool_name propagation (which would have caught #1), and no real-pool run wiring the capability in. utils/tool_schema.py also has zero direct tests for its error paths.

10. tool_schema.py error contract mismatch (utils/tool_schema.py:67-82) — _build_definition raises raw KeyError (missing name) / TypeError (non-dict input), not the documented ValueError (:96-98, module docstring :29-31). And the YAML path (:54-55,62) bypasses _build_definition entirely, so a YAML file missing name is silently accepted while identical JSON crashes — inconsistent validation.

Minor / nits

  • question.py:153-156 instantiates QuestionTools(name="question_tools") on every tool call (allocates a full pydantic-ai Tool each round-trip); cache on the capability.
  • capability.py:687,705 — inert pyright suppressions (reportAny/reportAssignmentType are disabled in pyproject.toml:463-468, and mypy --strict ignores pyright comments). The config.type == "team" string check is also unreachable with real configs (manifest.agents is never "team"). Prefer an isinstance/discriminator check.
  • apply_params_schema (tool_schema.py:85-107) mutates the caller's Tool in place; safe today because get_toolset() builds fresh Tools, but a footgun for cached tools. dict[str, Any](params) raises TypeError on non-dict parameters.
  • config/tools/task.yaml:80 strict: true is dead config (apply_params_schema copies only parameters, dropping siblings), and load_skills/expected_output are required in the schema but optional in _task's signature (capability.py:641-650). Schema-vs-signature mismatch is confusing even though the model-facing effect is consistent.
  • Duplicated _format_duration (capability.py:124-145 loose-typed vs notification.py:42-61 typed); vestigial _cancel_scope in the batcher (entered, never exited); coro: Any in manager.py:86,114 could be Coroutine[Any, Any, None].
  • test_question_for_user.py — ~20 test names/docstrings still reference the removed question_for_user; ~21 cast(...) usages that tests/AGENTS.md discourages.
  • manager.py:161-183 — the specific-exception tuple and the bare Exception handler are near-duplicates; the ordering is correct (CancelledError is BaseException), just collapse.

Docs / process

  • BackgroundTaskCapability has zero user documentation. Its four tools (task, background_output, background_cancel, steer_task) and config surface (capabilities: [{type: background_task}], schemas:, enabled_tools:, force_retrieval:, max_concurrent_tasks:) appear in no doc file; the new config/tools/*.yaml schemas are unreferenced.
  • src/agentpool/capabilities/AGENTS.md "Where to Look" table and docs/explanation/capabilities.md registry were not updated for either capability (nor docs/explanation/module-structure.md). Given background_task/ (~2,400 LOC, 4 modules) exceeds subsystems that each have their own AGENTS.md, a background_task/AGENTS.md + root context-loading row would be appropriate.
  • user-interaction.md is internally consistent (verified against parse_questionnaire and the tool signature) — nice. But it never shows the capabilities: [{type: question}] syntax it now deprecates tools: in favor of, and the deprecated path still emits a question-named legacy tool that would collide with the capability's question if both are enabled.
  • No OpenSpec change exists for either capability, despite AGENTS.md's "all significant changes go through OpenSpec." docs/rfcs/RFC-0034-background-task-redesign.md is still listed as Draft in docs/rfcs/STATUS.md and RFC-0055 describes this migration as "planned future migration … does not yet exist" — now stale. At minimum the PR should carry an OpenSpec proposal and update RFC-0034's status.

Positives

  • Clean entry-point migration (pyproject.toml:107); no broken references to question_for_user/ask_followup_question in live code/docs, and QuestionToolConfig.get_tool() deprecation warning is well-handled (no filterwarnings=error to trip it).
  • The concurrency core is genuinely sound: blocking-waiter design (on_completed fires before completion_event.set(), tokenized unregister), the after_node_run loop breaker, weakref.ref(run_ctx) in the deliver callback, and every task path converging to a terminal state. The anyio.TaskGroup → ensure_future fix rationale is credible (loop.call_later callbacks run outside the sniffio context).
  • Unifying to a single question tool simplifies the API, and the large layered background-task test suite is a strong asset.

Bottom line: don't merge until #1#3 are addressed (the elicitation regression is a functional break for durable sessions; the state-growth is a leak/correctness hazard; the telemetry gap violates an explicit red line). #4#7 should be fixed in this PR; the rest are follow-ups.

New%20session%20-%202026-08-03T11%3A40%3A07.327Z
opencode session  |  github run

#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
Million-mo requested a review from Leoyzen August 3, 2026 12:54
@Million-mo

Copy link
Copy Markdown
Collaborator Author

/oc

- 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.
@Million-mo

Copy link
Copy Markdown
Collaborator Author

/oc

@Million-mo
Million-mo merged commit 62f8b37 into main Aug 4, 2026
12 checks passed
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