Add GitHub Actions CI for syntax checks and optional remote Apps Script tests; add clasp/Apps Script CI infra (WiP)#88
Conversation
|
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:
📝 WalkthroughWalkthroughAdds an Apps Script manifest and clasp config/ignore updates, CI test-prep script, a GitHub Actions workflow for syntax checks and optional remote Apps Script tests (API-key and Vertex AI modes), README CI docs, updates to test helper/auth wiring in src/testFunctions.gs, and a small syntax fix in src/code.gs. ChangesApps Script CI/CD Infrastructure
Estimated code review effort🎯 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📝 Generate docstrings
✨ Simplify code
Comment |
![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: 69ef59d193
ℹ️ 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".
| - name: Install clasp | ||
| run: npm install --global @google/clasp |
There was a problem hiding this comment.
On every push and pull_request, AUTH_TESTS_REQUESTED is false but this step still installs @google/clasp before the later skip logic runs. That means the automatic syntax-only CI path can fail because of an npm registry/clasp install issue even though no remote Apps Script tests were requested or possible without secrets; gate this job/step behind the remote-test request or clasp availability so the always-on syntax validation remains independent.
Useful? React with 👍 / 👎.
| if [ "$HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON" != "true" ]; then | ||
| missing+=("VERTEX_AI_SERVICE_ACCOUNT_JSON") |
There was a problem hiding this comment.
When enable-vertex-ai-tests=true, this check fails unless VERTEX_AI_SERVICE_ACCOUNT_JSON is set, but the workflow never writes or activates that JSON and .github/scripts/prepare-tests.sh only injects VERTEX_AI_PROJECT_ID/VERTEX_AI_LOCATION; the generated smoke test uses GenAIApp.setGeminiAuth(...), which authenticates through Apps Script. In an environment with valid clasp credentials and a linked Vertex-enabled Apps Script project but no service-account key, the Vertex tests are blocked by an unrelated unused secret.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 @.github/scripts/prepare-tests.sh:
- Around line 44-58: The script currently writes CI secrets into source by
emitting const OPEN_AI_API_KEY / GEMINI_API_KEY / VERTEX_AI_PROJECT_ID /
VERTEX_AI_LOCATION into "$TEST_FILE" (via tmp_file and the node heredoc), which
persists secrets to the Apps Script project when clasp push runs; stop injecting
secrets into src/testFunctions.gs and instead set them in a server-side secret
store or disposable project: change the prepare-tests.sh logic around the
declarations block so it does not append raw keys to TEST_FILE, and instead (a)
use Apps Script Script Properties (PropertiesService) by pushing a small setup
script that sets script properties or (b) run tests against an isolated
disposable Apps Script project by parameterizing the clasp target, or (c) export
the secrets to a separate non-committed config that the test runtime
reads—ensure no raw API keys or project IDs are written into source and remove
the node heredoc that writes const ... into TEST_FILE.
- Around line 64-69: The Vertex smoke test function testVertexAISimpleChat
currently logs the model response but never fails CI on unexpected output;
update testVertexAISimpleChat to assert the response equals the exact sentinel
"Vertex AI test passed" (or the full expected reply string) and throw an Error
when it does not match, including the actual response in the error message;
locate the response variable and replace the console.log-only behavior with a
conditional that throws (e.g., using throw new Error(...)) so clasp run will
mark the workflow failed on unexpected output.
In @.github/workflows/test.yml:
- Around line 82-87: The workflow is exporting and validating
VERTEX_AI_SERVICE_ACCOUNT_JSON but the test runner never uses it; update the CI
to stop requiring/validating VERTEX_AI_SERVICE_ACCOUNT_JSON and only pass the
secrets actually used by prepare-tests.sh and the Apps Script runner
(VERTEX_AI_PROJECT_ID and VERTEX_AI_LOCATION); remove
VERTEX_AI_SERVICE_ACCOUNT_JSON from the exported/validated secrets block (and
the duplicate block at lines ~131-149) and ensure prepare-tests.sh and
GenAIApp.setGeminiAuth remain unchanged to receive only VERTEX_AI_PROJECT_ID and
VERTEX_AI_LOCATION.
- Around line 23-31: Add a GitHub Actions concurrency guard to serialize runs
that push to the same Apps Script project: for each job that performs "clasp
push --force" or reads/writes the SCRIPT_ID-backed project (jobs that reference
env var SCRIPT_ID or contain the clasp push step), add a concurrency block such
as concurrency: { group: "apps-script-${{ env.SCRIPT_ID }}", cancel-in-progress:
false } so runs targeting the same SCRIPT_ID are queued (not run concurrently)
and cannot overlap; apply this change to all jobs that push/tests the Apps
Script project (the jobs around the clasp push steps referenced in the diff).
- Around line 37-43: Replace the mutable tags for the GitHub Actions with pinned
commit SHAs: locate every usage of actions/checkout@v4 and actions/setup-node@v4
in the workflow and change them to the corresponding specific commit SHAs from
their upstream repos (pin both occurrences found in the file), ensuring you use
the official repo commit hashes for checkout and setup-node so the workflow
references a fixed, auditable version.
- Around line 1-31: The workflow currently uses the default GITHUB_TOKEN
permissions; add an explicit top-level permissions block to restrict access
(e.g., set permissions: contents: read and actions: read) under the workflow
"Test" declaration so the job only has repo checkout/CI read access; update the
.github/workflows/test.yml file by inserting a permissions section (permissions:
contents: read, actions: read) right after the name: Test line to limit token
scope.
- Around line 37-38: The workflow's checkout steps use actions/checkout@v4
without disabling credential persistence; update both checkout steps that
reference actions/checkout@v4 to include a with block setting
persist-credentials: false so the runner does not retain the GITHUB_TOKEN in
local git config (i.e., add the with: persist-credentials: false setting to each
actions/checkout@v4 step).
In `@appsscript.json`:
- Around line 4-6: The executionApi.access is currently set to "ANYONE", which
is overly permissive; change the executionApi.access value in appsscript.json
from "ANYONE" to a more restrictive option (e.g., "MYSELF" or "DOMAIN")
appropriate for your deployment so only authorized callers can invoke the
script, and verify the new value works with your intended callers and CI/deploy
processes.
In `@src/code.gs`:
- Around line 1053-1056: The closing brace for enableCodeInterpreter is
misplaced, causing subsequent Chat methods to be declared inside
enableCodeInterpreter; fix this by closing enableCodeInterpreter immediately
after setting _codeInterpreterContainerId and returning this (i.e., add "return
this;" then "};" right after the line that assigns _codeInterpreterContainerId)
so the rest of the Chat methods (all other this.* functions) are defined on the
Chat instance scope rather than nested inside enableCodeInterpreter.
🪄 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: 3177774a-320b-4656-b5a9-a0f5ba02a7e2
📒 Files selected for processing (8)
.clasp.json.example.claspignore.github/scripts/prepare-tests.sh.github/workflows/test.yml.gitignoreREADME.mdappsscript.jsonsrc/code.gs
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~80-~80: The official name of this software platform is spelled with a capital “H”.
Context: ...e test execution. The workflow lives at .github/workflows/test.yml; helper files inclu...
(GITHUB)
[uncategorized] ~80-~80: The official name of this software platform is spelled with a capital “H”.
Context: ...laspignore, .clasp.json.example, and .github/scripts/prepare-tests.sh. The real .c...
(GITHUB)
[style] ~139-~139: ‘without warning’ might be wordy. Consider a shorter alternative.
Context: .... - Credentials expire. This can happen without warning, so periodic regeneration may be requir...
(EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
[style] ~152-~152: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...and syntax validation can still pass. - If any auth mode is enabled and `CLASPRC_J...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~153-~153: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...gure or regenerate clasp credentials. - If API-key tests are enabled but `OPEN_AI_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~154-~154: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...fore pushing or running remote tests. - If Vertex AI tests are enabled but the req...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~161-~161: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...d VERTEX_AI_SERVICE_ACCOUNT_JSON. - Run all remote tests: set both inputs to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~170-~170: The official name of this software platform is spelled with a capital “H”.
Context: ...pps Script project or test file. In CI, .github/scripts/prepare-tests.sh injects `cons...
(GITHUB)
🪛 zizmor (1.25.2)
.github/workflows/test.yml
[warning] 37-38: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 90-91: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 1-267: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 32-65: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 91-91: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 94-94: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[warning] 3-21: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🔇 Additional comments (4)
README.md (1)
10-10: LGTM!Also applies to: 78-232
.clasp.json.example (1)
1-4: LGTM!.claspignore (1)
1-35: LGTM!.gitignore (1)
1-9: LGTM!
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Ping-pong health check failed |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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)
.github/scripts/prepare-tests.sh (1)
64-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winExtra closing brace causes syntax error in generated test function.
The heredoc generates a function
testVertexAISimpleChat()but includes an extraneous}on line 74 after the function already closes on line 73. This will cause a JavaScript syntax error when the generated code is pushed and parsed.🐛 Proposed fix
if (String(response).trim() !== "Vertex AI test passed") { throw new Error(`Unexpected Vertex AI response: ${response}`); } console.log(`Vertex AI simple chat response:\n${response}`); } -} EOF_VERTEX_TEST🤖 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 @.github/scripts/prepare-tests.sh around lines 64 - 75, There is an extra closing brace in the generated heredoc for the function testVertexAISimpleChat causing a JS syntax error; remove the stray `}` that appears after the function's closing brace in the EOF_VERTEX_TEST heredoc so the function only has a single closing brace, then regenerate the file and re-run tests/lint to confirm the syntax is fixed..github/workflows/test.yml (1)
95-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSecond checkout step missing
persist-credentials: false.The first checkout (line 41-43) correctly disables credential persistence, but this checkout in the
apps-script-testsjob does not. Apply the same hardening for consistency.🔒 Suggested fix
- name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/test.yml around lines 95 - 96, In the apps-script-tests job update the second checkout step that uses actions/checkout@v4 to include the hardening option persist-credentials: false (same as the earlier checkout), so modify the checkout step under the apps-script-tests job to add persist-credentials: false to the action inputs for consistent credential handling.
🤖 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 @.github/scripts/prepare-tests.sh:
- Around line 64-75: There is an extra closing brace in the generated heredoc
for the function testVertexAISimpleChat causing a JS syntax error; remove the
stray `}` that appears after the function's closing brace in the EOF_VERTEX_TEST
heredoc so the function only has a single closing brace, then regenerate the
file and re-run tests/lint to confirm the syntax is fixed.
In @.github/workflows/test.yml:
- Around line 95-96: In the apps-script-tests job update the second checkout
step that uses actions/checkout@v4 to include the hardening option
persist-credentials: false (same as the earlier checkout), so modify the
checkout step under the apps-script-tests job to add persist-credentials: false
to the action inputs for consistent credential handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1879dfef-5867-4008-8e11-351431ee855d
📒 Files selected for processing (3)
.github/scripts/prepare-tests.sh.github/workflows/test.ymlappsscript.json
📜 Review details
🔇 Additional comments (12)
.github/scripts/prepare-tests.sh (2)
44-58: Secrets are still injected into source files.The script writes
OPEN_AI_API_KEY,GEMINI_API_KEY,VERTEX_AI_PROJECT_ID, andVERTEX_AI_LOCATIONas const declarations intosrc/testFunctions.gs. Whenclasp push --forceruns, these secrets are persisted to the remote Apps Script project.
1-42: LGTM!.github/workflows/test.yml (9)
34-34: Missing concurrency guard for remote test runs.Concurrent workflow runs can overlap
clasp push --forceoperations to the same Apps Script project.
41-46: Actions should be pinned to commit SHAs.
actions/checkout@v4andactions/setup-node@v4use mutable tags instead of pinned commit hashes.
86-92:VERTEX_AI_SERVICE_ACCOUNT_JSONis validated but never used.The workflow requires this secret for Vertex AI tests (line 148-150) but
prepare-tests.shonly injectsVERTEX_AI_PROJECT_IDandVERTEX_AI_LOCATION.
23-25: LGTM!
40-48: LGTM!
50-70: LGTM!
103-111: LGTM!
159-215: LGTM!
241-271: LGTM!appsscript.json (1)
1-12: LGTM!
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)
.github/workflows/test.yml (2)
72-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the remote-test job behind the manual inputs.
This job still runs
npm install --global@google/clasp`` on everypush/`pull_request`, so a transient npm outage can fail syntax-only CI even when no remote tests were requested. That breaks the documented “syntax validation always runs, remote tests are optional” contract.Suggested change
apps-script-tests: name: Apps Script remote tests runs-on: ubuntu-latest needs: syntax-validation + if: github.event_name == 'workflow_dispatch' && (inputs['enable-api-key-tests'] || inputs['enable-vertex-ai-tests']) outputs: clasp-available: ${{ steps.clasp-setup.outputs.available }} clasp-authenticated: ${{ steps.clasp-auth.outputs.authenticated }}Also applies to: 115-116
🤖 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 @.github/workflows/test.yml around lines 72 - 75, The apps-script-tests job currently runs on every push/PR; gate it behind a manual workflow input by adding an if-condition on the job (apps-script-tests) that only runs when the workflow was manually dispatched and the input is set (for example: check github.event_name == 'workflow_dispatch' && github.event.inputs.run_remote_tests == 'true'); add a corresponding workflow_dispatch input definition named run_remote_tests (boolean or string) at the top of the workflow; apply the same if-condition to the other remote-test job that also installs `@google/clasp` so both remote-test jobs only run when the manual run_remote_tests input is true.
234-241:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop pushing injected API keys into the remote Apps Script project.
This path pushes
OPEN_AI_API_KEYandGEMINI_API_KEYinto Apps Script source because.github/scripts/prepare-tests.shprepends them tosrc/testFunctions.gsbeforeclasp push --force. That stores live credentials in the test project itself, where any project editor can read them and project history can retain them. Please switch the CI flow to provision secrets through Script Properties or another runtime-only channel instead of embedding them in pushed source.🤖 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 @.github/workflows/test.yml around lines 234 - 241, The workflow currently prepends OPEN_AI_API_KEY and GEMINI_API_KEY into src/testFunctions.gs via .github/scripts/prepare-tests.sh and then runs clasp push --force, which embeds secrets in the remote project; remove that injection and stop pushing credentials. Update .github/scripts/prepare-tests.sh to not modify src/testFunctions.gs (or add a non-sensitive test-only file), remove or keep clasp push only for clean source, and add a new CI step that provisions secrets at runtime using Script Properties (Apps Script API or a clasp-compatible properties/set call) or another runtime-only channel before running tests so tests can read keys via PropertiesService rather than from committed source. Ensure the workflow references the same secret names (OPEN_AI_API_KEY, GEMINI_API_KEY) when setting Script Properties and remove any step that pushes injected keys to the repo or remote project history.
♻️ Duplicate comments (2)
.github/workflows/test.yml (2)
85-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
VERTEX_AI_SERVICE_ACCOUNT_JSONfrom this workflow.This secret is exported, masked, and required here, but nothing in this job or
.github/scripts/prepare-tests.shforwards it into Apps Script. Vertex mode only consumesVERTEX_AI_PROJECT_IDandVERTEX_AI_LOCATION, so this currently fails runs for a secret the remote code never sees.Suggested change
- HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON: ${{ secrets.VERTEX_AI_SERVICE_ACCOUNT_JSON != '' }} CLASPRC_JSON: ${{ secrets.CLASPRC_JSON }} SCRIPT_ID: ${{ secrets.SCRIPT_ID }} OPEN_AI_API_KEY: ${{ secrets.OPEN_AI_API_KEY }} GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }} VERTEX_AI_PROJECT_ID: ${{ secrets.VERTEX_AI_PROJECT_ID }} - VERTEX_AI_SERVICE_ACCOUNT_JSON: ${{ secrets.VERTEX_AI_SERVICE_ACCOUNT_JSON }} @@ - for value in "$CLASPRC_JSON" "$SCRIPT_ID" "$OPEN_AI_API_KEY" "$GEMINI_API_KEY" "$VERTEX_AI_PROJECT_ID" "$VERTEX_AI_SERVICE_ACCOUNT_JSON"; do + for value in "$CLASPRC_JSON" "$SCRIPT_ID" "$OPEN_AI_API_KEY" "$GEMINI_API_KEY" "$VERTEX_AI_PROJECT_ID"; do if [ -n "$value" ]; then echo "::add-mask::$value" fi done @@ - if [ "$HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON" != "true" ]; then - missing+=("VERTEX_AI_SERVICE_ACCOUNT_JSON") - fi - if [ "${`#missing`[@]}" -gt 0 ]; then printf '::error::Vertex AI tests were requested but required configuration value(s) are missing: %s\n' "${missing[*]}" exit 1Also applies to: 105-109, 138-152
🤖 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 @.github/workflows/test.yml around lines 85 - 92, Remove the unused Vertex service account secret from the workflow by deleting the HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON and VERTEX_AI_SERVICE_ACCOUNT_JSON environment entries (the lines setting HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON: ${{ secrets.VERTEX_AI_SERVICE_ACCOUNT_JSON != '' }} and VERTEX_AI_SERVICE_ACCOUNT_JSON: ${{ secrets.VERTEX_AI_SERVICE_ACCOUNT_JSON }}), and ensure no other job sections (repeat removals where the same pair appears) export or rely on VERTEX_AI_SERVICE_ACCOUNT_JSON; keep only VERTEX_AI_PROJECT_ID and VERTEX_AI_LOCATION as required for Vertex mode and confirm prepare-tests.sh or other scripts do not expect that secret.
72-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize runs that push to the shared Apps Script project.
Every remote test run force-pushes to the same
SCRIPT_ID. Without aconcurrencyguard, overlapping runs can overwrite each other and execute against the wrong revision.Suggested change
apps-script-tests: name: Apps Script remote tests runs-on: ubuntu-latest needs: syntax-validation + concurrency: + group: apps-script-remote-tests + cancel-in-progress: false outputs: clasp-available: ${{ steps.clasp-setup.outputs.available }} clasp-authenticated: ${{ steps.clasp-auth.outputs.authenticated }}Also applies to: 239-241
🤖 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 @.github/workflows/test.yml around lines 72 - 75, The apps-script-tests workflow job ("apps-script-tests") lacks a concurrency guard, so parallel runs can force-push to the same SCRIPT_ID and clobber revisions; add a concurrency stanza to the workflow job (using a group keyed to the shared SCRIPT_ID or an environment variable like SCRIPT_ID or GITHUB_REF and cancel-in-progress: true) to serialize runs that push to the shared Apps Script project and prevent overlapping executions.
🤖 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 @.github/workflows/test.yml:
- Around line 72-75: The apps-script-tests job currently runs on every push/PR;
gate it behind a manual workflow input by adding an if-condition on the job
(apps-script-tests) that only runs when the workflow was manually dispatched and
the input is set (for example: check github.event_name == 'workflow_dispatch' &&
github.event.inputs.run_remote_tests == 'true'); add a corresponding
workflow_dispatch input definition named run_remote_tests (boolean or string) at
the top of the workflow; apply the same if-condition to the other remote-test
job that also installs `@google/clasp` so both remote-test jobs only run when the
manual run_remote_tests input is true.
- Around line 234-241: The workflow currently prepends OPEN_AI_API_KEY and
GEMINI_API_KEY into src/testFunctions.gs via .github/scripts/prepare-tests.sh
and then runs clasp push --force, which embeds secrets in the remote project;
remove that injection and stop pushing credentials. Update
.github/scripts/prepare-tests.sh to not modify src/testFunctions.gs (or add a
non-sensitive test-only file), remove or keep clasp push only for clean source,
and add a new CI step that provisions secrets at runtime using Script Properties
(Apps Script API or a clasp-compatible properties/set call) or another
runtime-only channel before running tests so tests can read keys via
PropertiesService rather than from committed source. Ensure the workflow
references the same secret names (OPEN_AI_API_KEY, GEMINI_API_KEY) when setting
Script Properties and remove any step that pushes injected keys to the repo or
remote project history.
---
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 85-92: Remove the unused Vertex service account secret from the
workflow by deleting the HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON and
VERTEX_AI_SERVICE_ACCOUNT_JSON environment entries (the lines setting
HAS_VERTEX_AI_SERVICE_ACCOUNT_JSON: ${{ secrets.VERTEX_AI_SERVICE_ACCOUNT_JSON
!= '' }} and VERTEX_AI_SERVICE_ACCOUNT_JSON: ${{
secrets.VERTEX_AI_SERVICE_ACCOUNT_JSON }}), and ensure no other job sections
(repeat removals where the same pair appears) export or rely on
VERTEX_AI_SERVICE_ACCOUNT_JSON; keep only VERTEX_AI_PROJECT_ID and
VERTEX_AI_LOCATION as required for Vertex mode and confirm prepare-tests.sh or
other scripts do not expect that secret.
- Around line 72-75: The apps-script-tests workflow job ("apps-script-tests")
lacks a concurrency guard, so parallel runs can force-push to the same SCRIPT_ID
and clobber revisions; add a concurrency stanza to the workflow job (using a
group keyed to the shared SCRIPT_ID or an environment variable like SCRIPT_ID or
GITHUB_REF and cancel-in-progress: true) to serialize runs that push to the
shared Apps Script project and prevent overlapping executions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 273249f4-06e2-4d9b-8b8d-978235d86372
📒 Files selected for processing (2)
.github/scripts/prepare-tests.sh.github/workflows/test.yml
💤 Files with no reviewable changes (1)
- .github/scripts/prepare-tests.sh
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 @.github/workflows/test.yml:
- Around line 82-84: The workflow concurrency currently uses cancel-in-progress:
true which can abort jobs after setCITestScriptProperties succeeds but before
the final cleanup, leaving CI credentials in PropertiesService; change the
concurrency behavior to serialize runs instead of canceling by setting
cancel-in-progress to false (or removing that key) for the group configuration
so jobs queue and complete their cleanup step reliably.
- Around line 247-264: The workflow is destructively overwriting Apps Script CI
properties: setCITestScriptProperties() deletes existing
OPEN_AI_API_KEY/GEMINI_API_KEY/VERTEX_AI_PROJECT_ID/VERTEX_AI_LOCATION and
clearCITestScriptProperties() always deletes them on teardown, risking loss of
non-CI config for the project referenced by SCRIPT_ID; change the workflow
and/or the implementation so you either restrict SCRIPT_ID to a disposable
CI-only project or implement a save-and-restore flow — before calling
setCITestScriptProperties() read and persist any existing values (e.g., into a
temporary secret/artifact or a backup properties object) and restore them in the
teardown step instead of blindly calling clearCITestScriptProperties(), and
update the teardown to restore backed-up values when present rather than always
deleting the keys.
In `@src/testFunctions.gs`:
- Around line 125-134: In testVertexAISimpleChat, replace the strict equality
check that throws on unexpected responses with a tolerant assertion: instead of
comparing String(response).trim() === "Vertex AI test passed", check that the
response contains the expected phrase (or matches a permissive regex) so small
punctuation/whitespace/formatting changes won't fail CI; update the if in
testVertexAISimpleChat to use String(response).includes("Vertex AI test passed")
or a regex like /Vertex AI test passed/i and keep the error throw but include
the full response in the message for debugging.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c52f7d85-3dc1-4290-b114-48dfcb60832d
📒 Files selected for processing (4)
.github/scripts/prepare-tests.sh.github/workflows/test.ymlappsscript.jsonsrc/testFunctions.gs
📜 Review details
🔇 Additional comments (13)
appsscript.json (3)
1-6: LGTM!
10-11: LGTM!
9-9: Drive OAuth scope is needed (DriveApp is used for reading and writing).The project actively uses Drive via
DriveApp.getFileById(...)to load input files (e.g., for code interpreter) andDriveApp.createFile(...)to create generated output files (src/code.gs, plus test helpers). Therefore thehttps://www.googleapis.com/auth/drivescope shouldn’t be removed; any attempt to narrow it (e.g.,drive.readonly/drive.file) would need validation because the code writes back to Drive and accepts arbitrary file IDs.> Likely an incorrect or invalid review comment.src/testFunctions.gs (8)
6-11: LGTM!
13-35: LGTM!
37-58: LGTM!
60-74: LGTM!
99-99: LGTM!
216-216: LGTM!
240-240: LGTM!
253-253: LGTM!.github/scripts/prepare-tests.sh (1)
31-43: LGTM!.github/workflows/test.yml (1)
6-10: LGTM!Also applies to: 32-34, 79-79, 112-120, 238-245
|
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 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken:
Lines 81–87 needs: syntax-validation
concurrency:
group: ${{ github.repository }}-${{ github.workflow }}-apps-script-tests
- cancel-in-progress: true
+ cancel-in-progress: false
outputs:
clasp-available: ${{ steps.clasp-setup.outputs.available }}
clasp-authenticated: ${{ steps.clasp-auth.outputs.authenticated }}Lines 244–249 if: env.AUTH_TESTS_REQUESTED == 'true' && steps.clasp-auth.outputs.authenticated == 'true'
run: clasp push --force
+ - name: Backup existing script properties
+ if: env.AUTH_TESTS_REQUESTED == 'true' && steps.clasp-auth.outputs.authenticated == 'true'
+ shell: bash
+ run: |
+ set -euo pipefail
+ # Read existing CI test property values and save them for restoration
+ backup="$(clasp run 'function() {
+ var props = PropertiesService.getScriptProperties();
+ var backup = {};
+ ["OPEN_AI_API_KEY", "GEMINI_API_KEY", "VERTEX_AI_PROJECT_ID", "VERTEX_AI_LOCATION"].forEach(function(key) {
+ var value = props.getProperty(key);
+ if (value !== null) {
+ backup[key] = value;
+ }
+ });
+ return JSON.stringify(backup);
+ }' 2>&1 | tail -n 1 || echo '{}')"
+ echo "PROPERTY_BACKUP=$backup" >> "$GITHUB_ENV"
+ echo "Backed up existing properties: $backup"
+
- name: Provision test script properties
if: env.AUTH_TESTS_REQUESTED == 'true' && steps.clasp-auth.outputs.authenticated == 'true'
shell: bashLines 295–305 clasp run "$function_name"
done
- - name: Clear test script properties
+ - name: Restore backed-up script properties
if: always() && env.AUTH_TESTS_REQUESTED == 'true' && steps.clasp-auth.outputs.authenticated == 'true'
shell: bash
run: |
set -euo pipefail
+ # First clear all CI test properties
params="$(node -e '
const propertyNames = [
"OPEN_AI_API_KEY",Lines 310–312 process.stdout.write(JSON.stringify([propertyNames]));
')"
clasp run clearCITestScriptProperties --params "$params"
+
+ # Then restore any backed-up values that existed before the test run
+ backup="${PROPERTY_BACKUP:-{}}"
+ if [ "$backup" != "{}" ] && [ -n "$backup" ]; then
+ echo "Restoring backed-up properties: $backup"
+ clasp run 'function(backup) {
+ var props = PropertiesService.getScriptProperties();
+ var restored = JSON.parse(backup);
+ for (var key in restored) {
+ if (restored.hasOwnProperty(key)) {
+ props.setProperty(key, restored[key]);
+ }
+ }
+ return "Restored " + Object.keys(restored).length + " properties";
+ }' --params "$(node -e 'process.stdout.write(JSON.stringify([process.env.PROPERTY_BACKUP || "{}"]))')"
+ else
+ echo "No backed-up properties to restore."
+ fi |
Fixed 1 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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 `@README.md`:
- Around line 104-105: Update the README to explicitly document the required
boolean gate run_remote_tests=true alongside enable-api-key-tests and
enable-vertex-ai-tests: add a sentence to the manual step that tells users to
set run_remote_tests=true when running any remote test mode and update the
“Workflow inputs and behavior” table rows that reference enable-api-key-tests /
enable-vertex-ai-tests (the sections around the existing text mentioning
enable-api-key-tests and enable-vertex-ai-tests and the later block at lines
~143-147) to include a column/entry for run_remote_tests (required=true) so
reviewers can see that remote tests will not start unless run_remote_tests is
set.
🪄 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: 1ea2193d-fe67-4d42-beff-8bdde5c499ca
📒 Files selected for processing (9)
.clasp.json.example.claspignore.github/scripts/prepare-tests.sh.github/workflows/test.yml.gitignoreREADME.mdappsscript.jsonsrc/code.gssrc/testFunctions.gs
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~80-~80: The official name of this software platform is spelled with a capital “H”.
Context: ...e test execution. The workflow lives at .github/workflows/test.yml; helper files inclu...
(GITHUB)
[uncategorized] ~80-~80: The official name of this software platform is spelled with a capital “H”.
Context: ...laspignore, .clasp.json.example, and .github/scripts/prepare-tests.sh. The real .c...
(GITHUB)
[style] ~139-~139: ‘without warning’ might be wordy. Consider a shorter alternative.
Context: .... - Credentials expire. This can happen without warning, so periodic regeneration may be requir...
(EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
[style] ~152-~152: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...and syntax validation can still pass. - If any auth mode is enabled and `CLASPRC_J...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~153-~153: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...gure or regenerate clasp credentials. - If API-key tests are enabled but `OPEN_AI_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~154-~154: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...fore pushing or running remote tests. - If Vertex AI tests are enabled but the req...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~161-~161: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...d VERTEX_AI_SERVICE_ACCOUNT_JSON. - Run all remote tests: set both inputs to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~170-~170: The official name of this software platform is spelled with a capital “H”.
Context: ...pps Script project or test file. In CI, .github/scripts/prepare-tests.sh injects `cons...
(GITHUB)
🔇 Additional comments (18)
.github/scripts/prepare-tests.sh (1)
1-44: LGTM!.github/workflows/test.yml (8)
1-42: LGTM!
43-79: LGTM!
81-127: LGTM!
128-176: LGTM!
177-234: LGTM!
235-257: LGTM!
259-305: LGTM!
307-378: LGTM!src/code.gs (1)
323-324: LGTM!Also applies to: 1055-1055, 1057-1057
src/testFunctions.gs (8)
6-11: LGTM!
13-35: LGTM!
37-58: LGTM!
60-88: LGTM!
90-112: LGTM!
137-137: LGTM!
163-172: LGTM!
254-254: LGTM!Also applies to: 278-278, 291-291
| 5. Choose the branch and set `enable-api-key-tests` and/or `enable-vertex-ai-tests` to `true` only for the remote test modes you want to run. | ||
|
|
There was a problem hiding this comment.
Document the required run_remote_tests=true gate for manual remote runs.
The current instructions/tables only mention enable-api-key-tests / enable-vertex-ai-tests, but the workflow also requires run_remote_tests=true; otherwise remote tests never start. Please add this required input to the manual steps and the “Workflow inputs and behavior” table.
Also applies to: 143-147
🤖 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 `@README.md` around lines 104 - 105, Update the README to explicitly document
the required boolean gate run_remote_tests=true alongside enable-api-key-tests
and enable-vertex-ai-tests: add a sentence to the manual step that tells users
to set run_remote_tests=true when running any remote test mode and update the
“Workflow inputs and behavior” table rows that reference enable-api-key-tests /
enable-vertex-ai-tests (the sections around the existing text mentioning
enable-api-key-tests and enable-vertex-ai-tests and the later block at lines
~143-147) to include a column/entry for run_remote_tests (required=true) so
reviewers can see that remote tests will not start unless run_remote_tests is
set.
aubrypaul
changed the title
2 participants
Motivation
Description
.github/workflows/test.ymlthat always runs aSyntax validationjob usingnode --checkon eachsrc/*.gsfile and optionally runs remote Apps Script tests viaclaspwhen enabled and properly configured..github/scripts/prepare-tests.shwhich injects test constants intosrc/testFunctions.gsand optionally appends a Vertex AI smoke test function for CI-only execution..claspignore,.clasp.json.example,.gitignore, andappsscript.jsonplus a README section describing CI usage, required secrets, and troubleshooting.src/code.gsto correct object return/closing punctuation and close theChatclass block.Testing
Testworkflow will run anode --checksyntax validation step acrosssrc/*.gsfiles when the workflow runs.clasp pushthe limited project contents, and run configured Apps Script test functions viaclasp run(these remote tests are gated on secrets and were not run in this PR).Codex Task