-
Notifications
You must be signed in to change notification settings - Fork 265
fix(grpc): missing field in gRPC spec - state_transition_history #514
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?
fix(grpc): missing field in gRPC spec - state_transition_history #514
Conversation
Summary of ChangesHello @a2a-bot, 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 significantly enhances the A2A API's task management by introducing a new Highlights
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the A2A specification, which involves regenerating several key files including Protobuf definitions, gRPC stubs, and Pydantic type models. The main changes include the addition of a ListTasks method, updates to the Message and AgentCapabilities structures, and improvements to the generated type stubs. My review focuses on the newly added Pydantic models, where I've suggested adding validation constraints to align with the documentation, which would improve data integrity. I've also noted a positive change in the Protobuf stubs regarding the use of __slots__, which enhances type safety.
| """ | ||
| Filter tasks by context ID to get tasks from a specific conversation or session. | ||
| """ | ||
| history_length: int | None = None |
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 docstring for history_length states it must be non-negative. This can be enforced at the type level using Pydantic's Field for better data integrity. Since this is a generated file, this change should be reflected in the source JSON schema.
| history_length: int | None = None | |
| history_length: int | None = Field(default=None, ge=0) |
| """ | ||
| Request-specific metadata. | ||
| """ | ||
| page_size: int | None = None |
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 docstring for page_size mentions that the value must be between 1 and 100. It would be beneficial to enforce this constraint directly in the Pydantic model using Field. This would provide automatic validation and clearer error messages for developers using this type. Since this file is auto-generated, this change should ideally be made in the source JSON schema that datamodel-codegen uses.
| page_size: int | None = None | |
| page_size: int | None = Field(default=None, ge=1, le=100) |
|
|
||
| class SendMessageConfiguration(_message.Message): | ||
| __slots__ = ("accepted_output_modes", "push_notification", "history_length", "blocking") | ||
| __slots__ = () |
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.
Changing __slots__ to an empty tuple is a good improvement for these generated Protobuf message stubs. It correctly signals that these classes do not have a __dict__ and prevents accidental assignment of attributes that are not part of the Protobuf definition, which is a common source of runtime errors. This change makes the type hints more robust.
| task_id: str | ||
| role: Role | ||
| content: _containers.RepeatedCompositeFieldContainer[Part] | ||
| parts: _containers.RepeatedCompositeFieldContainer[Part] |
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.
Beware: This may be an annoying breaking change.
Over-the-wire this it's still the same proto field number, so the recipient won't be impacted.
But people updating the python a2a package will need to change their code from the_message.content[0] to the_message.parts[0] if they are constructing the gRPC message manually.
Also, the helpers will break. For instance: proto_utils.FromProto.message() is still referencing message.content which does not exist anymore. (at least I see it's not updated in this commit)
Commit: a2aproject/A2A@a2de798