Repository navigation
Conversation
aggregate_evals.js logged "No reports found." and exited 0 when the artifacts directory contained no report.json files. Since the nightly workflow's Run Evals step is continue-on-error, a nightly whose matrix legs all failed (e.g. expired API key) finished green. On the scheduled nightly, zero reports now emit an ::error:: annotation and exit non-zero so the run is visibly red. Filtered workflow_dispatch runs can legitimately match nothing, so they keep exit 0 and emit a ::warning:: annotation instead; local runs stay quiet.
import.meta.dirname needs Node.js >= 20.11.0 while the repository supports >= 20.0.0, so derive the directory from import.meta.url via fileURLToPath instead, matching generate-settings-schema.test.ts.
The GoogleSearch tool passed only the caller's abort signal to generateContent, so a search whose underlying request never settled left the agent loop in a permanent "Thinking..." state until the user cancelled manually. The invocation now starts a 30 second timer and aborts the request when it fires, returning a structured WEB_SEARCH_FAILED tool error so the model can recover. External aborts are forwarded onto the same controller, so user cancellation still reports "Web search was cancelled." Timer and listener are cleaned up in a finally block.
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 system reliability by introducing a timeout mechanism for web searches and enhancing error reporting in the evaluation aggregation pipeline. By ensuring that hanging web searches are properly aborted and that CI jobs correctly identify failures in nightly runs, the changes improve both the user experience and the maintainability of the CI/CD process. 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
|
|
📊 PR Size: size/L
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 30-second timeout mechanism for the web search tool to prevent the agent loop from hanging indefinitely, accompanied by new unit tests. It also updates the evaluation aggregation script to fail during scheduled nightly runs if no reports are found, and adds corresponding tests. The review feedback correctly identifies a critical issue where the optional abortSignal is accessed without safety checks, which could lead to runtime TypeError crashes during setup and cleanup of the abort event listeners.
| const onExternalAbort = () => timeoutController.abort(signal.reason); | ||
| if (signal.aborted) { | ||
| onExternalAbort(); | ||
| } else { | ||
| signal.addEventListener('abort', onExternalAbort, { once: true }); | ||
| } |
There was a problem hiding this comment.
The abortSignal parameter in ExecuteOptions is optional, meaning signal can be undefined at runtime. Accessing signal.reason, signal.aborted, or calling signal.addEventListener directly without a safety check will throw a TypeError and crash the tool execution. We should guard these accesses to ensure robustness. Additionally, the abort() method should not take any arguments.
const onExternalAbort = () => timeoutController.abort();
if (signal) {
if (signal.aborted) {
onExternalAbort();
} else {
signal.addEventListener('abort', onExternalAbort, { once: true });
}
}References
- When consuming an object, if a property is optional in its type definition (interface), callers must handle the undefined case (e.g., by providing a default or optional chaining). Do not rely on the implementation details of the function that creates the object to always provide a value.
- The abort() method should not take arguments and should not throw an error. It should silently handle aborting the current stream.
| }; | ||
| } finally { | ||
| clearTimeout(timer); | ||
| signal.removeEventListener('abort', onExternalAbort); |
There was a problem hiding this comment.
If signal is undefined, calling signal.removeEventListener in the finally block will throw a TypeError. Use optional chaining to safely remove the event listener.
| signal.removeEventListener('abort', onExternalAbort); | |
| signal?.removeEventListener('abort', onExternalAbort); |
References
- When consuming an object, if a property is optional in its type definition (interface), callers must handle the undefined case (e.g., by providing a default or optional chaining). Do not rely on the implementation details of the function that creates the object to always provide a value.
gemini-cli
Bot
added
priority/p1
Addresses review feedback and completes the coverage of google-gemini#29594: - ExecuteOptions.abortSignal is optional, so the external signal is now handled defensively through createTimeoutAbortHandle. - The primary LLM-mediated WebFetch path had the same never-settling request hazard as WebSearch; a timed-out primary fetch now falls back to the direct URL fetch path, which keeps its own per-URL timeout. - The timer and listener logic moves into a shared createTimeoutAbortHandle utility with its own unit tests.
This branch is waiting to be deployed
1 participant
Summary
The
GoogleSearchandWebFetchtools passed only the caller's abort signal togeminiClient.generateContent, so a request whose underlying LLM call never settled left the agent loop in a permanentThinking...state (the issue reports 30+ minute hangs) until the user pressed Esc. This PR adds a 30 second execution timeout to both tools, as requested in the issue.GoogleSearch: when the timer fires, the request is aborted and the tool returns a structuredWEB_SEARCH_FAILEDtool error, letting the model recover or inform the user.WebFetch: the primary LLM-mediated fetch aborts on timeout and the existing all-or-nothing fallback takes over — the direct URL fetch path, which keeps its own per-URL timeout. External cancellation still stops the fallback, while a primary-fetch timeout does not.createTimeoutAbortHandleutility (packages/core/src/utils/abort.ts) that combines the external signal with the timeout and reports which one fired. It handles the case whereExecuteOptions.abortSignalisundefined.Details
Web search was cancelled.for search) and remains distinguishable from a timeout viadidTimeout().finallyblocks, including on the success path.Related Issues
Fixes #29594
How to Validate
npx vitest run packages/core/src/utils/abort.test.ts packages/core/src/tools/web-search.test.ts packages/core/src/tools/web-fetch.test.ts— 79 tests pass, including 5 new utility tests, 2 newGoogleSearchtests (timeout → structured error; external abort before the timeout keeps the unchanged cancelled result), and 1 newWebFetchtest (timed-out primary fetch falls back to the direct URL path and returns its result).Validation notes (honest scope): Prettier, ESLint, and
tsc --noEmitpass on the touched files; the fullpackages/coresuite andnpm run preflightwere not run locally — CI covers them.Pre-Merge Checklist