Skip to content

bug-runssehandler-writes-errors-as-plain-text-instead-of-sse-json-events#532

Open
indurireddy-TF wants to merge 3 commits intomainfrom
511-bug-runssehandler-writes-errors-as-plain-text-instead-of-sse-json-events
Open

bug-runssehandler-writes-errors-as-plain-text-instead-of-sse-json-events#532
indurireddy-TF wants to merge 3 commits intomainfrom
511-bug-runssehandler-writes-errors-as-plain-text-instead-of-sse-json-events

Conversation

@indurireddy-TF
Copy link
Contributor

This PR fixes Issue #511, where agent execution errors were being written to the SSE stream as plain text. This caused parsing failures in the Web UI, which expects every event in the stream to be a valid JSON payload prefixed with the SSE data: identifier.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @indurireddy-TF, 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 resolves a critical bug where agent execution errors were transmitted as plain text over Server-Sent Events (SSE), causing parsing failures in the Web UI. By encapsulating these errors within a structured models.Event and utilizing a new dedicated helper function for serialization, the change guarantees that all events, including errors, adhere to the expected SSE JSON format, thereby enhancing the reliability and usability of the event stream for front-end consumers.

Highlights

  • Structured Error Reporting: Agent execution errors are now formatted as structured models.Event objects instead of plain text, ensuring proper parsing by the Web UI.
  • New Helper Function for Event Flashing: A new private helper function, flashModelEvent, has been introduced to standardize the serialization and flushing of models.Event objects as Server-Sent Events (SSE).
  • UUID Integration: The github.com/google/uuid package is now used to generate unique identifiers for each error event, enhancing traceability.

🧠 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
  • server/adkrest/controllers/runtime.go
    • Added import for github.com/google/uuid to generate unique identifiers for events.
    • Modified the RunSSEHandler function to create a models.Event object when an agent execution error occurs, populating it with an ID, Time, Author, ErrorCode, and ErrorMessage.
    • Replaced the direct fmt.Fprintf call for error messages with a call to the newly introduced flashModelEvent function, ensuring errors are sent as structured JSON.
    • Extracted the logic for flushing models.Event objects as SSE data into a new private helper function flashModelEvent.
    • Updated the existing flashEvent function to convert session.Event to models.Event and then delegate to flashModelEvent for consistent serialization.
Activity
  • No specific activity (comments, reviews, etc.) was provided in the context.
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.

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

  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
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 addresses a bug where agent execution errors were sent as plain text in SSE streams, causing parsing failures. The fix involves creating a structured JSON error event and sending it over SSE, which is the correct approach. The refactoring to introduce a flashModelEvent function is clean and improves code reuse. I have one suggestion regarding the error event payload to ensure complete information is sent to the client.

Comment on lines +124 to 130
errorEvent := models.Event{
ID: uuid.NewString(),
Time: time.Now().Unix(),
Author: "system",
ErrorCode: "EXECUTION_ERROR",
ErrorMessage: err.Error(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The generated error event is missing the InvocationID. While regular events have this field populated, this error event will have an empty InvocationID. This could lead to incorrect grouping or display of events on the client side if it relies on this field for correlation. It would be beneficial to include the InvocationID if it's available. A possible approach is to capture the InvocationID from the first successful event and use it for subsequent error events. This might require declaring a variable to hold the ID before the for loop.

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.

[bug] RunSSEHandler writes errors as plain text instead of SSE JSON events

1 participant