feat(agents): generator-as-protocol-adapter and tool coercion#2279
feat(agents): generator-as-protocol-adapter and tool coercion#2279
Conversation
Add serialize_tools, serialize_messages, deserialize_response to BaseGenerator so provider subclasses own all wire-format translation. LiteLLMGenerator now delegates through these methods instead of calling to_litellm/from_litellm directly. Tool.run() gains input coercion (TypeAdapter + _params_model) and output serialization (_return_adapter), returning str to eliminate double-quoting in workflow. Workflow _run_tools simplified accordingly.
Summary of ChangesHello @Hartorn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural improvement by formalizing the role of generators as protocol adapters, centralizing the logic for translating between internal data structures and external LLM provider formats. It also enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed architectural refactoring by establishing the BaseGenerator as a protocol adapter. This change, along with the introduction of input coercion and output serialization in Tool.run(), greatly improves the separation of concerns and simplifies the workflow logic. The new architectural rules documented in generator-adapter.mdc are a fantastic addition for long-term maintainability.
I've found one issue related to error handling during argument validation in tools, which I've detailed in a specific comment. Once that is addressed, this will be an excellent contribution to the codebase.
…ware pipeline Replace WithRetryPolicy and WithRateLimiter mixins with a composable CompletionMiddleware pipeline on BaseGenerator. This decouples cross-cutting concerns (retry, rate-limiting) from the generator inheritance hierarchy, making them stackable and independently testable.
…d docs Update requires-python to 3.12, fix stale docstrings and types in base generator, align tests with new middleware API (remove RetryPolicy, with_retries, with_rate_limiter), and document the middleware pipeline in the README.
…o.sleep mock Patch asyncio.sleep instead of overriding _before_sleep to verify exponential backoff and max_delay capping without production hooks.
Narrow _retry_mw helper to LiteLLMRetryMiddleware and assert middleware round-trips correctly in test_chat_workflow_serialization.
…ne' into refactor/generator-as-protocol-adapter Made-with: Cursor # Conflicts: # libs/giskard-agents/src/giskard/agents/generators/litellm_generator.py
Move serialization/deserialization pipeline into BaseGenerator._complete so subclasses only implement _call_model with raw wire types. Remove provider-coupled methods (Message.to_litellm, Message.from_litellm, Tool.to_litellm_function) from domain objects. Extract FinishReason type alias for reuse. Made-with: Cursor
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed architectural refactoring, turning the BaseGenerator into a protocol adapter. This decouples the core agent logic from provider-specific data formats. The introduction of serialize_tools, serialize_messages, and deserialize_response provides clear extension points for new providers. Additionally, the Tool.run method is enhanced with input coercion and output serialization, ensuring that tools always return a string, which simplifies the workflow logic. The changes are supported by a comprehensive set of new and updated tests. My review found one potential issue with error handling during tool argument validation. Overall, this is a high-quality contribution that greatly improves the extensibility and robustness of the agent framework.
…nce API Extract RetryPolicy config model and give BaseGenerator dedicated retry_policy and rate_limiter fields with guaranteed middleware ordering. Adds with_retries() and with_rate_limiter() convenience methods. Renames middleware -> middlewares for codebase consistency.
…rd-AI/giskard-oss into refactor/generator-as-protocol-adapter
6d1e7d3 to
d54d577
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable architectural refactoring by establishing the generator as a protocol adapter. The introduction of serialize_tools, serialize_messages, and deserialize_response methods in BaseGenerator effectively decouples the core agent logic from provider-specific data formats. The changes to Tool.run() to include input coercion and automatic output serialization are also excellent improvements, simplifying tool implementation and workflow logic. The updates are consistently applied across the codebase, including extensive and well-written tests for the new functionality. I have one suggestion to simplify the argument preparation logic in Tool.run() for better clarity and to remove redundancy. Overall, this is a great step forward for the agent's architecture.
…el stubs - Move Pydantic model_validate into the try/except so ValidationError from malformed LLM arguments is caught by the tool's catch handler. - Re-inject RunContext after coercion since validated dict drops non-model keys. - Resolve merge conflicts: union imports from both branches in base.py, litellm_generator.py, and test_generator_retry.py. - Add _call_model stub to MockGenerator in giskard-checks tests to satisfy the new abstract method on BaseGenerator. Made-with: Cursor
d54d577 to
8278eea
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and beneficial architectural refactoring, positioning the BaseGenerator as a protocol adapter. This change centralizes provider-specific translation logic, improving modularity and maintainability. The addition of input coercion and output serialization for tools is also a great enhancement, making tool execution more robust and predictable. The implementation is solid and well-tested for the most part. My feedback focuses on a few areas: ensuring type safety for FinishReason, a minor refactoring opportunity in tool argument handling, and, most importantly, updating several test mocks in the giskard-checks library to align with the new generator architecture, which is crucial for maintaining test effectiveness.
…r/generator-as-protocol-adapter Made-with: Cursor # Conflicts: # libs/giskard-agents/src/giskard/agents/generators/_types.py # libs/giskard-agents/src/giskard/agents/generators/base.py # libs/giskard-agents/src/giskard/agents/generators/litellm_generator.py # libs/giskard-agents/src/giskard/agents/tools/tool.py # libs/giskard-agents/tests/test_generator_retry.py # libs/giskard-agents/tests/test_tools.py
mattbit
left a comment
There was a problem hiding this comment.
Great improvement on tools and removing the to_litellm methods which don't belong to the tools and chats. This stuff should be handled internally by the Generator implementation.
The "protocol adapter" for the serialization is not necessary, this serialize/deserialize clean up is specific to litellm and internal to the final BaseGenerator implementation.
We shouldn't impose a structure on how these transformations should be implemented (e.g. by message/by tools), as it will be different for every provider.
We should move what's needed in the LiteLLMGenerator (as internal helpers) and leave the BaseGenerator untouched.
I agree that in some cases having standardized serialization for chat completions could help, but right now it's not needed. It's also not necessarily compatible with different APIs. Especially we see providers APIs diverge from the chat completion standard, so it could be counterproductive to impose such abstraction.
…enerationParams.merge - Move serialize_tools, serialize_messages, deserialize_response from BaseGenerator into LiteLLMGenerator as private helpers - Simplify _call_model signature to (messages, params) -> (Message, FinishReason) - Add GenerationParams.merge() for param resolution instead of _resolve_params - Update _complete to use GenerationParams.merge() as template method - Use PEP 695 type alias for FinishReason - Update generator-adapter cursor rule to reflect new architecture - Adapt all test mocks to new _call_model contract Made-with: Cursor
- Add @OverRide to all test mock methods overriding BaseGenerator._call_model - Annotate unannotated fixture/test params (mock_response, persona, context, kwargs) - Remove unused generator fixture param from test_workflow_error_handling - Assign unused call results to _ to satisfy reportUnusedCallResult - Rename _calls/_call_count to public attrs in test-only mock generators Made-with: Cursor
Known limitation
The
serialize_tools,serialize_messages, anddeserialize_responsemethods onBaseGeneratorare currently "advisory" — they exist as override points but are called from insideLiteLLMGenerator._complete_once, not orchestrated by the base class. A new provider subclass could skip them entirely with no enforcement.Next step: Restructure so
BaseGeneratorowns the serialize → call → deserialize pipeline, with the subclass implementing only a_send(wire_messages, wire_tools, params)method. This also ties into replacing the mixin-based retry/rate-limiting with a composable middleware pipeline. Tracked as a follow-up.Add serialize_tools, serialize_messages, deserialize_response to BaseGenerator so provider subclasses own all wire-format translation. LiteLLMGenerator now delegates through these methods instead of calling to_litellm/from_litellm directly.
Tool.run() gains input coercion (TypeAdapter + _params_model) and output serialization (_return_adapter), returning str to eliminate double-quoting in workflow. Workflow _run_tools simplified accordingly.
Description
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.pdm.lockrunningpdm update-lock(only applicable whenpyproject.tomlhas beenmodified)