Fixed tool config and removed tool combination setter#81
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoves the Chat class's ChangesGemini Tool Configuration Refactoring
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/code.gs`:
- Around line 317-322: The method enableCodeInterpreter is missing its closing
brace and the fluent return; fix the function by adding the terminating "};" and
a "return this;" before the closing so the method sets
this._codeInterpreterEnabled = true, optionally assigns
this._codeInterpreterContainerId when containerId is provided, and then returns
this to maintain chaining (refer to enableCodeInterpreter,
_codeInterpreterEnabled, and _codeInterpreterContainerId to locate the code).
- Around line 879-884: The bug is that the string branch can overwrite a prior
numeric/zero-index match when fileIdOrIndex is an empty string; in the method
that sets targetFile (references: variable targetFile, parameter fileIdOrIndex,
and array this._generatedFiles), change the second conditional from `if (typeof
fileIdOrIndex === "string")` to `else if (typeof fileIdOrIndex === "string")`
(or otherwise short-circuit after the numeric branch) so that a numeric/zero
path doesn't fall through and get clobbered by the string find() for an empty
string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7c517f5-1d35-4bfc-8a69-3d41406c4d28
📒 Files selected for processing (1)
src/code.gs
📜 Review details
🔇 Additional comments (9)
src/code.gs (9)
55-59: LGTM!
453-455: LGTM!
539-549: LGTM!
640-642: LGTM!Also applies to: 659-661
773-790: LGTM!
794-847: LGTM!Also applies to: 853-855, 891-896
910-954: LGTM!
1561-1561: LGTM!Also applies to: 1591-1598, 1622-1624
956-972: src/code.gs:956-972 — MissingincludeServerSideToolInvocationsis OK for browsing-only
Gemini’sincludeServerSideToolInvocationsis required when combining built-in tools (likegoogle_search/url_context) with custom function calling in the same request. In the browsing-only path here,payload.toolsadds only the built-in browsing tools andtool_configis intentionally left unset, so this should not preventgoogle_search/url_contextfrom working.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/code.gs`:
- Around line 317-322: The method enableCodeInterpreter is missing its closing
brace and the fluent return; fix the function by adding the terminating "};" and
a "return this;" before the closing so the method sets
this._codeInterpreterEnabled = true, optionally assigns
this._codeInterpreterContainerId when containerId is provided, and then returns
this to maintain chaining (refer to enableCodeInterpreter,
_codeInterpreterEnabled, and _codeInterpreterContainerId to locate the code).
- Around line 879-884: The bug is that the string branch can overwrite a prior
numeric/zero-index match when fileIdOrIndex is an empty string; in the method
that sets targetFile (references: variable targetFile, parameter fileIdOrIndex,
and array this._generatedFiles), change the second conditional from `if (typeof
fileIdOrIndex === "string")` to `else if (typeof fileIdOrIndex === "string")`
(or otherwise short-circuit after the numeric branch) so that a numeric/zero
path doesn't fall through and get clobbered by the string find() for an empty
string.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7c517f5-1d35-4bfc-8a69-3d41406c4d28
📒 Files selected for processing (1)
src/code.gs
📜 Review details
🔇 Additional comments (9)
src/code.gs (9)
55-59: LGTM!
453-455: LGTM!
539-549: LGTM!
640-642: LGTM!Also applies to: 659-661
773-790: LGTM!
794-847: LGTM!Also applies to: 853-855, 891-896
910-954: LGTM!
1561-1561: LGTM!Also applies to: 1591-1598, 1622-1624
956-972: src/code.gs:956-972 — MissingincludeServerSideToolInvocationsis OK for browsing-only
Gemini’sincludeServerSideToolInvocationsis required when combining built-in tools (likegoogle_search/url_context) with custom function calling in the same request. In the browsing-only path here,payload.toolsadds only the built-in browsing tools andtool_configis intentionally left unset, so this should not preventgoogle_search/url_contextfrom working.
🛑 Comments failed to post (2)
src/code.gs (2)
317-322:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
enableCodeInterpreteris missing closing brace and return statement.The function definition is incomplete—it lacks
return this;for method chaining (consistent with all other fluent methods) and the closing};. This will cause a JavaScript syntax error at parse time, breaking the entire library.🐛 Proposed fix
this.enableCodeInterpreter = function (containerId) { this._codeInterpreterEnabled = true; if (containerId) { this._codeInterpreterContainerId = containerId; } + return this; + }; /** * Includes the content of a web page in the prompt sent to openAI📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.this.enableCodeInterpreter = function (containerId) { this._codeInterpreterEnabled = true; if (containerId) { this._codeInterpreterContainerId = containerId; } return this; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/code.gs` around lines 317 - 322, The method enableCodeInterpreter is missing its closing brace and the fluent return; fix the function by adding the terminating "};" and a "return this;" before the closing so the method sets this._codeInterpreterEnabled = true, optionally assigns this._codeInterpreterContainerId when containerId is provided, and then returns this to maintain chaining (refer to enableCodeInterpreter, _codeInterpreterEnabled, and _codeInterpreterContainerId to locate the code).
879-884:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winLogic error: missing
elsecauses empty string handling to break.When
fileIdOrIndexis an empty string"", lines 876-877 correctly settargetFile = this._generatedFiles[0]. However, line 879 usesifinstead ofelse if, so execution falls through to line 882 wheretypeof "" === "string"is true, overwritingtargetFilewithfind()(which returnsundefinedfor empty string).🐛 Proposed fix
else if (typeof fileIdOrIndex === "string" && fileIdOrIndex.trim() === "") { targetFile = this._generatedFiles[0]; } - if (typeof fileIdOrIndex === "number") { + else if (typeof fileIdOrIndex === "number") { targetFile = this._generatedFiles[fileIdOrIndex]; } else if (typeof fileIdOrIndex === "string") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.else if (typeof fileIdOrIndex === "number") { targetFile = this._generatedFiles[fileIdOrIndex]; } else if (typeof fileIdOrIndex === "string") { targetFile = this._generatedFiles.find(file => file.fileId === fileIdOrIndex); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/code.gs` around lines 879 - 884, The bug is that the string branch can overwrite a prior numeric/zero-index match when fileIdOrIndex is an empty string; in the method that sets targetFile (references: variable targetFile, parameter fileIdOrIndex, and array this._generatedFiles), change the second conditional from `if (typeof fileIdOrIndex === "string")` to `else if (typeof fileIdOrIndex === "string")` (or otherwise short-circuit after the numeric branch) so that a numeric/zero path doesn't fall through and get clobbered by the string find() for an empty string.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/code.gs (2)
949-955:⚠️ Potential issue | 🟠 MajorFix Gemini
function_calling_config.allowed_function_namestypeIn
src/code.gslines 949-955,allowed_function_namesis assignedadvancedParametersObject.function_call(a string per README/JSDoc), but Gemini expects a list/array of function names forallowed_function_names(used with mode"ANY"). Normalize single values to a one-element array before assigning.Suggested fix
if (advancedParametersObject?.function_call) { + const allowedFunctionNames = Array.isArray(advancedParametersObject.function_call) + ? advancedParametersObject.function_call + : [advancedParametersObject.function_call]; + payload.tool_config.function_calling_config.mode = "ANY"; payload.tool_config.function_calling_config.allowed_function_names = - advancedParametersObject.function_call; + allowedFunctionNames; delete advancedParametersObject.function_call; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/code.gs` around lines 949 - 955, The code assigns advancedParametersObject.function_call (documented as a string) directly to payload.tool_config.function_calling_config.allowed_function_names, but Gemini expects an array; update the assignment in the block handling advancedParametersObject?.function_call so that if advancedParametersObject.function_call is a string you wrap it into a one-element array, and if it is already an array you assign it as-is; keep the rest of the logic (setting mode to "ANY" and deleting advancedParametersObject.function_call) unchanged and reference payload.tool_config.function_calling_config.allowed_function_names and advancedParametersObject.function_call when making the change.
958-973:⚠️ Potential issue | 🟠 MajorGuard
includeServerSideToolInvocationsfor Gemini 2.5 when combining browsing + function calling.In
src/code.gs(_buildGeminiPayload), whenbrowsingis enabled andtools.length > 0, the code always injects Gemini built-in browsing tools (url_context,google_search) and unconditionally setspayload.tool_config.includeServerSideToolInvocations = true. Google documents this built-in+custom tool “tool context circulation” path as a Gemini 3 capability, so this should be rejected/disabled forgemini-2.5-*models to avoid unsupported requests. https://ai.google.dev/gemini-api/docs/tool-combinationSuggested guard
if (browsing) { + const isGemini3Model = model.startsWith("gemini-3"); + if (tools.length > 0 && !isGemini3Model) { + throw new Error("[GenAIApp] - Combining Gemini browsing with function calling is only supported on Gemini 3 models."); + } + if (tools.length > 0) { payload.tool_config.includeServerSideToolInvocations = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/code.gs` around lines 958 - 973, When building Gemini payload in _buildGeminiPayload, the code unconditionally sets payload.tool_config.includeServerSideToolInvocations = true and injects built-in browsing tools when browsing && tools.length > 0; guard this behavior for gemini-2.5 models by checking the model identifier (e.g., model or modelName) and skip setting includeServerSideToolInvocations and injecting the built-in url_context/google_search tools when the model string matches "gemini-2.5-*" (or otherwise indicates 2.5); only enable includeServerSideToolInvocations and push those built-in tools for models that support tool-combination (non-2.5). Ensure you reference the existing symbols browsing, tools, payload, and payload.tool_config.includeServerSideToolInvocations in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/code.gs`:
- Around line 949-955: The code assigns advancedParametersObject.function_call
(documented as a string) directly to
payload.tool_config.function_calling_config.allowed_function_names, but Gemini
expects an array; update the assignment in the block handling
advancedParametersObject?.function_call so that if
advancedParametersObject.function_call is a string you wrap it into a
one-element array, and if it is already an array you assign it as-is; keep the
rest of the logic (setting mode to "ANY" and deleting
advancedParametersObject.function_call) unchanged and reference
payload.tool_config.function_calling_config.allowed_function_names and
advancedParametersObject.function_call when making the change.
- Around line 958-973: When building Gemini payload in _buildGeminiPayload, the
code unconditionally sets payload.tool_config.includeServerSideToolInvocations =
true and injects built-in browsing tools when browsing && tools.length > 0;
guard this behavior for gemini-2.5 models by checking the model identifier
(e.g., model or modelName) and skip setting includeServerSideToolInvocations and
injecting the built-in url_context/google_search tools when the model string
matches "gemini-2.5-*" (or otherwise indicates 2.5); only enable
includeServerSideToolInvocations and push those built-in tools for models that
support tool-combination (non-2.5). Ensure you reference the existing symbols
browsing, tools, payload, and
payload.tool_config.includeServerSideToolInvocations in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b9ffb40-df9a-40b4-a7ba-e3472e7d4670
📒 Files selected for processing (1)
src/code.gs
📜 Review details
🔇 Additional comments (1)
src/code.gs (1)
317-323: LGTM!
1 participant
No description provided.