Skip to content

chore: Evaluate OpenCode performance#377

Draft
bartek-gralewicz wants to merge 2 commits intoepic/1.0_breaking_changesfrom
bgralewicz/test_opencode_performance
Draft

chore: Evaluate OpenCode performance#377
bartek-gralewicz wants to merge 2 commits intoepic/1.0_breaking_changesfrom
bgralewicz/test_opencode_performance

Conversation

@bartek-gralewicz
Copy link
Copy Markdown
Collaborator

Description

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Follow the CONTRIBUTING Guide.
  • Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
    • Important Prefixes for release-please:
      • fix: which represents bug fixes, and correlates to a SemVer patch.
      • feat: represents a new feature, and correlates to a SemVer minor.
      • feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
  • Ensure the tests and linter pass
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 focuses on significant internal refactoring and API cleanup to enhance the maintainability and clarity of the A2A client and server. The changes aim to improve the consistency of naming conventions and simplify data structures, particularly around task management, streaming, and push notification configurations. These updates lay a foundation for future performance evaluations by ensuring a more coherent and standardized codebase.

Highlights

  • API Renaming and Type Refactoring: Standardized naming conventions across the A2A client and server, including renaming resubscribeTask to subscribeToTask, getAgentCard to getExtendedAgentCard, and blocking to returnImmediately for clarity and consistency. The core type definitions were moved from a2a_types.ts to a2a.ts.
  • Push Notification Configuration Simplification: Streamlined the handling of task push notification configurations by directly using TaskPushNotificationConfig objects instead of nested request types, and updated related methods and parameters across client, server, and transport layers.
  • Parameter and URL Structure Updates: Adjusted method parameters and REST/gRPC URL constructions to align with the new naming conventions and simplified data structures, particularly for task-related operations and message sending.
  • Artifact ID Consistency: Renamed the artifactId field to id within Artifact objects for better consistency with general ID naming practices.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 introduces significant refactoring and renaming across the client, server, and transport layers to standardize method names, parameter structures, and type definitions. Key changes include renaming resubscribeTask to subscribeToTask, blocking to returnImmediately, and consolidating push notification configurations under TaskPushNotificationConfig. The API now uses direct id and taskId parameters instead of parsing them from resource names, and REST endpoints have been updated accordingly. The a2a_types.ts file has been removed, indicating a type definition consolidation. A critical typo in test/server/mocks/agent-executor.mock.ts needs to be addressed, and a type assertion in src/client/multitransport-client.ts for TaskPushNotificationConfig could lead to runtime errors if required fields are missing, suggesting a fragile implementation that needs further attention.

append: false,
lastChunk: false,
artifact: undefined,
91: undefined,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This line appears to be a typo or a merge conflict artifact and will cause a syntax error. It should be removed.

Comment on lines +328 to +335
result.configuration.taskPushNotificationConfig = {
...this.config.pushNotificationConfig,
taskId: params.message.taskId,
id: this.config.pushNotificationConfig.id || '', // Must have ID? Or server generates it?
// If config is partial, we assume missing fields are handled by server or not needed?
// But TaskPushNotificationConfig expects fields.
// This is tricky if TaskPushNotificationConfig is strict.
} as TaskPushNotificationConfig;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The type assertion as TaskPushNotificationConfig hides a potential issue where this.config.pushNotificationConfig (which is a Partial<TaskPushNotificationConfig>) may not have all the required fields of TaskPushNotificationConfig (like url, token, and tenant). This could lead to runtime errors if the receiving end expects a complete object.

To make this safer, you should validate that the necessary properties exist on this.config.pushNotificationConfig before creating the full object, or ensure the server can handle a partial configuration. The comments in the code suggest awareness of this, but it remains a fragile implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant