fix: use session.NewEvent in generateRequestConfirmationEvent#591
fix: use session.NewEvent in generateRequestConfirmationEvent#591mohamedabozeid wants to merge 1 commit intogoogle:mainfrom
Conversation
The function was constructing an Event with a raw struct literal, bypassing session.NewEvent(). This resulted in missing ID (empty string) and uninitialized StateDelta (nil map). Use NewEvent to ensure all fields are properly initialized.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @mohamedabozeid, 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 critical issues in event generation by refactoring 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 correctly fixes a bug where session.Event was being created with a raw struct literal, leading to missing fields. The switch to session.NewEvent() is the right approach, and the updated tests properly verify the fix. I have one minor suggestion to make the code slightly more concise.
| event.LLMResponse = model.LLMResponse{ | ||
| Content: &genai.Content{ | ||
| Parts: parts, | ||
| Role: genai.RoleModel, | ||
| }, | ||
| Timestamp: time.Now(), | ||
| LongRunningToolIDs: longRunningToolIDs, | ||
| Actions: session.EventActions{}, | ||
| } |
There was a problem hiding this comment.
Since session.Event embeds model.LLMResponse, you can set the Content field directly on the event object. This avoids the explicit model.LLMResponse struct literal, making the code a little more direct.
For example:
event.Content = &genai.Content{
Parts: parts,
Role: genai.RoleModel,
}This is functionally equivalent to the current implementation because session.NewEvent already zero-initializes the embedded model.LLMResponse struct.
|
duplicate to #588 |
Fixes #590
Description
generateRequestConfirmationEventininternal/llminternal/functions.goconstructs anEventusing a raw struct literal, bypassingsession.NewEvent(). This results in:ID— empty string instead of a UUID. Any downstream logic relying on event IDs for deduplication, ordering, tracing, or correlation will break.StateDelta— nil map instead ofmap[string]any{}, which could cause nil map panics if anything writes to it.Root Cause
The function was written as a raw struct literal
&session.Event{...}instead of callingsession.NewEvent(). TheNewEventconstructor sets:ID: uuid.NewString()Timestamp: time.Now()Actions: EventActions{StateDelta: make(map[string]any)}The raw literal only set
Timestampand leftIDempty andStateDeltanil.Fix
Replace the raw struct literal with
session.NewEvent()and set the remaining fields (Author,Branch,LLMResponse,LongRunningToolIDs) on the returned object. This is consistent with how events are created elsewhere in the codebase.Testing Plan
Unit Tests
Updated tests in
internal/llminternal/functions_test.go:IDand a non-zeroTimestamp.StateDeltafromNewEvent.Manual E2E Test
Verified with
go test ./internal/llminternal/...— all tests pass with no regressions.