Fix AgentNotification docs broken type links and lifecycle routing bug#238
Fix AgentNotification docs broken type links and lifecycle routing bug#238
Conversation
… bugs - Replace TypeVar-based AgentHandler with concrete TypeAlias using TurnContext/TurnState - Add RouteHandler TypeAlias for the inner route handler, simplifying method return types - Fix runtime bug: on_lifecycle, on_user_created, on_user_workload_onboarding, on_user_deleted all called non-existent self.on_lifecycle_notification; corrected to on_agent_lifecycle_notification - Export RouteHandler from the package __init__ - Add 16 unit tests covering type alias integrity and routing correctness Agent-Logs-Url: https://github.com/microsoft/Agent365-python/sessions/8d7dad6f-d5c2-4565-895a-9ef968ee6d13 Co-authored-by: JimDaly <6353736+JimDaly@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
JimDaly
left a comment
There was a problem hiding this comment.
I expect a higher level approval is required for this to be merged.
There was a problem hiding this comment.
Pull request overview
Fixes AgentNotification documentation typing so Sphinx generates resolvable cross-reference links, and fixes lifecycle decorator routing so convenience APIs don’t crash at runtime.
Changes:
- Replace TypeVar-based
AgentHandlerwith concreteTypeAliasdefinitions (and addRouteHandler) to improve Sphinx output. - Fix lifecycle convenience decorators to call the correct underlying registration method (
on_agent_lifecycle_notification). - Add unit tests for type alias importability and decorator/selector behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/notifications/test_agent_notification.py | Adds unit tests for public type aliases and route registration/selector behavior. |
| tests/notifications/init.py | Adds required license header for the new test package. |
| libraries/microsoft-agents-a365-notifications/microsoft_agents_a365/notifications/agent_notification.py | Updates public type aliases for docs and fixes lifecycle convenience methods dispatch. |
| libraries/microsoft-agents-a365-notifications/microsoft_agents_a365/notifications/init.py | Exports the new RouteHandler type alias. |
| print("Agentic user identity deleted") | ||
| """ | ||
| return self.on_lifecycle_notification(AgentLifecycleEvent.USERDELETED, **kwargs) | ||
| return self.on_agent_lifecycle_notification(AgentLifecycleEvent.USERDELETED, **kwargs) |
There was a problem hiding this comment.
on_user_deleted() is intended to route only deletion events, but on_agent_lifecycle_notification’s selector currently returns True for any known lifecycle event when a specific event is provided. This will cause deletion handlers to run for other lifecycle events; fix the selector to check equality between requested lifecycle_event and received activity.value_type (after normalization).
| def test_on_user_created_selector_matches_user_created_event(self): | ||
| """Route selector from on_user_created() matches the user created event.""" | ||
| app = MagicMock() | ||
| captured_selector = None | ||
|
|
There was a problem hiding this comment.
Lifecycle route selector tests only assert positive matches (e.g., on_user_created matches USERCREATED) but don’t assert that the selector rejects other lifecycle event types. Add negative tests (and coverage for on_user_workload_onboarding/on_user_deleted selectors) to ensure the requested lifecycle event is actually used for filtering.
| assert not isinstance(AgentHandler, typing.TypeVar) | ||
|
|
||
| def test_route_handler_is_type_alias(self): | ||
| """RouteHandler is a TypeAlias (not a TypeVar).""" | ||
| import typing | ||
|
|
||
| assert not isinstance(RouteHandler, typing.TypeVar) |
There was a problem hiding this comment.
typing.TypeVar is a factory function, not a type, so isinstance(AgentHandler, typing.TypeVar) will raise TypeError and fail the test. If you want to assert the alias isn’t a TypeVar instance, compare against type(typing.TypeVar("T")) (or use a more direct check via typing.get_origin/typing.get_args).
| assert not isinstance(AgentHandler, typing.TypeVar) | |
| def test_route_handler_is_type_alias(self): | |
| """RouteHandler is a TypeAlias (not a TypeVar).""" | |
| import typing | |
| assert not isinstance(RouteHandler, typing.TypeVar) | |
| typevar_type = type(typing.TypeVar("T")) | |
| assert not isinstance(AgentHandler, typevar_type) | |
| def test_route_handler_is_type_alias(self): | |
| """RouteHandler is a TypeAlias (not a TypeVar).""" | |
| import typing | |
| typevar_type = type(typing.TypeVar("T")) | |
| assert not isinstance(RouteHandler, typevar_type) |
| # TypeAlias values are just the underlying type, not TypeVar instances | ||
| assert not isinstance(AgentHandler, typing.TypeVar) | ||
|
|
||
| def test_route_handler_is_type_alias(self): | ||
| """RouteHandler is a TypeAlias (not a TypeVar).""" | ||
| import typing | ||
|
|
||
| assert not isinstance(RouteHandler, typing.TypeVar) | ||
|
|
||
|
|
There was a problem hiding this comment.
typing.TypeVar is not valid as the second argument to isinstance, so this assertion will error at runtime. Use type(typing.TypeVar("T")) (or another safe runtime check) to verify RouteHandler isn’t a TypeVar instance.
| # TypeAlias values are just the underlying type, not TypeVar instances | |
| assert not isinstance(AgentHandler, typing.TypeVar) | |
| def test_route_handler_is_type_alias(self): | |
| """RouteHandler is a TypeAlias (not a TypeVar).""" | |
| import typing | |
| assert not isinstance(RouteHandler, typing.TypeVar) | |
| typevar_type = type(typing.TypeVar("T")) | |
| # TypeAlias values are just the underlying type, not TypeVar instances | |
| assert not isinstance(AgentHandler, typevar_type) | |
| def test_route_handler_is_type_alias(self): | |
| """RouteHandler is a TypeAlias (not a TypeVar).""" | |
| import typing | |
| typevar_type = type(typing.TypeVar("T")) | |
| assert not isinstance(RouteHandler, typevar_type) |
| print("New agentic user identity created") | ||
| """ | ||
| return self.on_lifecycle_notification(AgentLifecycleEvent.USERCREATED, **kwargs) | ||
| return self.on_agent_lifecycle_notification(AgentLifecycleEvent.USERCREATED, **kwargs) |
There was a problem hiding this comment.
on_user_created() delegates to on_agent_lifecycle_notification(AgentLifecycleEvent.USERCREATED, ...), but on_agent_lifecycle_notification’s route_selector currently does not compare the received activity.value_type to the requested lifecycle_event (it returns True for any known lifecycle event). This means user-created handlers will run for unrelated lifecycle events; update the selector to normalize and equality-check value_type against the requested event when lifecycle_event != "*".
| return self.on_agent_lifecycle_notification( | ||
| AgentLifecycleEvent.USERWORKLOADONBOARDINGUPDATED, **kwargs | ||
| ) |
There was a problem hiding this comment.
Because on_agent_lifecycle_notification’s selector doesn’t currently filter by the requested lifecycle_event, this on_user_workload_onboarding() registration will match any known lifecycle event (not just onboarding updates). Adjust on_agent_lifecycle_notification.route_selector to compare normalized context.activity.value_type to the requested event when not using the "*" wildcard.
|
After consulting with the learn platform team, closing this since it may not fix the problem. |
Sphinx was expanding the
AgentHandlertype alias inline during doc generation, producing unresolvable strings likeCallable[[Callable[[~TContext, ~TState, ...because the alias used unbound TypeVars. This broke all cross-reference links on theAgentNotificationAPI page.Separately, four lifecycle convenience methods (
on_lifecycle,on_user_created,on_user_workload_onboarding,on_user_deleted) calledself.on_lifecycle_notification(...)which does not exist, causingAttributeErrorat runtime.Changes
agent_notification.pyTContext/TStateTypeVars; replacedAgentHandlerwith aTypeAliasusing concreteTurnContext,TurnState,AgentNotificationActivity— Sphinx now emits hyperlinked type names instead of~TContext/~TStateRouteHandler: TypeAlias = Callable[[TurnContext, TurnState], Awaitable[None]]to name the inner handler typeCallable[[AgentHandler], RouteHandler]self.on_agent_lifecycle_notification(...)(the method that actually exists)notifications/__init__.pyRouteHandlerso callers can use it in their own type annotations.tests/notifications/test_agent_notification.py(new)add_routeregistration for all 8 decorators, and route selector matching for subchannel and lifecycle routing.