fix(gappsscript): default update_script_content to safe merge mode - #982
Conversation
Add merge=True default so partial file updates overlay by name instead of deleting omitted project files. Document destructive full-replacement when merge=False and update skill/reference docs for LLM callers. Fixes taylorwilsdon#923
|
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:
📝 WalkthroughWalkthrough
ChangesApps Script content updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant update_script_content
participant AppsScriptAPI
Caller->>update_script_content: Submit files and merge option
update_script_content->>AppsScriptAPI: Fetch current files when merge=true
AppsScriptAPI-->>update_script_content: Return project content
update_script_content->>AppsScriptAPI: Update merged or replacement file set
AppsScriptAPI-->>Caller: Return status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/gappsscript/test_apps_script_tools.py (1)
194-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the default merge contract.
This test passes
merge=True, so a future default change would not be caught. Omit the argument here to verify the advertised default behavior.Proposed test adjustment
result = await _update_script_content_impl( service=mock_service, user_google_email="test@example.com", script_id="test123", files=files_to_update, - merge=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 `@tests/gappsscript/test_apps_script_tools.py` around lines 194 - 220, Update test_update_script_content_merge_fetches_existing_files to omit the explicit merge=True argument when calling _update_script_content_impl, so the test exercises the function’s advertised default merge behavior while preserving the existing setup and assertions.
🤖 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.
Nitpick comments:
In `@tests/gappsscript/test_apps_script_tools.py`:
- Around line 194-220: Update
test_update_script_content_merge_fetches_existing_files to omit the explicit
merge=True argument when calling _update_script_content_impl, so the test
exercises the function’s advertised default merge behavior while preserving the
existing setup and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00c7d660-e12b-4228-93b5-1894a752e5eb
📒 Files selected for processing (6)
README.mdREADME_NEW.mdgappsscript/README.mdgappsscript/apps_script_tools.pyskills/managing-google-workspace/references/apps-script.mdtests/gappsscript/test_apps_script_tools.py
…into fix/update-script-content-merge-default-923
|
Thanks! |
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)
gappsscript/apps_script_tools.py (1)
47-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject updates with a missing file name.
Default merge mode silently drops these entries, then pushes the unchanged project and reports success. Raise
ValueErrorinstead so callers can correct the malformed file payload.Proposed fix
- for file in updated_files: + for index, file in enumerate(updated_files): name = file.get("name") if not name: - continue + raise ValueError(f"File at index {index} is missing a non-empty 'name'.")🤖 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 `@gappsscript/apps_script_tools.py` around lines 47 - 50, Update the updated_files processing loop to raise ValueError when an entry lacks a valid name instead of continuing past it. Preserve normal processing for entries with names, ensuring malformed file payloads cannot be silently dropped or reported as a successful unchanged update.
🤖 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 `@gappsscript/apps_script_tools.py`:
- Around line 47-50: Update the updated_files processing loop to raise
ValueError when an entry lacks a valid name instead of continuing past it.
Preserve normal processing for entries with names, ensuring malformed file
payloads cannot be silently dropped or reported as a successful unchanged
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2bfddb9-9475-4e2b-b986-5cb23c18ca3b
📒 Files selected for processing (2)
gappsscript/apps_script_tools.pytests/gappsscript/test_apps_script_tools.py
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)
gappsscript/apps_script_tools.py (1)
382-394: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent lost updates during
merge=Truecontent updates.
updateScriptContentreads the project once, merges updates, and submits the completefilesarray. If another worker or external change modifies the same script betweengetContentandupdateContent, this update can be built from stale state and overwrite those changes. Add per-script_idserialization/locking, or implement retry/validate-before-write logic that detects concurrent misses.🤖 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 `@gappsscript/apps_script_tools.py` around lines 382 - 394, Update updateScriptContent’s merge=True path to prevent stale getContent results from overwriting concurrent changes: serialize operations per script_id with a per-script lock, or validate the fetched project state immediately before updateContent and retry when it changed. Ensure concurrent updates for the same script are detected or ordered safely while preserving the existing merge behavior.
🤖 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 `@gappsscript/apps_script_tools.py`:
- Around line 382-394: Update updateScriptContent’s merge=True path to prevent
stale getContent results from overwriting concurrent changes: serialize
operations per script_id with a per-script lock, or validate the fetched project
state immediately before updateContent and retry when it changed. Ensure
concurrent updates for the same script are detected or ordered safely while
preserving the existing merge behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 120b336c-3242-42cd-bbb3-357ea66b15be
📒 Files selected for processing (2)
gappsscript/apps_script_tools.pytests/gappsscript/test_apps_script_tools.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/gappsscript/test_apps_script_tools.py
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)
gappsscript/apps_script_tools.py (1)
40-76: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRaise
UserInputErrorfor file validation failures.
update_script_contentwraps_merge_script_files, andhandle_http_errorstreats plainException/ValueErroras unexpected tool errors withlogger.exception, while it logsUserInputErrorwarnings and re-raises it. These missing or ambiguous file names/types are expected user input failures, so switch these two raises toUserInputErrorafter importing it fromcore.utils.🤖 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 `@gappsscript/apps_script_tools.py` around lines 40 - 76, Update _merge_script_files to import and raise UserInputError from core.utils instead of ValueError for both missing file names and missing or ambiguous types. Preserve the existing validation messages and conditions so update_script_content and handle_http_errors classify these user input failures correctly.
🧹 Nitpick comments (1)
gappsscript/apps_script_tools.py (1)
20-28: 🩺 Stability & Availability | 🔵 TrivialPer-process lock only — won't serialize across multiple workers/instances.
_SCRIPT_UPDATE_LOCKSis process-local, so if this server ever runs with multiple worker processes/replicas, concurrent merges for the samescript_idfrom different processes can still race (lost update), even though single-process races are now correctly serialized. Worth documenting as a known limitation if multi-process/multi-instance deployment is supported.🤖 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 `@gappsscript/apps_script_tools.py` around lines 20 - 28, Document near _SCRIPT_UPDATE_LOCKS and _get_script_update_lock that the lock only serializes updates within a single process and does not coordinate multiple worker processes or service instances; explicitly note that multi-instance deployments can still race when merging the same script_id.
🤖 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 `@gappsscript/apps_script_tools.py`:
- Around line 40-76: Update _merge_script_files to import and raise
UserInputError from core.utils instead of ValueError for both missing file names
and missing or ambiguous types. Preserve the existing validation messages and
conditions so update_script_content and handle_http_errors classify these user
input failures correctly.
---
Nitpick comments:
In `@gappsscript/apps_script_tools.py`:
- Around line 20-28: Document near _SCRIPT_UPDATE_LOCKS and
_get_script_update_lock that the lock only serializes updates within a single
process and does not coordinate multiple worker processes or service instances;
explicitly note that multi-instance deployments can still race when merging the
same script_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb9f13c2-6a4e-434c-a122-8aa5189abcda
📒 Files selected for processing (2)
gappsscript/apps_script_tools.pytests/gappsscript/test_apps_script_tools.py
…into fix/update-script-content-merge-default-923 # Conflicts: # README_NEW.md
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 (3)
gappsscript/apps_script_tools.py (3)
20-29: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd concurrency guards for remote merges.
getContent/updateContentis still a server-side read-modify-replace, and thisasyncio.Lock, even with process isolation noted, does not serialize callers in other workers or service instances. Concurrency across those boundaries can cause the later update to discard another writer’s changes. Add distributed/remote locking or explicit single-writer guarantees ifupdate_script_contentis expected to be used concurrently.🤖 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 `@gappsscript/apps_script_tools.py` around lines 20 - 29, Add a distributed lock or equivalent single-writer guarantee around the remote read-modify-replace flow in update_script_content, covering both getContent and updateContent so callers across workers or service instances cannot overwrite one another’s changes. Retain the existing _get_script_update_lock as an in-process guard, and ensure the remote lock is acquired before reading and held through the update.
60-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate Apps Script file types before building merge keys.
_merge_script_files()currently accepts arbitrary non-nulltypevalues and can forward them to the API, includingJSONfor non-manifest files. RaiseUserInputErrorfor explicit types outsideSERVER_JS,HTML, and manifestJSON, and validate that manifestJSONis namedappsscriptbefore inserting intomerged.🤖 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 `@gappsscript/apps_script_tools.py` around lines 60 - 76, Update _merge_script_files() to validate each explicit file type before constructing the merge key: allow only SERVER_JS, HTML, and JSON, and require JSON files to use the manifest name appsscript; raise UserInputError for unsupported types or invalid JSON names, then proceed with key creation and merging only after validation.
32-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not copy explicit
Nonevalues into normalized files.
_normalize_script_filecurrently copies any present key, so"type": Nonesurvives normalization. After merge inference, line 76 overwrites the existing file type withNone, producing an invalid Apps ScriptupdateContentFile payload sincetypeshould beSERVER_JS,HTML, orJSON. Filter outNonevalues or reject explicit nulls consistently, and add a regression test.Proposed fix
- return {key: file[key] for key in ("name", "type", "source") if key in file} + return { + key: file[key] + for key in ("name", "type", "source") + if key in file and file[key] is not None + }🤖 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 `@gappsscript/apps_script_tools.py` around lines 32 - 39, Update _normalize_script_file to exclude fields whose values are None, ensuring normalized files never propagate explicit nulls for name, type, or source; preserve omitted fields for merge inference. Add a regression test covering a file with "type": None and verify the merged update retains the existing valid type.
🤖 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 `@gappsscript/apps_script_tools.py`:
- Around line 20-29: Add a distributed lock or equivalent single-writer
guarantee around the remote read-modify-replace flow in update_script_content,
covering both getContent and updateContent so callers across workers or service
instances cannot overwrite one another’s changes. Retain the existing
_get_script_update_lock as an in-process guard, and ensure the remote lock is
acquired before reading and held through the update.
- Around line 60-76: Update _merge_script_files() to validate each explicit file
type before constructing the merge key: allow only SERVER_JS, HTML, and JSON,
and require JSON files to use the manifest name appsscript; raise UserInputError
for unsupported types or invalid JSON names, then proceed with key creation and
merging only after validation.
- Around line 32-39: Update _normalize_script_file to exclude fields whose
values are None, ensuring normalized files never propagate explicit nulls for
name, type, or source; preserve omitted fields for merge inference. Add a
regression test covering a file with "type": None and verify the merged update
retains the existing valid type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c9fb9872-06d9-46a5-b36b-2e280cf82c7f
📒 Files selected for processing (2)
gappsscript/apps_script_tools.pytests/gappsscript/test_apps_script_tools.py
2 participants
Summary
update_script_contentnow defaults tomerge=true, overlaying supplied files onto the current Apps Script project by file name instead of replacing the entire project. This prevents accidental deletion of omitted files when LLM callers perform partial updates.Motivation
Fixes #923. Google Apps Script
projects.updateContentreplaces the full project file set, but the tool description implied partial updates. Agents repeatedly deleted unrelated files when pushing only the files they changed.Changes
_merge_script_fileshelper andmergeparameter (defaulttrue) toupdate_script_contentmerge=true: fetch current content, overlay updates by file name, push merged setmerge=false: preserve prior full-replacement semantics for intentional deletionsTests
uv run pytest tests/gappsscript/test_apps_script_tools.py -q→ 26 passeduv run ruff check gappsscript/apps_script_tools.py tests/gappsscript/test_apps_script_tools.py→ cleanNotes
merge=falsewith the complete desired file setmerge=trueadds onegetContentround-trip beforeupdateContentmerge=false; no silent delete path in merge modeFixes #923
Summary by CodeRabbit
New Features
Bug Fixes
Documentation