Resolve postponed handler annotations during signature validation by moonbox3 · Pull Request #7 · moonbox3/agent-framework

moonbox3 · 2026-02-16T09:09:49Z

Summary

resolve postponed handler annotations with typing.get_type_hints
raise clear errors when message or context annotations cannot be resolved
add regression tests for resolved future annotations and unresolved forward references

Testing

python/packages/core/tests/workflow/test_executor_postponed_annotations.py::test_postponed_annotations_are_resolved
python/packages/core/tests/workflow/test_executor_postponed_annotations.py::test_unresolved_message_annotation_raises
python/packages/core/tests/workflow/test_executor_postponed_annotations.py::test_unresolved_context_annotation_raises

## Summary - resolve postponed handler annotations via `typing.get_type_hints` in `_validate_handler_signature` - add clearer error when WorkflowContext validation receives unresolved string annotations - add regression test for `from __future__ import annotations` ## Testing - `python/tests/test_executor_postponed_annotations.py::test_postponed_annotations_are_resolved` Fixes #1

moonbox3

Automated PR Review Round 1: CHANGES_REQUESTED

Issue: #1
The core change—resolving postponed annotations via typing.get_type_hints() before validating handler signatures—addresses the reported correctness bug and the added regression test covers the basic happy-path. However, the PR also introduces unrelated GitHub Actions workflows that clone/execute external private code using secrets (high supply-chain/secrets risk), and the new type-hint resolution path currently swallows get_type_hints() failures which can silently fall back to unresolved annotations. Test coverage is also too narrow for the newly introduced failure/error branches. As-is, this is not safe to merge without changes.

Reviewer summaries:

Reviewer 1: Fix is directionally correct and improves support for from __future__ import annotations, but the PR includes out-of-scope, high-risk GitHub Actions workflows and has unsafe/broad exception handling around get_type_hints(); tests lack coverage for failure modes.
Reviewer 2: Main correctness fix looks sound, yet the added workflows materially increase security exposure and should not be merged as part of this issue; get_type_hints() errors are swallowed leading to potentially misleading validation; add negative tests.
Reviewer 3: Happy-path regression test exists, but new branches (type hint resolution failure and unresolved string annotation error paths) are untested; request changes to strengthen tests and assertions.

Blocking issues:

Remove or split the new GitHub Actions workflows that clone and execute external/private repo code using privileged secrets; this is out of scope for issue #1 and introduces significant supply-chain/secrets exposure risk.
In _validate_handler_signature(), do not broadly catch and ignore typing.get_type_hints() failures while continuing validation. Narrow the exception handling and/or fail fast with a clear error when required annotations cannot be resolved to avoid silent mis-validation and confusing downstream errors.
Add tests covering failure modes: ensure an unknown forward reference (or other get_type_hints() failure) triggers deterministic, clear errors (including checking the intended error message and exception chaining) rather than silently continuing; also strengthen assertions beyond just output-type extraction.

Suggestions:

Consider providing an explicit localns/globalns to typing.get_type_hints() (where appropriate) to improve resolution of nested/local types, or document limitations.
Improve error messages for unresolved string annotations by including the original annotation string (safely) to aid debugging.
Remove the redundant ctx_annotation = ctx_annotation assignment to avoid confusion.
If workflows are ever reintroduced in a separate PR, harden them: avoid embedding tokens in clone URLs, pin actions/images to immutable SHAs/digests, and minimize secret scope.

moonbox3 · 2026-02-16T09:10:50Z

+    type_hint_error: Exception | None = None
+    try:
+        type_hints = typing.get_type_hints(
+            func,


Blocking: Catching broad Exception from typing.get_type_hints() can mask real resolution/runtime issues and then continue with unresolved/incorrect annotations. Narrow the exceptions and/or fail fast when required annotations can’t be resolved.

moonbox3 · 2026-02-16T09:10:50Z

        # the ctx parameter doesn't need a type annotation - types come from the decorator.
        output_types: list[type[Any] | types.UnionType] = []
        workflow_output_types: list[type[Any] | types.UnionType] = []
    else:


Non-blocking: ctx_annotation = ctx_annotation is redundant and reads like a leftover. Consider removing for clarity.

moonbox3 · 2026-02-16T09:10:50Z

+def test_postponed_annotations_are_resolved() -> None:
+    executor = PostponedAnnotationExecutor(id="postponed")
+    assert set(executor.output_types) == {OutputTypeA}
+    assert set(executor.workflow_output_types) == {OutputTypeB}


Blocking: Test only covers the happy-path. Add at least one failure-mode test where get_type_hints() fails (e.g., undefined forward ref) and assert the intended ValueError message and exception chaining, so the new error-handling branches are covered.

moonbox3 · 2026-02-16T09:11:10Z

Automated PR Comment Resolution Round 1

Verdict: changes_required
Two blocking review comments require code/test updates: (1) avoid broadly swallowing exceptions from typing.get_type_hints (or fail fast deterministically), and (2) add a regression test that forces get_type_hints failure to exercise the new error-handling/exception chaining. Also remove a redundant assignment noted as non-blocking cleanup.

Per-comment decisions:

Comment 2811362816: fix - Current implementation catches broad Exception from typing.get_type_hints and then continues, which can hide genuine errors and create inconsistent behavior. Need to narrow the exception handling and/or ensure we fail fast when required annotations cannot be resolved, rather than proceeding with potentially wrong annotations.
Comment 2811362821: fix - The line ctx_annotation = ctx_annotation is a no-op and looks like leftover scaffolding; removing it improves clarity with no functional impact.
Comment 2811362827: fix - Test suite currently only validates the successful resolution path. A failure-mode test is required to cover the branch where get_type_hints fails and confirm we raise the intended ValueError with proper message and exception chaining, ensuring the new behavior is stable and validated.

Blocking issues selected for code changes:

Broad exception catching around typing.get_type_hints can mask real resolution/runtime issues and allow validation to proceed with unresolved/incorrect annotations; adjust to narrow exceptions and/or fail fast when annotations are required.
Missing test coverage for get_type_hints failure path; add a failure-mode test asserting ValueError message and exception chaining.

Suggestions selected for implementation:

Refactor annotation resolution into a small helper that either returns resolved hints or raises a dedicated ValueError when required parameters (message/ctx) cannot be resolved; this will simplify logic and make failure behavior explicit.
In the failure-mode test, define the handler using a forward reference to an undefined name (under postponed annotations) to reliably trigger get_type_hints failure, then assert exc_info.value.__cause__ is set and message mentions unresolved annotation for the specific parameter.
Remove redundant ctx_annotation = ctx_annotation assignment for readability.

moonbox3 · 2026-02-16T09:11:22Z

Automated Feedback Disposition Round 1

Prioritize fixing annotation resolution failure handling and adding failure-mode tests; defer workflow-related/out-of-scope items and low-impact cleanup.

Implementing now (blocking):

Adjust _validate_handler_signature() to fail fast when required annotations cannot be resolved via typing.get_type_hints (avoid broad catch-and-continue) so validation does not proceed with unresolved types.
Add failure-mode tests that trigger get_type_hints resolution errors (e.g., unknown forward reference) and assert deterministic ValueError messaging plus exception chaining (including parameter name).

Implementing now (suggestions):

Remove redundant ctx_annotation = ctx_annotation assignment for clarity.

Deferred for now:

Remove or split the new GitHub Actions workflows that clone and execute external/private repo code using privileged secrets; this is out of scope for issue Python: [Bug]: Executor type checking fails when using __future__ annotations in Python 3.12 #1 and introduces significant supply-chain/secrets exposure risk.
Rationale: Out of scope for the current issue/PR; should be handled in a separate security-focused change.
Broad exception catching around typing.get_type_hints can mask real resolution/runtime issues and allow validation to proceed with unresolved/incorrect annotations; adjust to narrow exceptions and/or fail fast when annotations are required.
Rationale: Covered by the implement-blocking item to fail fast on unresolved required annotations.
Missing test coverage for get_type_hints failure path; add a failure-mode test asserting ValueError message and exception chaining.
Rationale: Already included in the implement-blocking test coverage item.
Consider providing an explicit localns/globalns to typing.get_type_hints (where appropriate) to improve resolution of nested/local types, or document limitations.
Rationale: Optional enhancement; not required to resolve the reported bug.
Improve error messages for unresolved string annotations by including the original annotation string (safely) to aid debugging.
Rationale: Nice-to-have; current errors are sufficient for the fix scope.
If workflows are ever reintroduced in a separate PR, harden them: avoid embedding tokens in clone URLs, pin actions/images to immutable SHAs/digests, and minimize secret scope.
Rationale: Out of scope for this issue and PR; should be handled in a dedicated security hardening effort.
Refactor annotation resolution into a small helper that either returns resolved hints or raises a dedicated ValueError when required parameters (message/ctx) cannot be resolved; this will simplify logic and make failure behavior explicit.
Rationale: Refactor is optional and larger than needed for the targeted fix.
In the failure-mode test, define the handler using a forward reference to an undefined name (under postponed annotations) to reliably trigger get_type_hints failure, then assert exc_info.value.cause is set and message mentions unresolved annotation for the specific parameter.
Rationale: Specific approach can be adopted while implementing the failure-mode test; the core requirement is already in implement-blocking.
Remove redundant ctx_annotation = ctx_annotation assignment for readability.
Rationale: Already included as an implement suggestion.

moonbox3

Automated PR Review Round 2: CHANGES_REQUESTED

Issue: #1
Fix addresses issue #1 by resolving postponed (string) annotations via typing.get_type_hints(..., include_extras=True) during handler signature validation and adds a happy-path regression test. However, the PR also introduces unrelated, high-risk GitHub Actions workflows that clone/execute external/private repositories with secrets, and the new type-hint resolution error handling/tests are not strict enough to guarantee deterministic, debuggable failure behavior. Request changes before merge.

Reviewer summaries:

Reviewer 1: Resolves postponed annotations using typing.get_type_hints and adds a regression test, but adds risky out-of-scope CI workflows that execute external code with secrets; also overly broad exception handling may silently degrade validation; missing negative-path tests.
Reviewer 2: Core bugfix is correct, but new workflows are a supply-chain/secrets exposure risk and unrelated to issue #1; get_type_hints exception handling is too broad; add tests for failure-mode and exception chaining.
Reviewer 3: Happy-path test exists, but no coverage for get_type_hints failure branches; plus unrelated high-risk workflows added—unsafe to merge as-is.

Blocking issues:

Remove the newly added GitHub Actions workflows that clone and execute external/private repository code using secrets/tokens (supply-chain + secret-exposure risk) as they are out of scope for issue #1. If needed, move to a separate security-reviewed change with hardening.
In _validate_handler_signature, do not catch broad Exception from typing.get_type_hints and then proceed with unresolved/raw annotations when annotations are required. Narrow exception handling and/or fail fast with a clear ValueError that preserves the original exception as __cause__ to keep validation deterministic.
Add tests for negative/failure paths where typing.get_type_hints fails (e.g., unresolved forward reference under postponed annotations). Assert the raised ValueError references the specific parameter (message/ctx) and that exception chaining (__cause__) is preserved.

Suggestions:

Improve error messages by including the original (sanitized/bounded) annotation string/value to aid debugging.
Optionally provide/consider appropriate globalns/localns when calling get_type_hints (or document limitations), especially for annotations defined in class scopes.
Remove the no-op assignment ctx_annotation = ctx_annotation to avoid confusion and clarify whether resolved vs raw annotations are intended.

moonbox3 · 2026-02-16T09:17:24Z

+            include_extras=True,
+        )
+    except Exception as exc:
+        type_hint_error = exc


Blocking: Catching broad Exception around typing.get_type_hints and continuing can mask real resolution errors and lead to inconsistent validation. Narrow the exceptions and/or fail fast when annotations are required, raising a ValueError that preserves the original exception as __cause__.

moonbox3 · 2026-02-16T09:17:25Z

+    ctx_annotation = _get_annotation(ctx_param)
+    if skip_message_annotation and ctx_annotation == inspect.Parameter.empty:
        # When explicit types are provided via @handler(input=..., output=...),
        # the ctx parameter doesn't need a type annotation - types come from the decorator.


Non-blocking: ctx_annotation = ctx_annotation is a no-op and looks like an unfinished refactor. Remove it or rename variables to clearly distinguish resolved vs raw annotations.

moonbox3 · 2026-02-16T09:17:25Z

+def test_postponed_annotations_are_resolved() -> None:
+    executor = PostponedAnnotationExecutor(id="postponed")
+    assert set(executor.output_types) == {OutputTypeA}
+    assert set(executor.workflow_output_types) == {OutputTypeB}


Blocking (tests): Add failure-mode tests that force typing.get_type_hints() to fail under postponed annotations (e.g., undefined forward ref) for both message and ctx. Assert the ValueError mentions the specific parameter and that exc_info.value.__cause__ is set to the underlying exception (exception chaining preserved).

moonbox3 · 2026-02-16T09:17:39Z

Automated PR Comment Resolution Round 2

Verdict: changes_required
Two unique blocking themes in reviewer feedback: (1) exception handling around typing.get_type_hints must not broadly swallow errors / must fail fast deterministically with proper exception chaining, and (2) add failure-mode tests that force get_type_hints to fail (for both message and ctx). One non-blocking cleanup removes a redundant self-assignment. Automated workflow-generated comment is ignored.

Per-comment decisions:

Comment 2811362816: fix - Reviewer flags that catching broad Exception from typing.get_type_hints can mask genuine issues and potentially proceed with unresolved/incorrect annotations. Adjust implementation to narrow exception handling to expected resolution-related exceptions (e.g., NameError, TypeError, AttributeError) and/or ensure we deterministically raise ValueError when required annotations cannot be resolved, preserving cause.
Comment 2811362821: fix - Redundant no-op assignment should be removed (or variables clarified) for readability; straightforward code cleanup.
Comment 2811362827: fix - Tests currently insufficient: need explicit coverage of get_type_hints failure path and assertions on ValueError message + exception chaining to validate new behavior.
Comment 3907271979: ignore - Automated comment generated by this workflow; treat as non-actionable duplication of the underlying review feedback.
Comment 2811388934: fix - Duplicate of earlier blocking comment but still actionable: ensure we do not broadly catch Exception and continue; instead fail fast with a ValueError that preserves the original exception as cause when annotations are required.
Comment 2811388938: fix - Duplicate of earlier non-blocking cleanup request: remove the ctx_annotation self-assignment and/or clearly separate raw vs resolved annotations.
Comment 2811388941: fix - Blocking tests request: add failure-mode tests for both message and ctx under postponed annotations that reliably trigger get_type_hints failure (undefined forward ref), asserting parameter-specific error text and that cause is set.

Blocking issues selected for code changes:

Exception handling around typing.get_type_hints is too broad and risks masking real errors; must narrow to expected resolution failures and/or fail fast deterministically when required annotations cannot be resolved, preserving exception chaining via raise ... from exc.
Missing failure-mode test coverage for get_type_hints failure path(s), specifically for both message and ctx, asserting error message mentions the correct parameter and that __cause__ is populated.

Suggestions selected for implementation:

Consider refactoring annotation resolution into a small helper that returns resolved hints or raises a dedicated ValueError for unresolved required parameters; reduces duplicated logic and makes fail-fast intent explicit.
In tests, trigger get_type_hints failure using from __future__ import annotations and an undefined forward reference for both message type and WorkflowContext type arg to reliably exercise the except branch and validate exception chaining.

… 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>

moonbox3 added 5 commits February 2, 2026 13:23

AI triage bot initial commit for testing

39e6bff

Update triage yaml

1a63e76

Add docs bot yaml

58ac724

Merge branch 'main' into ai-triage-bot

ece7a14

moonbox3 commented Feb 16, 2026

View reviewed changes

moonbox3 changed the title ~~Resolve postponed WorkflowContext annotations in handler validation~~ Resolve postponed handler annotations during signature validation Feb 16, 2026

moonbox3 commented Feb 16, 2026

View reviewed changes

moonbox3 closed this Feb 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve postponed handler annotations during signature validation#7

Resolve postponed handler annotations during signature validation#7
moonbox3 wants to merge 5 commits intomainfrom
codex/fix/postponed-workflow-context-annotations

moonbox3 commented Feb 16, 2026 •

edited

Loading

Uh oh!

moonbox3 left a comment

Uh oh!

moonbox3 Feb 16, 2026

Uh oh!

moonbox3 Feb 16, 2026

Uh oh!

moonbox3 Feb 16, 2026

Uh oh!

moonbox3 commented Feb 16, 2026

Uh oh!

moonbox3 commented Feb 16, 2026

Uh oh!

moonbox3 left a comment

Uh oh!

moonbox3 Feb 16, 2026

Uh oh!

moonbox3 Feb 16, 2026

Uh oh!

moonbox3 Feb 16, 2026

Uh oh!

moonbox3 commented Feb 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

moonbox3 commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

moonbox3 left a comment

Choose a reason for hiding this comment

Automated PR Review Round 1: CHANGES_REQUESTED

Uh oh!

moonbox3 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

moonbox3 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

moonbox3 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

moonbox3 commented Feb 16, 2026

Automated PR Comment Resolution Round 1

Uh oh!

moonbox3 commented Feb 16, 2026

Automated Feedback Disposition Round 1

Uh oh!

moonbox3 left a comment

Choose a reason for hiding this comment

Automated PR Review Round 2: CHANGES_REQUESTED

Uh oh!

moonbox3 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

moonbox3 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

moonbox3 Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

moonbox3 commented Feb 16, 2026

Automated PR Comment Resolution Round 2

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

moonbox3 commented Feb 16, 2026 •

edited

Loading