Skip to content

Improve capabilities docs#4832

Open
dsfaccini wants to merge 12 commits intopydantic:mainfrom
dsfaccini:bugs-in-capabilities
Open

Improve capabilities docs#4832
dsfaccini wants to merge 12 commits intopydantic:mainfrom
dsfaccini:bugs-in-capabilities

Conversation

@dsfaccini
Copy link
Copy Markdown
Collaborator

Summary

Bugs found

  1. Hooks after_* fires forward, should reverse — violates the documented contract in docs/hooks.md ("after_* hooks fire in reverse order" including "on the same Hooks instance")
  2. DynamicToolset for_run_step() no error recovery — if factory or __aenter__ raises, old toolset is lost with no rollback
  3. on_*_error handlers chain-replace original error — when handler A raises a new exception, handler B sees the new error, not the original
  4. Capability returning self from for_run() with mutated state uses stale cache — identity check misses mutations when for_run() returns self
  5. Tool retry count persists across DynamicToolset tool swaps — ToolManager tracks retries by name, so swapped implementations inherit old counts
  6. History processor composition creates orphaned tool returns — no validation that processed messages remain semantically consistent

Test plan

  • uv run pytest tests/test_capabilities_bugs.py -v — 12 tests, all pass

  • make lint — clean

  • make typecheck — clean (pyright passes)

  • AI generated code

🤖 Generated with Claude Code

Dissect PR pydantic#4640 (Capability abstraction) and identify 6 bugs:

1. Hooks after_* fires forward, should reverse (violates docs/hooks.md contract)
2. DynamicToolset for_run_step() has no error recovery on factory/enter failure
3. on_*_error handlers chain-replace original error, losing context
4. Capability returning self from for_run() with mutated state uses stale cache
5. Tool retry count persists across DynamicToolset tool swaps
6. History processor composition can create orphaned tool returns

Each bug has a reproducing test in tests/test_capabilities_bugs.py.
DOCS-QUESTIONS.md lists doc gaps found by a separate review.
NICHE-DOCS.md has caveat notes for bugs 2, 5, 6 (niche edge cases).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added size: L Large PR (501-1500 weighted lines) bug Report that something isn't working, or PR implementing a fix labels Mar 24, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@github-actions github-actions bot added size: M Medium PR (101-500 weighted lines) and removed size: L Large PR (501-1500 weighted lines) labels Mar 25, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

dsfaccini and others added 2 commits March 25, 2026 13:17
Exit old toolset before calling factory in for_run_step(), so that
if the factory or new toolset's __aenter__ raises, the old toolset
has been properly cleaned up.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 7 additional findings in Devin Review.

Open in Devin Review

Comment on lines 85 to 94
# Exit old toolset before evaluating factory
old_toolset = self._toolset
self._toolset = None
if old_toolset is not None:
await old_toolset.__aexit__(None, None, None)

# Manage the transition in-place
if self._toolset is not None:
await self._toolset.__aexit__(None, None, None)
new_toolset = await self._evaluate_factory(ctx)
self._toolset = new_toolset
if self._toolset is not None:
await self._toolset.__aenter__()
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 25, 2026

Choose a reason for hiding this comment

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

🚩 DynamicToolset identity check removal changes behavior for stable-instance factories

The old for_run_step had an optimization: if new_toolset is self._toolset: return self which skipped the __aexit__/__aenter__ cycle when the factory returned the same toolset instance. The new code always exits the old toolset before evaluating the factory, making this optimization impossible. For factories that return the same stateful toolset (e.g., one with real connection resources in __aenter__/__aexit__), this means every run step will close and reopen those resources unnecessarily.

The existing test test_dynamic_toolset_for_run_step_same_instance_skips_transition at tests/test_toolsets.py:1116 still passes, but only because FunctionToolset has no-op lifecycle methods. Its docstring ("skips transition when factory returns the same instance") and inline comment ("early return without transition") no longer describe actual behavior.

This appears to be a deliberate design tradeoff (cleanup safety over performance), not a bug — but the test documentation is now misleading, and downstream users relying on identity-based skip behavior may notice a behavioral change.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

Devin found the same on my PR :D #4846 (comment)

@DouweM DouweM closed this in #4846 Mar 25, 2026
@DouweM DouweM reopened this Mar 27, 2026
@DouweM
Copy link
Copy Markdown
Collaborator

DouweM commented Mar 27, 2026

@dsfaccini Didn't mean to close this; you'll just want to fix merge conflicts / see what I already fixed that's not needed here anymore.

# Conflicts:
#	docs/capabilities.md
#	pydantic_ai_slim/pydantic_ai/toolsets/_dynamic.py
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 4 new potential issues.

View 9 additional findings in Devin Review.

Open in Devin Review

Comment on lines +1 to +11
# `pydantic_ai.agent.spec`

::: pydantic_ai.agent.spec
options:
members:
- AgentSpec

::: pydantic_ai._template
options:
members:
- TemplateStr
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 New docs/api/agent_spec.md page not added to mkdocs.yml navigation

The new API reference page docs/api/agent_spec.md (documenting AgentSpec and TemplateStr) was created but never added to the mkdocs.yml nav section. All other API reference pages are listed under API Reference > pydantic_ai (see mkdocs.yml:140-187), but api/agent_spec.md is missing. This means users cannot discover the page through site navigation, and the TemplateStr API docs (which are only rendered via the ::: directive in this file) are effectively hidden. The page should be added between api/agent.md and api/builtin_tools.md in the nav.

Prompt for agents
Add docs/api/agent_spec.md to the mkdocs.yml navigation. In mkdocs.yml, find the API Reference > pydantic_ai section (around line 143) where api/agent.md is listed, and add api/agent_spec.md immediately after it, before api/builtin_tools.md.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

Good point Devin

```

[`Agent.from_spec`][pydantic_ai.Agent.from_spec] accepts a dict or [`AgentSpec`][pydantic_ai.agent.spec.AgentSpec] instance and supports additional keyword arguments that supplement or override the spec:
[`Agent.from_spec`][pydantic_ai.agent.Agent.from_spec] accepts a dict or [`AgentSpec`](#agentspec-reference) instance and supports additional keyword arguments that supplement or override the spec:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 AgentSpec reference changed from API xref to anchor link, violating doc rules

The AgentSpec reference was changed from [AgentSpec][pydantic_ai.agent.spec.AgentSpec] to [AgentSpec](#agentspec-reference). This violates docs/.cursor/rules.mdc ("Always reference the python code in the docs e.g. ModelSettings should link to [ModelSettings][pydantic_ai.settings.ModelSettings]") and docs/AGENTS.md rule:66 ("Use reference-style links for API elements: [ElementName][module.path.ElementName]"). Notably, the same file still uses proper API xrefs for AgentSpec in other places (docs/agent-spec.md:70, docs/agent-spec.md:173), making this inconsistent.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

## `AgentSpec` reference

The [`AgentSpec`][pydantic_ai.agent.spec.AgentSpec] model represents the full spec structure:
The [`AgentSpec`](#agentspec-reference) model represents the full spec structure:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Second AgentSpec reference changed from API xref to anchor link, violating doc rules

Same violation as in line 37: AgentSpec is referenced as [AgentSpec](#agentspec-reference) instead of [AgentSpec][pydantic_ai.agent.spec.AgentSpec], violating the mandatory documentation rules in docs/.cursor/rules.mdc and docs/AGENTS.md rule:66 that require API elements to use reference-style links.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

This now links to this same section lol. Should link to API.

@DouweM DouweM changed the title bug: Capabilities review Improve capabilities docs Mar 27, 2026
Comment on lines +1 to +11
# `pydantic_ai.agent.spec`

::: pydantic_ai.agent.spec
options:
members:
- AgentSpec

::: pydantic_ai._template
options:
members:
- TemplateStr
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.

Good point Devin

## `AgentSpec` reference

The [`AgentSpec`][pydantic_ai.agent.spec.AgentSpec] model represents the full spec structure:
The [`AgentSpec`](#agentspec-reference) model represents the full spec structure:
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.

This now links to this same section lol. Should link to API.

::: pydantic_ai.agent.spec
options:
members:
- AgentSpec
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.

I wouldn't mind this one being documented on api/agent.md

members:
- AgentSpec

::: pydantic_ai._template
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.

@dsfaccini You know this is wrong :) Please review your own PRs!

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.

@dsfaccini Weren't there a ton of docs gaps that were going to be addressed as part of this PR?

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 5 new potential issues.

View 9 additional findings in Devin Review.

Open in Devin Review

## Template strings

[`TemplateStr`][pydantic_ai.TemplateStr] provides Handlebars-style templates (`{{variable}}`) that are rendered against the agent's [dependencies](dependencies.md) at runtime. In spec files, strings containing `{{` are automatically converted to template strings:
`TemplateStr` provides Handlebars-style templates (`{{variable}}`) that are rendered against the agent's [dependencies](dependencies.md) at runtime. In spec files, strings containing `{{` are automatically converted to template strings:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 TemplateStr link removed in docs/agent-spec.md line 74, violating mandatory docs linking rule

The mandatory rule in docs/.cursor/rules.mdc requires: "Always reference the python code in the docs e.g. ModelSettings should link to [ModelSettings][pydantic_ai.settings.ModelSettings]." This line was changed from [TemplateStr][pydantic_ai.TemplateStr] to bare `TemplateStr`, actively removing the API reference link. TemplateStr is exported from pydantic_ai (pydantic_ai_slim/pydantic_ai/__init__.py:3).

Suggested change
`TemplateStr` provides Handlebars-style templates (`{{variable}}`) that are rendered against the agent's [dependencies](dependencies.md) at runtime. In spec files, strings containing `{{` are automatically converted to template strings:
[`TemplateStr`][pydantic_ai.TemplateStr] provides Handlebars-style templates (`{{variable}}`) that are rendered against the agent's [dependencies](dependencies.md) at runtime. In spec files, strings containing `{{` are automatically converted to template strings:
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Template variables are resolved from the fields of the `deps` object. When a `deps_type` (or [`deps_schema`](#deps_schema)) is provided, template variable names are validated at construction time.

In Python code, [`TemplateStr`][pydantic_ai.TemplateStr] can be used explicitly, but a callable with [`RunContext`][pydantic_ai.tools.RunContext] is generally preferred for IDE autocomplete and type checking:
In Python code, `TemplateStr` can be used explicitly, but a callable with [`RunContext`][pydantic_ai.tools.RunContext] is generally preferred for IDE autocomplete and type checking:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 TemplateStr link removed in docs/agent-spec.md line 82, violating mandatory docs linking rule

Same mandatory rule violation as BUG-0001. This line was changed from [TemplateStr][pydantic_ai.TemplateStr] to bare `TemplateStr`, removing the API reference link.

Suggested change
In Python code, `TemplateStr` can be used explicitly, but a callable with [`RunContext`][pydantic_ai.tools.RunContext] is generally preferred for IDE autocomplete and type checking:
In Python code, [`TemplateStr`][pydantic_ai.TemplateStr] can be used explicitly, but a callable with [`RunContext`][pydantic_ai.tools.RunContext] is generally preferred for IDE autocomplete and type checking:
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

`before_model_request` hooks see the full `request_context.messages` list, including any [message history](message-history.md) passed to `agent.run()`, and can modify it.

!!! note "Skip and chain behavior"
All skip exceptions (`SkipModelRequest`, `SkipToolValidation`, `SkipToolExecution`) short-circuit the hook chain: remaining capabilities' `before_*` hooks do not fire, and `after_*` hooks are not called for the skipped operation. A skip raised from `wrap_*` propagates immediately — inner capabilities' wrap hooks never execute.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 SkipModelRequest, SkipToolValidation, SkipToolExecution unlinked in new docs content

The mandatory rule in docs/.cursor/rules.mdc requires all Python code references to be linked. Line 558 introduces new content with three bare-backtick exception class references (SkipModelRequest, SkipToolValidation, SkipToolExecution) that are not linked to their API paths. These are all in pydantic_ai.exceptions and are linked correctly elsewhere in the same file (e.g., docs/capabilities.md:553).

Suggested change
All skip exceptions (`SkipModelRequest`, `SkipToolValidation`, `SkipToolExecution`) short-circuit the hook chain: remaining capabilities' `before_*` hooks do not fire, and `after_*` hooks are not called for the skipped operation. A skip raised from `wrap_*` propagates immediately — inner capabilities' wrap hooks never execute.
All skip exceptions ([`SkipModelRequest`][pydantic_ai.exceptions.SkipModelRequest], [`SkipToolValidation`][pydantic_ai.exceptions.SkipToolValidation], [`SkipToolExecution`][pydantic_ai.exceptions.SkipToolExecution]) short-circuit the hook chain: remaining capabilities' `before_*` hooks do not fire, and `after_*` hooks are not called for the skipped operation. A skip raised from `wrap_*` propagates immediately — inner capabilities' wrap hooks never execute.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

| [`before_tool_validate`][pydantic_ai.capabilities.AbstractCapability.before_tool_validate] | `(ctx: `[`RunContext`][pydantic_ai.tools.RunContext]`, *, call: `[`ToolCallPart`][pydantic_ai.messages.ToolCallPart]`, tool_def: `[`ToolDefinition`][pydantic_ai.tools.ToolDefinition]`, args: `[`RawToolArgs`][pydantic_ai.capabilities.RawToolArgs]`) -> `[`RawToolArgs`][pydantic_ai.capabilities.RawToolArgs] | Modify raw args before validation (e.g. JSON repair) |
| [`after_tool_validate`][pydantic_ai.capabilities.AbstractCapability.after_tool_validate] | `(ctx: `[`RunContext`][pydantic_ai.tools.RunContext]`, *, call: `[`ToolCallPart`][pydantic_ai.messages.ToolCallPart]`, tool_def: `[`ToolDefinition`][pydantic_ai.tools.ToolDefinition]`, args: `[`ValidatedToolArgs`][pydantic_ai.capabilities.ValidatedToolArgs]`) -> `[`ValidatedToolArgs`][pydantic_ai.capabilities.ValidatedToolArgs] | Modify validated args |
| [`wrap_tool_validate`][pydantic_ai.capabilities.AbstractCapability.wrap_tool_validate] | `(ctx: `[`RunContext`][pydantic_ai.tools.RunContext]`, *, call: `[`ToolCallPart`][pydantic_ai.messages.ToolCallPart]`, tool_def: `[`ToolDefinition`][pydantic_ai.tools.ToolDefinition]`, args: `[`RawToolArgs`][pydantic_ai.capabilities.RawToolArgs]`, handler: `[`WrapToolValidateHandler`][pydantic_ai.capabilities.WrapToolValidateHandler]`) -> `[`ValidatedToolArgs`][pydantic_ai.capabilities.ValidatedToolArgs] | Wrap the validation step |
| [`on_tool_validate_error`][pydantic_ai.capabilities.AbstractCapability.on_tool_validate_error] | `(ctx: `[`RunContext`][pydantic_ai.tools.RunContext]`, *, call: `[`ToolCallPart`][pydantic_ai.messages.ToolCallPart]`, tool_def: `[`ToolDefinition`][pydantic_ai.tools.ToolDefinition]`, args: `[`RawToolArgs`][pydantic_ai.capabilities.RawToolArgs]`, error: ValidationError | `[`ModelRetry`][pydantic_ai.exceptions.ModelRetry]`) -> `[`ValidatedToolArgs`][pydantic_ai.capabilities.ValidatedToolArgs] | Handle validation errors (see [error hooks](#error-hooks)) |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 ValidationError unlinked in on_tool_validate_error signature table

The mandatory rule in docs/.cursor/rules.mdc requires all Python code references to be linked. In the changed on_tool_validate_error signature on line 573, ValidationError (from pydantic) is bare-backticked while ModelRetry next to it is properly linked. The old version had Exception (which is a builtin), but the new ValidationError is a pydantic class that should be linked as [ValidationError][pydantic.ValidationError] per the docs rule.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +432 to +433
| [`get_instructions()`][pydantic_ai.capabilities.AbstractCapability.get_instructions] | [`AgentInstructions`][pydantic_ai.agent.AgentInstructions] \| `None` | [Instructions](agent.md#instructions) (static strings, [template strings](agent-spec.md#template-strings), or callables) |
| [`get_model_settings()`][pydantic_ai.capabilities.AbstractCapability.get_model_settings] | [`AgentModelSettings`][pydantic_ai.agent.AgentModelSettings] \| `None` | [Model settings](agent.md#model-run-settings) dict, or a callable for per-step settings |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚩 AgentInstructions and AgentModelSettings link paths changed in configuration methods reference table

Lines 432-433 change the cross-reference paths for AgentInstructions from pydantic_ai._instructions.AgentInstructions to pydantic_ai.agent.AgentInstructions, and AgentModelSettings from pydantic_ai.agent.abstract.AgentModelSettings to pydantic_ai.agent.AgentModelSettings. This aligns with the new __all__ export added in pydantic_ai_slim/pydantic_ai/agent/__init__.py:114 ('AgentInstructions'). However, AgentInstructions is defined in pydantic_ai._instructions — these links will only resolve if mkdocstrings can follow the re-export chain. Worth verifying with make docs-serve that these links actually resolve.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Report that something isn't working, or PR implementing a fix size: M Medium PR (101-500 weighted lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants