Resolve Executor handler annotations with get_type_hints#8
Resolve Executor handler annotations with get_type_hints#8
Conversation
## Summary - resolve class-based @handler annotations using `typing.get_type_hints` (with include_extras when available) - validate WorkflowContext against resolved annotations to support postponed/forward refs - add regression test for `from __future__ import annotations` on Executor handlers ## Testing - added `test_executor_future.py::TestExecutorFutureAnnotations::test_executor_handler_future_annotations` Fixes #1
moonbox3
left a comment
There was a problem hiding this comment.
Automated PR Review Round 1: CHANGES_REQUESTED
Issue: #1
The switch to resolving annotations with typing.get_type_hints is directionally correct for postponed/forward annotations, but the current implementation can raise unexpectedly in cases where ctx/message annotations should be optional and it expands the code-execution surface during handler registration. Additionally, the PR introduces new GitHub Actions workflows that handle high-privilege secrets and execute external code with insufficient hardening (including unpinned actions and PAT-in-URL cloning). Tests cover only a happy path and rely on private internals, leaving key regressions and failure modes unvalidated.
Reviewer summaries:
- Reviewer 1: Correctness fix is promising but get_type_hints is called too eagerly and workflows add unsafe secret-handling and unpinned actions.
- Reviewer 2: Workflows introduce major supply-chain/secret-exfiltration risk, and get_type_hints evaluation during registration can be a security and reliability hazard unless constrained.
- Reviewer 3: Tests are brittle (private internals) and miss negative/edge coverage for unresolved forward refs and the new ValueError behavior.
Blocking issues:
- Remove or split out the newly added GitHub Actions workflows, or harden them substantially: avoid long-lived PATs (especially in clone URLs), do not execute code from separately cloned repos in the same job where high-privilege secrets are present, restrict triggers/gating (e.g., protected environments/approvals), and pin third-party actions to immutable SHAs.
- Fix executor handler validation so typing.get_type_hints is not evaluated when annotations are optional due to explicit @handler(input=..., output=...) types; currently eager evaluation can raise (e.g., unresolved ctx forward refs) before the skip/bypass logic applies.
- Constrain/mitigate typing.get_type_hints evaluation during handler registration: it can evaluate forward refs and execute code via globalns/localns; avoid resolving unless needed, and provide a safe fallback path (or conditional resolution limited to required parameters) when evaluation fails.
- Add targeted tests to cover the new unresolved-annotation failure path (asserting the intended exception/message) and the explicit-types decorator path where ctx/message annotations are omitted or contain unresolved forward refs but should not fail when skipped.
Suggestions:
- Reduce brittleness of tests by asserting via public/stable behavior (e.g., registration/dispatch outcomes) rather than inspecting private _handler_specs internals.
- Consider caching resolved type hints per handler function to avoid repeated get_type_hints cost and repeated evaluation side effects.
- Add coverage for additional postponed/forward-annotation edge cases relevant to supported Python versions (e.g., Optional/Union, local scope types, Annotated/include_extras behavior differences).
| type_hints: dict[str, Any] = {} | ||
| try: | ||
| try: | ||
| type_hints = typing.get_type_hints( |
There was a problem hiding this comment.
typing.get_type_hints() evaluates annotations and may execute code via forward refs/globalns; since handlers may be user-supplied, avoid evaluating unless necessary and/or provide a safe fallback (e.g., use raw annotations when resolution fails, or resolve only specific required parameters).
There was a problem hiding this comment.
Automated status: round 1. Accepted for implementation in the next remediation pass.
| try: | ||
| try: | ||
| type_hints = typing.get_type_hints( | ||
| func, |
There was a problem hiding this comment.
Correctness: eager get_type_hints() runs even when ctx/message annotations should be optional due to explicit @handler(input=..., output=...) types; unresolved ctx forward refs (or problematic return annotations) can raise before reaching the skip/bypass logic. Resolve hints conditionally/only for required params.
There was a problem hiding this comment.
Automated status: round 1. Accepted for implementation in the next remediation pass.
| except TypeError: | ||
| type_hints = typing.get_type_hints(func, globalns=getattr(func, "__globals__", None)) | ||
| except Exception as exc: | ||
| raise ValueError(f"Handler {func.__name__} has unresolved type annotations: {exc}") from exc |
There was a problem hiding this comment.
Behavior change: unresolved annotations now raise ValueError—add a regression test that intentionally hits this path (e.g., unresolved forward ref) so failures are explicit and stable.
There was a problem hiding this comment.
Automated status: round 1. Accepted for implementation in the next remediation pass.
| ) -> None: | ||
| pass | ||
|
|
||
| executor = FutureExecutor(id="future") |
There was a problem hiding this comment.
This asserts on private _handler_specs which is brittle and may not reflect real runtime behavior; prefer asserting via public registration/dispatch behavior and add negative coverage for unresolved forward refs and the explicit-types skip path.
There was a problem hiding this comment.
Automated status: round 1. Accepted for implementation in the next remediation pass.
… return type - Move module docstring before 'from __future__' import so it populates __doc__ (comment #4) - Change find_class return annotation from type[Any] to type to avoid misleading callers about non-type returns like copyreg._reconstructor (comment #2) Comments #1, #3, #5, #6, #7, #8 were already addressed in the current code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rosoft#4941) * Harden Python checkpoint persistence defaults Add RestrictedUnpickler to _checkpoint_encoding.py that limits which types may be instantiated during pickle deserialization. By default FileCheckpointStorage now uses the restricted unpickler, allowing only: - Built-in Python value types (primitives, datetime, uuid, decimal, collections, etc.) - All agent_framework.* internal types - Additional types specified via the new allowed_checkpoint_types parameter on FileCheckpointStorage This narrows the default type surface area for persisted checkpoints while keeping framework-owned scenarios working without extra configuration. Developers can extend the allowed set by passing "module:qualname" strings to allowed_checkpoint_types. The decode_checkpoint_value function retains backward-compatible unrestricted behavior when called without the new allowed_types kwarg. Fixes microsoft#4894 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: resolve mypy no-any-return error in checkpoint encoding Add explicit type annotation for super().find_class() return value to satisfy mypy's no-any-return check. Fixes microsoft#4894 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Simplify find_class return in _RestrictedUnpickler (microsoft#4894) Remove unnecessary intermediate variable and apply # noqa: S301 # nosec directly on the super().find_class() call, matching the established pattern used on the pickle.loads() call in the same file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for microsoft#4894: Python: Harden Python checkpoint persistence defaults * Restore # noqa: S301 on line 102 of _checkpoint_encoding.py (microsoft#4894) The review feedback correctly identified that removing the # noqa: S301 suppression from the find_class return statement would cause a ruff S301 lint failure, since the project enables bandit ("S") rules. This restores consistency with lines 82 and 246 in the same file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for microsoft#4894: Python: Harden Python checkpoint persistence defaults * Address PR review comments on checkpoint encoding (microsoft#4894) - Move module docstring to proper position after __future__ import - Fix find_class return type annotation to type[Any] - Add missing # noqa: S301 pragma on find_class return - Improve error message to reference both allowed_types param and FileCheckpointStorage.allowed_checkpoint_types - Add -> None return annotation to FileCheckpointStorage.__init__ - Replace tempfile.mktemp with TemporaryDirectory in test - Replace contextlib.suppress with pytest.raises for precise assertion - Remove unused contextlib import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR microsoft#4941 review comments: fix docstring position and return type - Move module docstring before 'from __future__' import so it populates __doc__ (comment #4) - Change find_class return annotation from type[Any] to type to avoid misleading callers about non-type returns like copyreg._reconstructor (comment #2) Comments #1, #3, #5, #6, #7, #8 were already addressed in the current code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for microsoft#4894: review comment fixes * fix: use pickle.UnpicklingError in RestrictedUnpickler and improve docstring (microsoft#4894) - Change _RestrictedUnpickler.find_class to raise pickle.UnpicklingError instead of WorkflowCheckpointException, since it is pickle-level concern that gets wrapped by the caller in _base64_to_unpickle. - Remove now-unnecessary WorkflowCheckpointException re-raise in _base64_to_unpickle (pickle.UnpicklingError is caught by the generic except Exception handler and wrapped). - Expand decode_checkpoint_value docstring to show a concrete example of the module:qualname format with a user-defined class. - Add regression test verifying find_class raises pickle.UnpicklingError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address PR microsoft#4941 review comments for checkpoint encoding - Comment 1 (line 103): Already resolved in prior commit — _RestrictedUnpickler now raises pickle.UnpicklingError instead of WorkflowCheckpointException. - Comment 2 (line 140): Add concrete usage examples to decode_checkpoint_value docstring showing both direct allowed_types usage and FileCheckpointStorage allowed_checkpoint_types usage. Rename 'SafeState' to 'MyState' across all docstrings for consistency, making it clear this is a user-defined class name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: replace deprecated 'builtin' repo with pre-commit-hooks in pre-commit config pre-commit 4.x no longer supports 'repo: builtin'. Merge those hooks into the existing pre-commit-hooks repo entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * style: apply pyupgrade formatting to docstring example Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: resolve pre-commit hook paths for monorepo git root The poe-check and bandit hooks referenced paths relative to python/ but pre-commit runs hooks from the git root (monorepo root). Fix poe-check entry to cd into python/ first, and update bandit config path to python/pyproject.toml. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix pre-commit config paths for prek --cd python execution Revert bandit config path from 'python/pyproject.toml' to 'pyproject.toml' and poe-check entry from explicit 'cd python' wrapper to direct invocation, since prek --cd python already sets the working directory to python/. Also apply ruff formatting fixes to cosmos checkpoint storage files. Fixes microsoft#4894 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: add builtins:getattr to checkpoint deserialization allowlist Pickle uses builtins:getattr to reconstruct enum members (e.g., WorkflowMessage.type which is a MessageType enum). Without it in the allowlist, checkpoint roundtrip tests fail with WorkflowCheckpointException. Fixes microsoft#4894 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for microsoft#4894: review comment fixes --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
typing.get_type_hints(with include_extras when available)from __future__ import annotationson Executor handlersTesting
test_executor_future.py::TestExecutorFutureAnnotations::test_executor_handler_future_annotations