Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive middleware system for the Genkit Python package, allowing developers to intercept and modify generation, model calls, and tool executions. Key changes include new BaseMiddleware and GenerateMiddleware abstractions, a MiddlewareRef type for referencing middleware, and updated registration mechanisms via Genkit and a new middleware_plugin helper. The augment_with_context middleware has been refactored to align with the new system, and the reflection API is updated to expose middleware definitions. The feedback suggests improving the handling of the genkit argument in middleware resolution, enhancing middleware name validation, and ensuring robustness against concurrent plugin modifications during initialization.
py/packages/genkit/src/genkit/_core/_middleware/_augment_with_context.py
Outdated
Show resolved
Hide resolved
py/packages/genkit/tests/genkit/core/endpoints/reflection_test.py
Outdated
Show resolved
Hide resolved
| from ._utils import _CONTEXT_PREFACE, _context_item_template, _last_user_message | ||
|
|
||
|
|
||
| def augment_with_context( |
There was a problem hiding this comment.
do we stil use ths helper?
|
|
||
| """Abstract middleware runtime hook signatures. | ||
|
|
||
| ``MiddlewareRuntime`` lives here with types from ``genkit._core._model`` and |
There was a problem hiding this comment.
remove this comment
| async def initialize_all_plugins(self) -> None: | ||
| """Initialize every registered plugin once (lazy init is idempotent). | ||
|
|
||
| Used by the reflection server so values such as default models are |
There was a problem hiding this comment.
Shorten this comment.
|
|
||
|
|
||
| def builtin_generate_middleware_definitions() -> list[GenerateMiddleware]: | ||
| """Stock middleware definitions; empty until built-in middleware PR lands.""" |
There was a problem hiding this comment.
augment_with_context should show up here
| _FORBIDDEN_IN_MIDDLEWARE_KEY_SEGMENT = re.compile(r'[\x00-\x1f/\\:]|\s') | ||
|
|
||
|
|
||
| class _MiddlewareRegistryView(Protocol): |
There was a problem hiding this comment.
call it RegistryLike maybe?
1 participant
BaseMiddleware/GenerateMiddleware/generate_middleware,middleware_plugin, andPlugin.generate_middleware()so apps register middleware definitions on the app.generate(..., use=[...])accepts onlyMiddlewareRef(or dicts coerced toMiddlewareRef); resolves names via the registry.wrap_toolchain preserved withasyncio.gather).Genkit.generate_middleware()builds definitions without registering; registration is via plugins.GenerateActionOptions.usetyped aslist[MiddlewareRef] | Nonewith dict coercion in validation._ai/_middleware.py.register_builtin_generate_middlewareis a no-op for names until then.