Skip to content

fix(agents): resolve string annotations in tools - #2687

Merged
kevinmessiaen merged 4 commits into
Giskard-AI:mainfrom
BILLKISHORE:fix/tool-pep563-annotations
Aug 5, 2026
Merged

fix(agents): resolve string annotations in tools#2687
kevinmessiaen merged 4 commits into
Giskard-AI:mainfrom
BILLKISHORE:fix/tool-pep563-annotations

Conversation

@BILLKISHORE

@BILLKISHORE BILLKISHORE commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

What

@tool breaks in any module using from __future__ import annotations (PEP 563), where every annotation reaches inspect.signature as a string.

Tools taking a user-defined type raise at decoration time:

pydantic.errors.PydanticUserError: `move` is not fully defined;
you should define `Point`, then call `move.model_rebuild()`.

create_model receives the string "Point" and tries to resolve it in tool.py's namespace instead of the caller's.

The quieter failure is RunContext. Tool.from_callable detects it with param.annotation is RunContext, which is never true against the string "RunContext". For def needs_ctx(text: str, ctx: RunContext):

run_context_param = None              (expected: 'ctx')
schema properties = ['text', 'ctx']   (expected: ['text'])
required = ['text', 'ctx']
result = ERROR: 1 validation error for needs_ctx
         ctx  Field required

Context injection stops, ctx leaks into the model-facing schema as a required property, and the resulting validation error is swallowed by the default catch handler and returned to the model as an ordinary tool result.

Fix

-        sig = inspect.signature(fn)
+        sig = inspect.signature(fn, eval_str=True)

eval_str=True resolves annotations against fn.__globals__, which is the defining module.

One behaviour change worth flagging: a type imported only under if TYPE_CHECKING: now fails with NameError: name 'Decimal' is not defined where it previously failed with PydanticUserError: 'price' is not fully defined. Both fail at decoration time, before and after, so nothing that used to work stops working. The message just points at the real cause.

Testing

New libs/giskard-agents/tests/test_tools_future_annotations.py, 4 tests covering RunContext detection, schema exclusion, injection, and a model-annotated parameter. All 4 fail before the change and pass after, on 3.13. I also checked directly that inspect.signature resolves these annotations the same way on 3.12, 3.13 and 3.14.

make test-unit PACKAGE=giskard-agents: 121 passed, 2 skipped. Full suite across all packages: 1281 passed. ruff check and basedpyright clean.

xref: no open PR touches tools/tool.py.

Note: the lint job will be red here until #2686 lands. Unrelated to this change.

Type of Change

  • 🔧 Bug fix (non-breaking change which fixes an issue)

Disclosure: drafted with AI assistance (Claude Code). I reproduced both failure modes, confirmed the tests fail before the fix and pass after, and ran the checks myself before submitting.

Under `from __future__ import annotations` the RunContext parameter was never
detected, so it leaked into the model-facing schema and context injection
silently stopped.
@BILLKISHORE BILLKISHORE mentioned this pull request Aug 2, 2026
Assert on the tool's returned string, not just the RunContext side
effect, so the test pins the pre-fix "swallowed validation error"
failure mode instead of only its symptom. Add coverage for the
TYPE_CHECKING-only-import behavior change called out in the PR
description (PydanticUserError -> NameError, both at decoration time).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@kevinmessiaen kevinmessiaen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix @BILLKISHORE

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

Development

Successfully merging this pull request may close these issues.

3 participants