Add Gemini code-execution artifact support and tests (WiP)#86
Add Gemini code-execution artifact support and tests (WiP)#86Benjamin-Sayaque wants to merge 13 commits into
Conversation
aubrypaul
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d62bf24607
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGemini code-execution responses now yield inline base64 artifacts extracted into _generatedFiles; spreadsheet uploads are marked for lazy CSV conversion resolved at payload build time for both OpenAI and Gemini; generated-file retrieval/download behavior and test harness model/config are updated. ChangesGemini Code Interpreter Artifacts & Spreadsheet Conversion
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/code.gs (1)
965-970:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
elsecauses empty string input to fail.Line 965 uses
ifinstead ofelse if. When an empty string is passed:
- Lines 962-964 correctly set
targetFileto_generatedFiles[0]- Line 968's
else if (typeof "" === "string")then executes and overwritestargetFilewith thefind()result (which will beundefinedsince no file matches an empty string)This causes the subsequent check at line 971 to throw an error instead of returning the first file.
🐛 Proposed fix
if (fileIdOrIndex === undefined || fileIdOrIndex === null) { targetFile = this._generatedFiles[0]; } 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") {🤖 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 965 - 970, The branch that overwrites targetFile when fileIdOrIndex is an empty string should skip empty-string inputs; update the string branch condition (the clause that currently reads else if (typeof fileIdOrIndex === "string")) to require a non-empty string (for example: else if (typeof fileIdOrIndex === "string" && fileIdOrIndex !== "") ) so that when fileIdOrIndex is "" the previously-set targetFile (e.g., _generatedFiles[0]) is not overwritten; touch the logic around the variable fileIdOrIndex and the assignment to targetFile in this block to ensure the subsequent existence check works correctly.
🤖 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 900-904: The artifact extraction loop using parts.forEach only
reads part.inlineData, so if Gemini returns snake_case inline_data those
artifacts are skipped; update the parts.forEach callback to normalize or
fallback to both shapes (e.g., const inlineData = part?.inlineData ||
part?.inline_data) and then use inlineData and inputInlineData as before; ensure
this mirrors the logic used in the collectInputInlineData helper to handle both
inlineData and inline_data consistently.
---
Outside diff comments:
In `@src/code.gs`:
- Around line 965-970: The branch that overwrites targetFile when fileIdOrIndex
is an empty string should skip empty-string inputs; update the string branch
condition (the clause that currently reads else if (typeof fileIdOrIndex ===
"string")) to require a non-empty string (for example: else if (typeof
fileIdOrIndex === "string" && fileIdOrIndex !== "") ) so that when fileIdOrIndex
is "" the previously-set targetFile (e.g., _generatedFiles[0]) is not
overwritten; touch the logic around the variable fileIdOrIndex and the
assignment to targetFile in this block to ensure the subsequent existence check
works correctly.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 629f0f05-9204-405c-84c6-e67b85e1aef1
📒 Files selected for processing (2)
src/code.gssrc/testFunctions.gs
📜 Review details
🔇 Additional comments (9)
src/code.gs (5)
305-326: LGTM!
555-572: LGTM!
930-939: LGTM!
981-994: LGTM!
1062-1066: LGTM!src/testFunctions.gs (4)
18-19: LGTM!
189-216: LGTM!
218-252: LGTM!
254-264: LGTM!
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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)
555-557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve Gemini artifacts across internal reruns.
When Gemini uses both code execution and user-defined functions, this branch captures artifacts from the current response, but the recursive
this.run()at Lines 669-674 re-enters Line 469 and clears_generatedFilesbefore the next turn. Since_buildGeminiPayload()can send bothfunctionDeclarationsandcode_execution,getGeneratedFiles()ends up keeping only the last loop's artifacts for that supported combination.💡 Proposed fix
- if (advancedParametersObject) { - return this.run(advancedParametersObject); - } - else { - return this.run(); - } + const currentGeneratedFiles = this._generatedFiles.slice(); + const result = advancedParametersObject ? this.run(advancedParametersObject) : this.run(); + this._generatedFiles = currentGeneratedFiles.concat(this._generatedFiles); + return result;Also applies to: 669-674
959-970:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the blank-string default path reachable.
Lines 962-964 intentionally treat
""as “use the first generated file”, but the separate string branch at Lines 968-970 immediately overwritestargetFilewith an empty lookup and then throws. Make this a singleif / else ifchain so the fallback survives.💡 Proposed fix
- if (typeof fileIdOrIndex === "number") { + else if (typeof fileIdOrIndex === "number") { targetFile = this._generatedFiles[fileIdOrIndex]; } else if (typeof fileIdOrIndex === "string") { targetFile = this._generatedFiles.find(file => file.fileId === fileIdOrIndex || file.filename === 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 959 - 970, The current branching allows the blank-string check (typeof fileIdOrIndex === "string" && fileIdOrIndex.trim() === "") to set targetFile but then the later separate if for typeof fileIdOrIndex === "number" / else if typeof fileIdOrIndex === "string" overwrites it; change the control flow into a single if / else if / else chain around fileIdOrIndex so the empty-string branch is mutually exclusive (e.g., check undefined/null, then trimmed-empty string, then number, then non-empty string) and assign targetFile from this._generatedFiles accordingly (references: fileIdOrIndex, targetFile, this._generatedFiles and the find using file.fileId/file.filename).
🤖 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 959-970: The current branching allows the blank-string check
(typeof fileIdOrIndex === "string" && fileIdOrIndex.trim() === "") to set
targetFile but then the later separate if for typeof fileIdOrIndex === "number"
/ else if typeof fileIdOrIndex === "string" overwrites it; change the control
flow into a single if / else if / else chain around fileIdOrIndex so the
empty-string branch is mutually exclusive (e.g., check undefined/null, then
trimmed-empty string, then number, then non-empty string) and assign targetFile
from this._generatedFiles accordingly (references: fileIdOrIndex, targetFile,
this._generatedFiles and the find using file.fileId/file.filename).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 845afd4d-2782-40c7-a3d7-0a9d67e5344f
📒 Files selected for processing (1)
src/code.gs
📜 Review details
🔇 Additional comments (1)
src/code.gs (1)
320-326: LGTM!Also applies to: 900-910, 986-994, 1062-1066
Fixed 1 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 327-331: Remove the leftover Git merge conflict markers
("<<<<<<<", "=======", ">>>>>>>") in the code and replace them with a single
empty line so the script contains no conflict artifacts; search for those
markers in src/code.gs (around where the diff shows the markers) and ensure the
final file has only one blank line in that spot and no other conflict markers
remain.
🪄 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: 8a3a12ad-66ca-4823-91ce-66dfa1516b32
📒 Files selected for processing (1)
src/code.gs
…o-csv-conversion-utilities Support spreadsheet attachments: convert XLSX/Sheets/ODS/XLS to CSV for Gemini
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/code.gs (1)
1032-1037: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMissing
elsereduces readability.Line 1032 should be
else ifto match the existing conditional chain structure. While the current logic works correctly (becausetypeof undefined !== "number"), the inconsistent structure makes the flow harder to follow.♻️ Suggested improvement
else if (typeof fileIdOrIndex === "string" && fileIdOrIndex.trim() === "") { targetFile = this._generatedFiles[0]; } - if (typeof fileIdOrIndex === "number") { + else if (typeof fileIdOrIndex === "number") { targetFile = this._generatedFiles[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 1032 - 1037, Change the conditional chain to be explicit by using "else if" for the string branch so the flow is consistent and more readable; locate the block that checks fileIdOrIndex (references: variable fileIdOrIndex, assignment targetFile, and the _generatedFiles array) and replace the standalone "if (typeof fileIdOrIndex === 'string')" with "else if (typeof fileIdOrIndex === 'string')" so the numeric and string checks form a single if/else-if chain.
🤖 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 358-362: There are stray tokens "return this;" and "};" outside
any function and a duplicated comment marker "/** OPTIONAL"; remove the two
standalone lines "return this;" and "};" (or if they belong to the preceding
method, move them inside that method so the function properly returns this and
closes with });) and delete the duplicate "/** OPTIONAL" so only one comment
remains; also scan the surrounding function/method (the one that should end
where these lines currently sit) to ensure its braces and return statement are
balanced and syntactically correct.
---
Outside diff comments:
In `@src/code.gs`:
- Around line 1032-1037: Change the conditional chain to be explicit by using
"else if" for the string branch so the flow is consistent and more readable;
locate the block that checks fileIdOrIndex (references: variable fileIdOrIndex,
assignment targetFile, and the _generatedFiles array) and replace the standalone
"if (typeof fileIdOrIndex === 'string')" with "else if (typeof fileIdOrIndex ===
'string')" so the numeric and string checks form a single if/else-if chain.
🪄 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: ad3fa90b-5324-45fd-ab3d-921d439d89b7
📒 Files selected for processing (1)
src/code.gs
📜 Review details
🔇 Additional comments (13)
src/code.gs (13)
165-218: LGTM!
757-796: LGTM!
925-981: LGTM!
1053-1061: LGTM!
1076-1113: LGTM!
1168-1172: LGTM!
1206-1209: LGTM!
2255-2270: LGTM!
2301-2343: LGTM!
2345-2362: LGTM!
2364-2384: LGTM!
2386-2429: LGTM!
2431-2470: LGTM!
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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 (1)
src/code.gs (1)
1032-1036:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLogic error:
ifshould beelse ifcausing empty string handling to fail.Line 1032 uses
ifinstead ofelse if. WhenfileIdOrIndexis an empty string:
- Line 1030 correctly sets
targetFile = this._generatedFiles[0]- Line 1032 doesn't match (not a number)
- Line 1035 matches (empty string is a string)
- Line 1036 overwrites
targetFiletoundefined(no file hasfileId === "")- Line 1038 throws "not found"
🐛 Proposed fix
- if (typeof fileIdOrIndex === "number") { + else if (typeof fileIdOrIndex === "number") { targetFile = this._generatedFiles[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 1032 - 1036, Replace the second unconditional if with an else if so the earlier branch that handles default/index values isn't overwritten for empty-string inputs: in the method that sets targetFile (uses this._generatedFiles and variable targetFile), change the `if (typeof fileIdOrIndex === "number")` / subsequent `if (typeof fileIdOrIndex === "string")` sequence to `if (typeof fileIdOrIndex === "number") { ... } else if (typeof fileIdOrIndex === "string") { ... }` so mounting a default targetFile (e.g., this._generatedFiles[0]) isn't clobbered when fileIdOrIndex === "" and targetFile remains valid.
🤖 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 1032-1036: Replace the second unconditional if with an else if so
the earlier branch that handles default/index values isn't overwritten for
empty-string inputs: in the method that sets targetFile (uses
this._generatedFiles and variable targetFile), change the `if (typeof
fileIdOrIndex === "number")` / subsequent `if (typeof fileIdOrIndex ===
"string")` sequence to `if (typeof fileIdOrIndex === "number") { ... } else if
(typeof fileIdOrIndex === "string") { ... }` so mounting a default targetFile
(e.g., this._generatedFiles[0]) isn't clobbered when fileIdOrIndex === "" and
targetFile remains valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d9eb69a-26a5-41c4-be3a-e583a75611a5
📒 Files selected for processing (2)
src/code.gssrc/testFunctions.gs
📜 Review details
🔇 Additional comments (14)
src/code.gs (10)
358-362: Critical syntax error: stray statements outside function scope.Lines 358-359 contain
return this;and};outside of any function body. Line 361-362 shows a duplicated/** OPTIONALcomment. This will cause the script to fail at parse time.This issue appears to persist from the previous review.
165-218: LGTM!
589-606: LGTM!
757-796: LGTM!
925-981: LGTM!
1048-1061: LGTM!
1075-1174: LGTM!
2255-2270: LGTM!
2301-2470: LGTM!
3018-3018: LGTM!src/testFunctions.gs (4)
18-19: LGTM!
218-252: LGTM!
254-264: LGTM!
189-216: Fix: EnsureGEMINI_API_KEYis provided totestGeminiCodeExecution()
testGeminiCodeExecution()referencesGEMINI_API_KEY, but a repository search forGEMINI_API_KEY =in*.gsfiles returns no definition. The test therefore relies on the Apps Script runtime (or test harness/config) supplyingGEMINI_API_KEYexternally; make that setup explicit (or load it from a configuration source like properties) so the test doesn’t fail due to an undefined constant.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 37-41: The code sets TIMEOUT_SECONDS using parseInt on
PropertiesService.getScriptProperties().getProperty which can yield NaN or
invalid values; update the initialization of TIMEOUT_SECONDS to defensively
parse and validate the property value (use parseInt or Number, check
isFinite/isNaN), fall back to the safe default (e.g., 360) when missing or
malformed, and clamp the result to allowed bounds (minimum sensible value and
maximum 1800) before using it; reference the TIMEOUT_SECONDS constant and the
call to PropertiesService.getScriptProperties().getProperty in your fix.
- Around line 2349-2383: The finally cleanup in _convertXlsxBlobToCsv can leave
temp files if Drive.Files.remove throws; wrap the
Drive.Files.remove(convertedFileId) call inside a try-catch so removal failures
are not swallowed silently—catch the error and log a clear message including
convertedFileId and originalFilename (e.g., via console.error) so admins can
investigate, but do not rethrow (we want the original error from
_spreadsheetToCsvResult to propagate if present).
In `@src/testFunctions.gs`:
- Around line 15-18: The commented-out call to testVision() lacks an
explanation; either restore or remove it. Update the invocation area (near
testVision(), testMaximumAPICalls(), testInputTokenWarning()) by adding a brief
comment stating why testVision() is disabled (e.g., requires API key, external
resources, or flaky) or delete the commented line entirely if the vision tests
are deprecated, and ensure any retained explanation mentions how/when to
re-enable the test.
🪄 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: ee34db71-8371-4bf8-ba0d-232566daa2c3
📒 Files selected for processing (2)
src/code.gssrc/testFunctions.gs
📜 Review details
🔇 Additional comments (22)
src/code.gs (18)
170-223: LGTM!
340-362: LGTM!
591-608: LGTM!
759-800: LGTM!
945-1001: LGTM!
1044-1066: LGTM!
1068-1081: LGTM!
1095-1151: LGTM!
1206-1210: LGTM!
1244-1248: LGTM!
1831-1836: LGTM!
2293-2308: LGTM!
2385-2402: LGTM!
2404-2424: LGTM!
2426-2469: LGTM!
2471-2483: LGTM!
2485-2497: LGTM!
2499-2510: LGTM!src/testFunctions.gs (4)
29-57: LGTM!
171-180: LGTM!
182-191: LGTM!
3-3: Confirm Gemini model ID:gemini-3.5-flashis the official API name.
gemini-3.5-flashis listed as the Gemini API model identifier in the official Gemini API docs, so the update insrc/testFunctions.gs(line 3) aligns with the supported model name.
| //testVision(); | ||
| testMaximumAPICalls(); | ||
| testInputTokenWarning(); | ||
| // OpenAI-only tests - require valid Drive file IDs. | ||
| // Code interpreter tests - require valid Drive file IDs. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
testVision() is commented out without explanation.
Consider adding a comment explaining why vision tests are disabled, or remove the commented code if it's no longer needed.
🤖 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/testFunctions.gs` around lines 15 - 18, The commented-out call to
testVision() lacks an explanation; either restore or remove it. Update the
invocation area (near testVision(), testMaximumAPICalls(),
testInputTokenWarning()) by adding a brief comment stating why testVision() is
disabled (e.g., requires API key, external resources, or flaky) or delete the
commented line entirely if the vision tests are deprecated, and ensure any
retained explanation mentions how/when to re-enable the test.
2 participants
Motivation
enableCodeInterpreterprovider-aware by documenting container reuse for OpenAI and ignoring container IDs for Gemini.Description
enableCodeInterpreterto returnthisand clarified that the optionalcontainerIdis only applicable to OpenAI containers._extractGeminiInlineArtifactsto parse Gemini responses and collect inline base64 artifacts with inferred filenames and MIME types, and integrated it in the run flow for Gemini models.getGeneratedFiles, extendingdownloadGeneratedFileto accept filename lookups and to decode base64dataartifacts into blobs, and changedgetContainerIdto returnnullfor Gemini models._buildGeminiPayloadto include acode_executiontool when code execution is enabled for Gemini, and retained existing OpenAI container download behavior.testGeminiCodeExecutionandtestGeminiCodeExecutionWithArtifact) and registered them intestAll().Testing
testAll()which includes the new Gemini tests, and the suite completed successfully.assertGeminiArtifactMetadataandassertBlobLikeare used by the Gemini tests to validate artifact metadata and blob decoding.Codex Task