Repository navigation
Python: resolve postponed @response_handler annotations - #8328
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Add fallback-path coverage and verify workflow_output_types registration for two-argument contexts.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes postponed-annotation handling for Python @response_handler methods.
Changes:
- Resolve annotations with
typing.get_type_hints, with raw-annotation fallback. - Add regression coverage for future annotations.
File summaries
| File | Summary |
|---|---|
python/packages/core/tests/workflow/test_executor_future.py |
Tests response-handler registration with postponed annotations. |
python/packages/core/agent_framework/_workflows/_request_info_mixin.py |
Resolves response-handler parameter annotations. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Ricky-7-Yan
left a comment
There was a problem hiding this comment.
Reviewed current head cf3e412. Resolving annotations with typing.get_type_hints() and using the same (NameError, AttributeError, RecursionError) raw-annotation fallback as the existing @handler and function-executor validation paths keeps the decorator behaviors aligned. The regression exercises the public class-decoration/discovery path, verifies request/response plus both WorkflowContext type arguments, and the added unresolved-reference case covers the fallback diagnostic. I found no blocking issue in the scoped diff.
The full upstream Python workflows have not run on this external branch; this approval relies on the current code/tests and the author's reported full core suite, Ruff, and Pyright results rather than treating the governance checks as full CI.
|
Thanks for the update. Before this is ready, could you please:
Once those are addressed, please re-request review. Thanks! |
|
Hi Evan Mattson (@moonbox3) , I’ve resolved the remaining outdated review discussions at _request_info_mixin.py:387 and _request_info_mixin.py:358, and confirmed that there are no unresolved review conversations left. I’ve re-requested your review—please take another look when you have a chance. Thank you for your time! |
|
Please fix failing CI/CD checks. |
|
Hi Evan Mattson (@moonbox3) , I’ve addressed the failing CI/CD checks and pushed the fixes in commits 7108dc7 and 48d8156. The relevant tests, Ruff, and Pyrefly checks pass locally. The PR is ready for another review when you have a chance. Thank you for pointing this out and for your time. |
Motivation & Context
The introspection path of
@response_handlerreads raw values frominspect.signature(func). Withfrom __future__ import annotations, valid request, response, andWorkflowContext[...]annotations are strings, so a valid executor class currently fails during decoration withValueErrorand cannot be imported or instantiated. The existing@handlerpath already resolves these annotations; issue #8327 tracks the missing response-handler counterpart.The bug was reproduced on clean upstream commit
3c670707766a8455da6491a9049cc9d575e019f0with a four-parameter response handler usingoriginal_request: str,response: int, andctx: WorkflowContext[str].Description & Review Guide
typing.get_type_hints(func)during signature validation, matching the existing@handlerbehavior.@response_handlermethods normally.ValueError: Response handler parameter 'ctx' must be annotated ... got WorkflowContext[str]; after the fix it registers(str, int)with workflow output typestr.5044 passed, 131 skipped, 2 xfailed; relevant request-info, executor, and future-annotation tests passed; Ruff format/lint passed; Pyright reported0 errors, 0 warningsfor both changed files._validate_handler_signature.Related Issue
Fixes #8327
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.