fix(core): throw explicit error on dropped tool responses#26668
Conversation
Summary of ChangesHello, 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 improves the reliability and debuggability of the LocalAgentExecutor by enforcing strict validation on tool response processing. By throwing explicit errors instead of silently handling missing data, the system now immediately surfaces internal concurrency or scheduler issues that violate the Gemini API tool response requirements. 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 the 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 counterproductive. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances error handling in the LocalAgentExecutor by ensuring that every tool call from the model has a corresponding response, as required by the Gemini API protocol. It introduces a critical system failure error when tool execution results are unexpectedly dropped or lost by the scheduler, while accounting for aborted executions and successful task completions. Additionally, unit tests were added to verify these failure scenarios. I have no feedback to provide.
|
Size Change: +209 B (0%) Total Size: 34 MB
ℹ️ View Unchanged
|
gemini-cli
Bot
added
the
area/agent
| } | ||
|
|
||
| // Reconstruct toolResponseParts in the original order | ||
| // Reconstruct toolResponseParts in the original order. |
There was a problem hiding this comment.
nit: excessive commenting here and below
| } else { | ||
| // If the execution was aborted (or timed out) mid-turn, the scheduler | ||
| // was interrupted, so empty responses are expected. | ||
| if (signal.aborted) { |
There was a problem hiding this comment.
this seems stylistically hard to read. I'd recommend avoiding nested if/else's and prefer early returns(good to have in your gemini.md).
Something like:
// The Gemini API requires exactly one response per function call.
const toolResponseParts: Part[] = [];
for (const [index, functionCall] of functionCalls.entries()) {
const callId = functionCall.id ?? `${promptId}-${index}`;
const part = syncResults.get(callId);
if (part) {
toolResponseParts.push(part);
continue;
}
const isAborted = signal.aborted;
const isTaskComplete = functionCall.name === COMPLETE_TASK_TOOL_NAME && taskCompleted;
// Safely skip missing responses if the run stopped or the turn won't be sent back.
if (isAborted || isTaskComplete) {
continue;
}
throw new Error(
`[LocalAgentExecutor] Critical System Failure: Tool execution result was lost/dropped by the scheduler for callId ${callId} (${functionCall.name}). This indicates an internal race condition or scheduler bug.`,
);
}
return {
nextMessage: { role: 'user', parts: toolResponseParts },
submittedOutput,
taskCompleted,
aborted,
};
aishaneeshah
force-pushed
the
fix/issue-25359-protocol-violation
branch
2 times, most recently
from
a929023 to
e4e2320
Compare
aishaneeshah
force-pushed
the
fix/issue-25359-protocol-violation
branch
from
e4e2320 to
c772390
Compare
2 participants
Summary
Refactors
LocalAgentExecutorto throw a fail-fast systemErrorwhen a tool response is dropped by the scheduler. This resolves two critical Gemini API protocol violations onmainthat caused immediate400 Bad Requestcrashes:textpart ({ text: "All tool calls failed..." }) instead of the requiredfunctionResponseparts when all tool calls failed.Allows agent / developer to see the actual error, and gives it a chance to recover.
Details
functionResponseparts matching the exact count of requested tool calls.mainviolated this by returning atextfallback on total failure, and silently ignoring dropped parallel responses. Both triggered backend400 Bad Requestrejections.toolResponsePartsstrictly. Throws an explicit systemErrorif any requested response is missing/dropped.textfallback block entirely.complete_task: No response turn is sent when the task completes successfully.signal.aborted === true) are bypassed.local-executor.test.tsto assert critical throwing behavior.Related Issues
Fixes #25359
How to Validate
npm test -w @google/gemini-cli-core -- src/agents/local-executor.test.tsPre-Merge Checklist