Skip to content

Conversation

@cjohnhanson
Copy link
Contributor

@cjohnhanson cjohnhanson commented Nov 5, 2025

Enables CallDeferred and ApprovalRequired exceptions to carry arbitrary metadata via an optional metadata parameter. The metadata is accessible in DeferredToolRequests.metadata keyed by tool_call_id.

Backward compatible - metadata defaults to empty dict if not provided.

Enables CallDeferred and ApprovalRequired exceptions to carry arbitrary
metadata via an optional `metadata` parameter. The metadata is accessible
in DeferredToolRequests.metadata keyed by tool_call_id.

This allows tools to:
- Provide cost/time estimates for approval decisions
- Include task IDs for external execution tracking
- Store context about why approval is required
- Attach priority or urgency information

Backward compatible - metadata defaults to empty dict if not provided.
Extract metadata population logic into separate _populate_deferred_calls
helper function to reduce cyclomatic complexity from 16 to 15.
Per Douwe's comments:
1. Store None instead of {} when no metadata provided
2. Don't add tool_call_id to metadata dict when None
3. Update Temporal wrap/unwrap methods to handle metadata

- Updated test assertions to reflect None metadata behavior
- Updated doc example snapshots to show metadata={}
- Fixed codespell issue with table formatting
@cjohnhanson cjohnhanson force-pushed the feat/deferred-tool-metadata branch from 7e17594 to 42cf5b8 Compare November 7, 2025 21:53
- Replace contrived task_id parameter with realistic tool signatures
- Add ComputeDeps class demonstrating dependency injection pattern
- Show using ctx.deps to compute metadata from tool arguments
- Remove incorrect statement about backwards compatibility
- Update test_examples.py to match new realistic example
- Add mock model responses for flight booking example
- Update documentation snapshots to include metadata field
- Follow existing pattern for deferred tool testing
- Complete test_approval_required_without_metadata() by running agent
  with approval results to hit the tool implementation line
- Complete test_mixed_deferred_tools_with_metadata() by running agent
  with all deferred tool results to hit tool implementation lines
- Remove unreachable branch check in _populate_deferred_calls() - keys
  are always present in both dicts by construction

This achieves 100% coverage for the deferred tool metadata feature.
@cjohnhanson cjohnhanson marked this pull request as ready for review November 7, 2025 23:53
@cjohnhanson cjohnhanson requested a review from DouweM November 7, 2025 23:53

_(This example is complete, it can be run "as is" — you'll need to add `asyncio.run(main())` to run `main`)_

## Attaching Metadata to Deferred Tools
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DouweM I wasn't 100% sure on whether this would benefit from a dedicated example in the docs. I could easily shift this to just be included in the existing examples.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As much as I appreciate the Meltano reference in the flight destinations, I think we can make the existing examples stronger by using metadata so we should do it there (with one line explanations of the metadata arg, possibly inside one of those # (1) tooltips) instead of in its own section:

  • The CallDeferred example could have scheduling the task return a task_id that can then be included in metadata, instead of making the TaskResult aware of tool_call_id.
  • the ApprovalRequired example could have reason: 'protected' or something that can be displayed to the user.

- Remove 'Assertio' from codespell ignore-words-list (no actual usage found)
- Remove unused ML training example test expectations (train_model, process_dataset)
  that were replaced with flight booking example in commit d4f1613

_(This example is complete, it can be run "as is" — you'll need to add `asyncio.run(main())` to run `main`)_

## Attaching Metadata to Deferred Tools
Copy link
Collaborator

Choose a reason for hiding this comment

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

As much as I appreciate the Meltano reference in the flight destinations, I think we can make the existing examples stronger by using metadata so we should do it there (with one line explanations of the metadata arg, possibly inside one of those # (1) tooltips) instead of in its own section:

  • The CallDeferred example could have scheduling the task return a task_id that can then be included in metadata, instead of making the TaskResult aware of tool_call_id.
  • the ApprovalRequired example could have reason: 'protected' or something that can be displayed to the user.

approvals: list[ToolCallPart] = field(default_factory=list)
"""Tool calls that require human-in-the-loop approval."""
metadata: dict[str, dict[str, Any]] = field(default_factory=dict)
"""Metadata for deferred tool calls, keyed by tool_call_id.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""Metadata for deferred tool calls, keyed by tool_call_id.
"""Metadata for deferred tool calls, keyed by `tool_call_id`.

assert len(result.output.approvals) == 1
assert result.output.approvals[0].tool_name == 'requires_approval'
# When no metadata is provided, the tool_call_id should not be in metadata dict
tool_call_id = result.output.approvals[0].tool_call_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can revert this to the snapshot -- the metadata type doesn't allow tool_call_id: None anyway

# When no metadata is provided, the tool_call_id should not be in metadata dict
tool_call_id = response.calls[0].tool_call_id
assert tool_call_id not in response.metadata
assert response.metadata == {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as up -- I like snapshots :)


result = agent.run_sync('Hello')
assert isinstance(result.output, DeferredToolRequests)
assert len(result.output.calls) == 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless there's a particular reason not to, lets use snapshots here!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants