Skip to content

fix: Merging of events in rearrangeEventsForAsyncFunctionResponsesInHistory #682

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.

Sign up for GitHub

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

Open
r4inee wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −5
Open

fix: Merging of events in rearrangeEventsForAsyncFunctionResponsesInHistory #682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions core/src/main/java/com/google/adk/flows/llmflows/Contents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,14 @@ private static List<Event> rearrangeEventsForAsyncFunctionResponsesInHistory(
resultEvents.add(mergeFunctionResponseEvents(responseEventsToAdd));
}
}

// Skip events between the function call and the last function response.
if (!sortedIndices.isEmpty()) {
int maxIndex = sortedIndices.get(sortedIndices.size() - 1);
if (maxIndex > i) {
i = maxIndex;
}
}
Comment on lines +438 to +444
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

high

This change correctly skips intermediate events. However, I've noticed a potential logic issue in the surrounding method. The functionCallIdToResponseEventIndex map only stores the index of the last function response for a given call ID. This prevents merging of multiple responses for a single function call, as only the last response is ever processed. mergeFunctionResponseEvents is not called in this case.

For example, in the rearrangeHistory_multipleFRsForSameFC_returnsMergedFR test, fr1, fr2, and fr3 should be merged, but with the current logic, only fr3 is picked up and added to the results.

To properly support merging, you might consider changing functionCallIdToResponseEventIndex to a Map<String, List<Integer>> to track all responses for a function call. This seems like a significant issue, so I've marked it as high severity.

}
} else {
// gemini-3 specific part: buffer response events
Expand Down
64 changes: 59 additions & 5 deletions core/src/test/java/com/google/adk/flows/llmflows/ContentsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,27 +203,81 @@ public void rearrangeHistory_asyncFR_returnsRearrangedList() {
public void rearrangeHistory_multipleFRsForSameFC_returnsMergedFR() {
Event fcEvent = createFunctionCallEvent("fc1", "tool1", "call1");
Event frEvent1 =
createFunctionResponseEvent("fr1", "tool1", "call1", ImmutableMap.of("status", "running"));
createFunctionResponseEvent("fr1", "tool1", "call1", ImmutableMap.of("status", "pending"));
Event frEvent2 =
createFunctionResponseEvent("fr2", "tool1", "call1", ImmutableMap.of("status", "done"));
createFunctionResponseEvent("fr2", "tool1", "call1", ImmutableMap.of("status", "running"));
Event frEvent3 =
createFunctionResponseEvent("fr3", "tool1", "call1", ImmutableMap.of("status", "done"));
ImmutableList<Event> inputEvents =
ImmutableList.of(
createUserEvent("u1", "Query"),
fcEvent,
createUserEvent("u2", "Wait"),
frEvent1,
createUserEvent("u3", "Done?"),
frEvent2);
frEvent2,
frEvent3,
createUserEvent("u4", "Follow up query"));

List<Content> result = runContentsProcessor(inputEvents);

assertThat(result).hasSize(3); // u1, fc1, merged_fr
assertThat(result).hasSize(4); // u1, fc1, merged_fr, u4
assertThat(result.get(0)).isEqualTo(inputEvents.get(0).content().get());
assertThat(result.get(1)).isEqualTo(inputEvents.get(1).content().get()); // Check merged event
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

medium

The comment // Check merged event is misleading. This line asserts the fcEvent is present in the result. The merged event is mergedContent, which is checked on the following lines.

Suggested change
assertThat(result.get(1)).isEqualTo(inputEvents.get(1).content().get()); // Check merged event
assertThat(result.get(1)).isEqualTo(inputEvents.get(1).content().get()); // Check fcEvent

Content mergedContent = result.get(2);
assertThat(mergedContent.parts().get()).hasSize(1);
assertThat(mergedContent.parts().get().get(0).functionResponse().get().response().get())
.containsExactly("status", "done"); // Last FR wins
.containsExactly("status", "done"); // Last FR wins (frEvent3)
assertThat(result.get(3)).isEqualTo(inputEvents.get(7).content().get()); // u4
}

@Test
public void rearrangeHistory_multipleFRsForMultipleFC_returnsMergedFR() {
Event fcEvent1 = createFunctionCallEvent("fc1", "tool1", "call1");
Event fcEvent2 = createFunctionCallEvent("fc2", "tool1", "call2");

Event frEvent1 =
createFunctionResponseEvent("fr1", "tool1", "call1", ImmutableMap.of("status", "pending"));
Event frEvent2 =
createFunctionResponseEvent("fr2", "tool1", "call1", ImmutableMap.of("status", "done"));

Event frEvent3 =
createFunctionResponseEvent("fr3", "tool1", "call2", ImmutableMap.of("status", "pending"));
Event frEvent4 =
createFunctionResponseEvent("fr4", "tool1", "call2", ImmutableMap.of("status", "done"));

ImmutableList<Event> inputEvents =
ImmutableList.of(
createUserEvent("u1", "I"),
fcEvent1,
createUserEvent("u2", "am"),
frEvent1,
createUserEvent("u3", "waiting"),
frEvent2,
createUserEvent("u4", "for"),
fcEvent2,
createUserEvent("u5", "you"),
frEvent3,
createUserEvent("u6", "to"),
frEvent4,
createUserEvent("u7", "Follow up query"));

List<Content> result = runContentsProcessor(inputEvents);

assertThat(result).hasSize(7); // u1, fc1, merged_fr, u4, fc2, merged_fr2, u7
assertThat(result.get(0)).isEqualTo(inputEvents.get(0).content().get()); // fc1
assertThat(result.get(1)).isEqualTo(inputEvents.get(1).content().get()); // merged_fr
Comment on lines +268 to +269
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

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

medium

These comments are incorrect. result.get(0) is u1 and result.get(1) is fc1. The merged function response for fc1 is at result.get(2).

Suggested change
assertThat(result.get(0)).isEqualTo(inputEvents.get(0).content().get()); // fc1
assertThat(result.get(1)).isEqualTo(inputEvents.get(1).content().get()); // merged_fr
assertThat(result.get(0)).isEqualTo(inputEvents.get(0).content().get()); // u1
assertThat(result.get(1)).isEqualTo(inputEvents.get(1).content().get()); // fc1

Content mergedContent = result.get(2);
assertThat(mergedContent.parts().get()).hasSize(1);
assertThat(mergedContent.parts().get().get(0).functionResponse().get().response().get())
.containsExactly("status", "done"); // Last FR wins (frEvent2)
assertThat(result.get(3)).isEqualTo(inputEvents.get(6).content().get()); // u4
assertThat(result.get(4)).isEqualTo(inputEvents.get(7).content().get()); // fc2
Content mergedContent2 = result.get(5);
assertThat(mergedContent2.parts().get()).hasSize(1);
assertThat(mergedContent2.parts().get().get(0).functionResponse().get().response().get())
.containsExactly("status", "done"); // Last FR wins (merged_fr2)
assertThat(result.get(6)).isEqualTo(inputEvents.get(12).content().get()); // u7
}

@Test
Expand Down