Skip to content

feat: PydanticAI thinning refactor - #69

Closed
Million-mo wants to merge 1 commit into
wolf1069b:develop/agenticfrom
Million-mo:refactor/pydanticai-thinning
Closed

feat: PydanticAI thinning refactor#69
Million-mo wants to merge 1 commit into
wolf1069b:develop/agenticfrom
Million-mo:refactor/pydanticai-thinning

Conversation

@Million-mo

Copy link
Copy Markdown
Collaborator

Summary

Replace ~1,500 lines of custom code that duplicates PydanticAI functionality with thin adapters delegating to native PydanticAI capabilities. AgentPool becomes "thin" at the agent-engine layer (where PydanticAI is source of truth) and "thick" only at the orchestration layer (sessions, multi-agent composition, protocol servers, skills).

Changes

  • HooksCapabilityAdapter (): bridges AgentPool hooks to pydantic_ai.capabilities.Hooks with priority combining (deny > ask > allow), matcher filtering, and return type normalization. from_agent_hooks() factory for transparent migration from existing CallableHook/CommandHook/PromptHook.
  • ProcessHistory: delegates to pydantic_ai.capabilities.ProcessHistory directly, removing custom ProcessHistoryAdapter (~200 lines of caching + signature validation)
  • Tool thinning: simplified Tool.to_pydantic_ai() to 1:1 mapping, removed ToolKind taxonomy and redundant metadata fields
  • Event passthrough: PartStartEvent/PartDeltaEvent use session_id via context instead of subclassing
  • PromptInjectionManager: inject()/consume() preserved for tool result augmentation; queue()/pop_queued() removed for native agents (delegated to PendingMessageDrainCapability)

Regression Tests

116 new tests covering:

  • Hook combination priority semantics (deny > ask > allow)
  • ProcessHistoryAdapter wrapping behavior
  • PromptInjectionManager full lifecycle
  • Tool.to_pydantic_ai() conversion (schema, deferred, approval)
  • Event subclass behavior (session_id, factory methods)

All 116 tests pass on develop/agentic.

Documentation

  • RFC-0001: Architecture decision record
  • Migration guide: docs/migration/pydanticai-thinning.md
  • CHANGELOG.md with deprecations and migration notes
  • OpenSpec change: pydanticai-thinning-refactor (all tasks complete)

Deprecations (no breaking changes)

  • AgentHooks.as_capability() → use HooksCapabilityAdapter.from_agent_hooks()
  • Hook ABC hierarchy → thin adapters, remains for YAML config
  • ProcessHistoryAdapter → use pydantic_ai.capabilities.ProcessHistory directly
  • ToolKind → string-based tool name patterns
  • ToolResult.structured_content → PydanticAI native ToolReturn
  • PartStartEvent/PartDeltaEvent subclassing → use AgentContext/RunContext.deps

Test Results

pytest: 116 passed (new regression tests)
No behavioral changes — existing tests pass at baseline rate

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

…y, tool thinning, event passthrough

Replace ~1,500 lines of custom code that duplicates PydanticAI functionality
with thin adapters delegating to native PydanticAI capabilities:

- HooksCapabilityAdapter: bridges AgentPool hooks to
  pydantic_ai.capabilities.Hooks with priority combining (deny > ask > allow)
- ProcessHistory: delegates to pydantic_ai.capabilities.ProcessHistory directly
- Tool thinning: simplified Tool.to_pydantic_ai() conversion, removed
  ToolKind taxonomy and redundant metadata
- Event passthrough: PartStartEvent/PartDeltaEvent use session_id via
  context instead of subclassing
- PromptInjectionManager: inject/consume preserved for tool result
  augmentation; queue/pop_queued removed for native agents (delegated
  to PendingMessageDrainCapability)

Added:
- src/agentpool/agents/native_agent/hooks_capability_adapter.py
- 116 regression tests (hook combining, process history, prompt
  injection, tool conversion, event subclasses)
- RFC-0001, migration guide, CHANGELOG
- OpenSpec change: pydanticai-thinning-refactor (all tasks complete)

No behavioral changes — all existing tests pass at baseline rate.

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

@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 implements a thinning refactor of AgentPool's integration with PydanticAI, delegating hook lifecycle management to PydanticAI's native capabilities via a new HooksCapabilityAdapter. It also adds comprehensive regression tests covering hook combining, process history, prompt injection, tool conversion, and event subclassing. The review feedback suggests robustifying coroutine detection in _execute_hook to support functools.partial and custom callables, replacing deprecated asyncio.get_event_loop() calls, and using explicit is not None checks instead of implicit truthiness checks on dictionary mappings.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +329 to +332
if inspect.iscoroutinefunction(hook):
result = await hook(**kwargs)
else:
result = await asyncio.get_event_loop().run_in_executor(None, lambda: hook(**kwargs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using inspect.iscoroutinefunction directly on the hook callable can fail to detect coroutines if the hook is a functools.partial wrapper or a custom callable object (an instance with a __call__ method). If undetected, calling the async hook synchronously or via run_in_executor will return an unawaited coroutine object, causing the hook to not execute and raising runtime warnings. Additionally, asyncio.get_event_loop() is deprecated in favor of asyncio.get_running_loop() within active async contexts. Avoid using hasattr to check for method or attribute existence; use explicit type checks like isinstance(func, Callable) instead.

Suggested change
if inspect.iscoroutinefunction(hook):
result = await hook(**kwargs)
else:
result = await asyncio.get_event_loop().run_in_executor(None, lambda: hook(**kwargs))
import functools
from collections.abc import Callable
func = hook
while isinstance(func, functools.partial):
func = func.func
is_coro = inspect.iscoroutinefunction(func) or (
isinstance(func, Callable)
and inspect.iscoroutinefunction(func.__call__)
)
if is_coro:
result = await hook(**kwargs)
else:
loop = asyncio.get_running_loop()
result = await loop.run_in_executor(None, functools.partial(hook, **kwargs))
References
  1. Avoid using hasattr to check for method or attribute existence if the project enforces strict type-safety rules. Use explicit type checks, protocols, or other type-safe alternatives instead.

if not pattern.search(tool_name):
return False

if input_match:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Avoid using implicit truthiness checks on objects implementing __len__ or __bool__ (such as dictionaries/mappings) when checking for existence or nullness. Use an explicit is not None check instead to prevent unexpected behavior when the collection is instantiated but empty.

Suggested change
if input_match:
if input_match is not None:
References
  1. Avoid using implicit truthiness checks (e.g., if pool:) to check for the existence or nullness of objects that inherit from MutableMapping or implement __len__/__bool__. Since empty collections evaluate to False in a boolean context, such checks will fail when the object is instantiated but empty. Use explicit is not None checks instead.

@Leoyzen Leoyzen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

.gitignore要更新下。docs 和 specs 是不是单独和 feature 来提交?

@Million-mo

Copy link
Copy Markdown
Collaborator Author

Closing: Superseded by #93

This PR's goals have been substantially achieved by subsequent work on develop/agentic:

This PR's proposal Current status on develop/agentic
HooksCapabilityAdapter (hooks → pydantic_ai.capabilities.Hooks) ✅ Implemented via AgentHooks.as_capability() + _ToolInterceptCapability in hook_manager.py (commit 0089d6c, PR #93)
ProcessHistory delegation ProcessHistoryAdapter in process_history_capability.py
Event passthrough (session_id on PartStart/PartDelta) PartStartEvent/PartDeltaEvent subclass PydanticAI events with session_id field
PromptInjectionManager inject/consume ✅ Preserved in hook_manager.py after_tool_execute
Tool thinning (remove ToolKind) ❌ Not landed — ToolKind still exists in tools/base.py

The branch is 22 commits behind develop/agentic with conflicts. The 116 regression tests have value as behavioral baselines and may be cherry-picked into a future PR.

Action: Closing as superseded. The remaining thinning work (ToolKind removal, ResourceProvider migration) is tracked in openspec/changes/followup-thin-wrapper-refactor/.

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.

2 participants