-
Notifications
You must be signed in to change notification settings - Fork 315
feat(byo): add lifespan hook for BYO agents #1064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||||
| import asyncio | ||||||||
| import importlib | ||||||||
| import sys | ||||||||
|
||||||||
| import sys |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dynamic import uses the agent name directly without adjusting sys.path to include working_dir. If the agent module is in a subdirectory (e.g., working_dir/name), this import may fail. Consider using sys.path.insert(0, working_dir) before the import to ensure the module can be found, similar to how AgentLoader handles the working directory.
| try: | |
| try: | |
| sys.path.insert(0, working_dir) |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message 'Failed to load agent module for lifespan' is too generic and doesn't indicate which module failed to load. Consider including the module name in the error message: f\"Failed to load agent module '{name}' for lifespan\" to aid in debugging.
| logger.exception("Failed to load agent module for lifespan") | |
| logger.exception(f"Failed to load agent module '{name}' for lifespan") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,4 @@ | ||
| __all__ = ["agent", "lifespan"] | ||
|
|
||
| from . import agent | ||
| from .lifespan import lifespan |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import logging | ||
| from contextlib import asynccontextmanager | ||
| from typing import Any | ||
|
|
||
|
|
||
| @asynccontextmanager | ||
| async def lifespan(app: Any): | ||
| logging.info("Lifespan: setup") | ||
| try: | ||
| yield | ||
| finally: | ||
| logging.info("Lifespan: teardown") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
build_local()method doesn't compose the user-defined lifespan with any base lifespan likebuild()does withtoken_service.lifespan(). While this may be intentional for local mode, it creates inconsistency. If users define a lifespan expecting composition behavior (e.g., wrapping around a base lifespan), it won't work in local mode. Consider either using_compose_lifespan()with a no-op base lifespan or documenting this behavioral difference.