Automated Code Review

Reviewers: 4 | Confidence: 92%

✓ Correctness

This PR adds validation to reject unresolved TypeVar annotations in handler and function executor message parameters, raising clear ValueError messages that guide users toward using explicit type parameters. The implementation is correct: the TypeVar check is properly gated behind skip_message_annotation so that explicit input=/output= parameters bypass it, the check runs after get_type_hints() resolution (which preserves unresolved TypeVars), and the import styles are consistent with each file's existing conventions (TypeVar directly imported in _executor.py, typing.TypeVar in _function_executor.py). Tests comprehensively cover rejection, bypass via explicit types, concrete type acceptance, and error message content.

✓ Security Reliability

This PR adds early validation to reject unresolved TypeVar annotations in handler/function executor message parameters, raising clear ValueError messages that guide users toward explicit type declarations. The changes are well-scoped, correctly gated behind the existing skip_message_annotation flag so that explicit input= overides bypass the check, and both implementation files use their respective TypeVar import style consistently. No security or reliability issues found.

✓ Test Coverage

The test coverage for the new TypeVar rejection logic is solid. Both _executor.py and _function_executor.py changes are tested for the rejection path (unresolved TypeVar raises ValueError), the bypass path (explicit input= skips the check), and the error message content. A concrete-type happy-path regression test exists for the handler but is missing for FunctionExecutor—though existing tests elsewhere in the file already cover that path. One minor observation: test_handler_rejects_unresolved_typevar_with_workflow_context_generic exercises the same code path as test_handler_rejects_unresolved_typevar_in_message_annotation since the TypeVar check is on the message parameter, not the context annotation, making it redundant rather than an additional coverage gain. Overall, the new behavior is well-tested with meaningful assertions.

✓ Design Approach

The approach is correct and well-targeted: failing fast at decorator-application time (class definition) with a clear error message and a pointer to the explicit-types escape hatch is the right design. There are no leaky abstractions or symptom-level masking — registering a TypeVar as a dispatch type would silently produce incorrect runtime behavior, so rejecting it early is the proper fix. One minor inconsistency: _function_executor.py calls typing.get_type_hints(func) without the try/except guard that exists in _executor.py; this pre-exists the PR but the new TypeVar check is inserted immediately after that call, making it slightly more exposed to unexpected errors. The TypeVar check in _executor.py is placed after validate_workflow_context_annotation, so a function with both a TypeVar message annotation and a bad ctx annotation will surface the ctx error first, which is less actionable — the TypeVar check should come earlier. Neither issue is blocking.

Suggestions

• In _executor.py, move the TypeVar check to before validate_workflow_context_annotation so users whose handler has a TypeVar message annotation see the more actionable 'unresolved TypeVar' error rather than a confusing ctx-annotation error.
• In _function_executor.py, wrap the typing.get_type_hints(func) call (line 328) in a try/except with a raw-annotation fallback, matching the defensive pattern already used in _executor.py. This is pre-existing but the new TypeVar check sits immediately after it.
• test_handler_rejects_unresolved_typevar_with_workflow_context_generic exercises the identical code path as test_handler_rejects_unresolved_typevar_in_message_annotation (the check is on message_type, not ctx_annotation). Consider removing the duplicate or repurposing it to test a genuinely different scenario (e.g., a bounded or constrained TypeVar).
• Consider adding a test with a bounded TypeVar (e.g., TypeVar('T', bound=SomeBase)) to explicitly document that bounded TypeVars are also rejected, guarding against future refactors that might change the detection logic.
• Consider adding a test_function_executor_allows_concrete_types test (analogous to test_handler_allows_concrete_types) to keep the two test suites symmetric.

Automated review by moonbox3's agents

Pull request overview

This PR improves the Python workflow executor registration experience by failing fast when a class-based @handler method or a function-based FunctionExecutor uses an unresolved TypeVar as the message annotation, replacing a later confusing workflow edge TypeCompatibilityError with an actionable registration-time ValueError.

Changes:

• Add explicit rejection of unresolved TypeVar message annotations during handler/function signature validation.
• Provide clearer error messages that recommend using explicit decorator types (@handler(input=..., output=...) / @executor(input=..., output=...)).
• Add regression tests covering the new failure mode and the explicit-type bypass behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Automated Code Review

Reviewers: 4 | Confidence: 89%

✓ Correctness

This PR reorders validation steps in _executor.py (message TypeVar check before ctx validation) and adds a try/except around typing.get_type_hints() in _function_executor.py to mirror the existing resilience pattern in _executor.py. Tests are updated to cover bounded TypeVars and concrete types. The changes are logically correct: the reorder only affects which error surfaces first when both message and ctx annotations are invalid, the fallback for get_type_hints is consistent with the sibling module, and the new bounded-TypeVar test improves coverage since TypeVar('_BT', bound=str) is still an instance of TypeVar.

✓ Security Reliability

This PR makes two changes: (1) in _executor.py, it reorders validation so the message-type TypeVar check runs before ctx-parameter validation, giving clearer error messages when both are problematic; (2) in _function_executor.py, it wraps typing.get_type_hints() in a try/except to handle from __future__ import annotations gracefully, matching the existing pattern in _executor.py. Both changes are low-risk. The broad except Exception catch is intentional and already established in the codebase—typing.get_type_hints() can raise various exceptions (NameError, AttributeError, RecursionError) when forward references can't be resolved, and the fallback to raw annotations is a reasonable degradation. Tests are appropriate. No security or reliability issues found.

✗ Test Coverage

The PR makes two functional changes: (1) reorders validation in _executor.py so the TypeVar rejection on the message parameter fires before the WorkflowContext annotation check on ctx, and (2) adds a try/except fallback around typing.get_type_hints() in _function_executor.py to mirror existing logic in _executor.py. Tests were added for a bounded TypeVar case and a concrete-type happy path in FunctionExecutor. However, there are notable test coverage gaps: the new get_type_hints fallback path in _function_executor.py has no test exercising the except branch (unlike _executor.py which has test_handler_unresolvable_annotation_raises in test_executor_future.py), and the bounded TypeVar rejection test was only added for Executor but not for FunctionExecutor.

✓ Design Approach

The PR makes two related changes: (1) adds the same try/except guard around typing.get_type_hints in _function_executor.py that already existed in _executor.py, making both files consistent when from __future__ import annotations causes forward-reference resolution failures; and (2) reorders the validation steps in _executor.py so the message-TypeVar check fires before the ctx annotation check, giving a clearer error for the common case. The test rename replaces a test that was effectively a duplicate of the adjacent unbounded-TypeVar test with a new bounded-TypeVar case, which improves coverage. No fundamental design flaws are present. One concern: the except Exception catch is intentionally broad (per comment), but when get_type_hints fails the fallback returns raw p.annotation values which, under from __future__ import annotations, are strings—not actual types. This means the TypeVar guard (isinstance(message_type, TypeVar)) will silently pass for a string annotation, and downstream ctx validation may produce a confusing error rather than the root-cause one. Narrowing the except to (NameError, AttributeError, RecursionError) would preserve the resilience goal while letting unexpected failures surface naturally. This pattern was already established in _executor.py (not introduced here), so it's not a new regression, but extending it to _function_executor.py without addressing the concern propagates the issue.

Flagged Issues

• The new try/except fallback around typing.get_type_hints() in _function_executor.py (lines 331–334) has no test exercising the except branch. test_executor_future.py has test_handler_unresolvable_annotation_raises for the equivalent code in _executor.py, but test_function_executor_future.py has no analogous test. This is the primary behavioral change in the file and needs coverage.

Suggestions

• Add a bounded TypeVar rejection test for FunctionExecutor (analogous to the new test_handler_rejects_bounded_typevar_in_message_annotation for Executor) to ensure parity between the two validation paths.
• Consider adding a test that validates the improved error ordering from the reorder in _executor.py—e.g., a handler with both a TypeVar message and an unannotated ctx should raise the TypeVar error, not a WorkflowContext error.
• Narrow except Exception to except (NameError, AttributeError, RecursionError) in both _executor.py and _function_executor.py. When get_type_hints fails for other reasons, the fallback yields raw string annotations (from from __future__ import annotations) that silently bypass the isinstance(message_type, TypeVar) guard and may produce confusing downstream errors rather than surfacing the actual resolution failure.

Automated review by moonbox3's agents

Automated Code Review

Reviewers: 4 | Confidence: 93%

✓ Correctness

The new test correctly verifies that the TypeVar validation error (line 740 of _executor.py) fires before the WorkflowContext annotation validation (line 756). The test constructs a handler with both an unresolved TypeVar message annotation (_T) and an unannotated ctx parameter, and asserts that only the TypeVar error is raised. The _T TypeVar is already defined at line 927 of the test file. The test is well-structured and the assertion matches the actual error message text.

✓ Security Reliability

This is a clean, well-structured test addition that verifies error priority ordering when a handler has both an unresolved TypeVar message annotation and an unannotated context parameter. The test is read-only (no side effects), uses existing test infrastructure correctly, and the assertion aligns with the actual code path in _executor.py where the TypeVar check (line 740) precedes WorkflowContext validation (line 756). No security or reliability concerns are present — this is purely a test file change with no trust boundary implications.

✓ Test Coverage

The new test test_handler_typevar_error_takes_priority_over_context_error is a well-structured regression/behavior test that validates error ordering when both a TypeVar message annotation and an unannotated ctx parameter are present. The test correctly asserts that the ValueError with 'unresolved TypeVar' is raised before the WorkflowContext validation would fire, matching the code flow in _executor.py (TypeVar check at line 740 precedes validate_workflow_context_annotation at line 756). The test is meaningful — it checks a specific match string, not just 'no exception'. One minor gap: there is no analogous test for FunctionExecutor, which has the same TypeVar-before-context ordering in _function_executor.py. This is a suggestion, not a blocking issue, since the PR scope is focused on Executor.

✓ Design Approach

The new test verifies that when a handler has both an unresolved TypeVar message annotation and an unannotated ctx parameter, the TypeVar error is raised first. The implementation already enforces this order (TypeVar check on line 740 precedes WorkflowContext validation on line 756), so the test is a valid regression guard. No fundamental design issues found.

Suggestions

• Consider adding a parallel test for FunctionExecutor in test_function_executor.py to verify the same error-priority behavior, since _function_executor.py has the same TypeVar-before-context validation ordering.
• Consider adding a brief comment noting that the pined validation ordering is intentional (TypeVar is more actionable to fix first), so future refactors don't inadvertently swap the order.

Automated review by moonbox3's agents