From ce3f4c94516953c32fe369586ad9ca4ec4266c31 Mon Sep 17 00:00:00 2001 From: syf2211 Date: Thu, 30 Jul 2026 00:06:23 +0000 Subject: [PATCH 1/6] fix(gappsscript): default update_script_content to safe merge mode 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 #923 --- README.md | 2 +- README_NEW.md | 2 +- gappsscript/README.md | 6 +- gappsscript/apps_script_tools.py | 70 +++++++++++++++++-- .../references/apps-script.md | 5 +- tests/gappsscript/test_apps_script_tools.py | 68 +++++++++++++++++- 6 files changed, 139 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 9d6fc1652..af5ba41ac 100644 --- a/README.md +++ b/README.md @@ -1033,7 +1033,7 @@ The required scopes (`chat.spaces.readonly`, `chat.messages.readonly`, `chat.mes | `get_script_project` | Core | Get complete project with all files | | `get_script_content` | Core | Retrieve specific file content | | `create_script_project` | Core | Create new standalone or bound project | -| `update_script_content` | Core | Update or create script files | +| `update_script_content` | Core | Merge or replace script project files | | `run_script_function` | Core | Execute function with parameters | | `list_deployments` | Extended | List all project deployments | | `manage_deployment` | Extended | Create, update, or delete script deployments | diff --git a/README_NEW.md b/README_NEW.md index 88b25bbcc..c7e223373 100644 --- a/README_NEW.md +++ b/README_NEW.md @@ -174,7 +174,7 @@ export OAUTHLIB_INSECURE_TRANSPORT=1 # Development only | `get_script_project` | Core | Get complete project with all files | | `get_script_content` | Core | Retrieve specific file content | | `create_script_project` | Core | Create new standalone or bound project | -| `update_script_content` | Core | Update or create script files | +| `update_script_content` | Core | Merge or replace script project files | | `run_script_function` | Core | Execute function with parameters | | `list_deployments` | Extended | List all project deployments | | `manage_deployment` | Extended | Create, update, or delete script deployments | diff --git a/gappsscript/README.md b/gappsscript/README.md index 6a28d501b..e78eaaf6b 100644 --- a/gappsscript/README.md +++ b/gappsscript/README.md @@ -186,7 +186,7 @@ Essential operations for reading, writing, and executing scripts: - `get_script_project`: Get full project with all files - `get_script_content`: Get specific file content - `create_script_project`: Create new project -- `update_script_content`: Modify project files +- `update_script_content`: Merge or replace project files (`merge=true` by default) - `run_script_function`: Execute functions - `generate_trigger_code`: Generate trigger setup code @@ -271,7 +271,9 @@ Files: The AI will: 1. Read current code 2. Generate improved version -3. Call `update_script_content` with new files +3. Call `update_script_content` with the changed files (`merge=true` merges by file name) + +To delete files or replace the entire project, pass `merge=false` with the complete desired file set. ### Run Script Function diff --git a/gappsscript/apps_script_tools.py b/gappsscript/apps_script_tools.py index 4c218e1c7..3bf72dc21 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -16,6 +16,35 @@ logger = logging.getLogger(__name__) +def _normalize_script_file(file: Dict[str, Any]) -> Dict[str, str]: + """Return the Script API file fields used for updateContent requests.""" + return { + "name": file.get("name", ""), + "type": file.get("type", ""), + "source": file.get("source", ""), + } + + +def _merge_script_files( + existing_files: List[Dict[str, Any]], + updated_files: List[Dict[str, Any]], +) -> List[Dict[str, str]]: + """Overlay updated files onto the current project by file name.""" + merged = { + file.get("name"): _normalize_script_file(file) + for file in existing_files + if file.get("name") + } + + for file in updated_files: + name = file.get("name") + if not name: + continue + merged[name] = _normalize_script_file(file) + + return list(merged.values()) + + # Internal implementation functions for testing async def _list_script_projects_impl( service: Any, @@ -320,26 +349,45 @@ async def _update_script_content_impl( user_google_email: str, script_id: str, files: List[Dict[str, str]], + merge: bool = True, ) -> str: """Internal implementation for update_script_content.""" logger.info( - f"[update_script_content] Email: {user_google_email}, ID: {script_id}, Files: {len(files)}" + f"[update_script_content] Email: {user_google_email}, ID: {script_id}, " + f"Files: {len(files)}, merge: {merge}" ) - request_body = {"files": files} + files_to_push = [_normalize_script_file(file) for file in files] + + if merge: + current_content = await asyncio.to_thread( + service.projects().getContent(scriptId=script_id).execute + ) + files_to_push = _merge_script_files( + current_content.get("files", []), files_to_push + ) + + request_body = {"files": files_to_push} updated_content = await asyncio.to_thread( service.projects().updateContent(scriptId=script_id, body=request_body).execute ) - output = [f"Updated script project: {script_id}", "", "Modified files:"] + mode = "merged into project" if merge else "replaced entire project" + output = [ + f"Updated script project: {script_id} ({mode})", + "", + "Files in project after update:", + ] for file in updated_content.get("files", []): file_name = file.get("name", "Untitled") file_type = file.get("type", "Unknown") output.append(f"- {file_name} ({file_type})") - logger.info(f"[update_script_content] Updated {len(files)} files in {script_id}") + logger.info( + f"[update_script_content] Updated {len(files_to_push)} files in {script_id}" + ) return "\n".join(output) @@ -359,21 +407,29 @@ async def update_script_content( user_google_email: str, script_id: str, files: List[Dict[str, str]], + merge: bool = True, ) -> str: """ - Updates or creates files in a script project. + Update or create files in a script project. + + By default this merges the supplied files into the existing project by file + name, leaving other files untouched. Set merge=False to replace the entire + project: any existing file omitted from `files` is permanently deleted. Args: service: Injected Google API service client user_google_email: User's email address script_id: The script project ID - files: List of file objects with name, type, and source + files: File objects with name, type, and source to create or update + merge: When True (default), overlay these files onto the current + project. When False, replace the full project file set; omitted + files are deleted. Returns: str: Formatted string confirming update with file list """ return await _update_script_content_impl( - service, user_google_email, script_id, files + service, user_google_email, script_id, files, merge ) diff --git a/skills/managing-google-workspace/references/apps-script.md b/skills/managing-google-workspace/references/apps-script.md index b9c616e33..0dbd07846 100644 --- a/skills/managing-google-workspace/references/apps-script.md +++ b/skills/managing-google-workspace/references/apps-script.md @@ -63,13 +63,14 @@ Retrieves content of a specific file within a project. | file_name | string | yes | | | ### update_script_content -Updates or creates files in a script project. +Merge or replace files in a script project. Defaults to merging supplied files by name into the existing project; set `merge=false` to replace the full project (omitted files are deleted). | Parameter | Type | Required | Default | Notes | |-----------|------|----------|---------|-------| | user_google_email | string | yes | | | | script_id | string | yes | | | -| files | array | yes | | List of objects with `name`, `type`, and `source` | +| files | array | yes | | Objects with `name`, `type`, and `source` | +| merge | boolean | no | true | `true` overlays updates; `false` replaces the entire project | --- diff --git a/tests/gappsscript/test_apps_script_tools.py b/tests/gappsscript/test_apps_script_tools.py index e4bdfc248..29f516394 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -20,6 +20,7 @@ _get_script_project_impl, _create_script_project_impl, _update_script_content_impl, + _merge_script_files, _run_script_function_impl, _create_deployment_impl, _list_deployments_impl, @@ -140,7 +141,7 @@ async def test_create_script_project(): @pytest.mark.asyncio async def test_update_script_content(): - """Test updating script project files""" + """Test updating script project files with merge disabled.""" mock_service = Mock() files_to_update = [ {"name": "Code", "type": "SERVER_JS", "source": "function main() {}"} @@ -154,10 +155,75 @@ async def test_update_script_content(): user_google_email="test@example.com", script_id="test123", files=files_to_update, + merge=False, ) assert "Updated script project: test123" in result + assert "replaced entire project" in result assert "Code" in result + mock_service.projects().getContent.assert_not_called() + + +def test_merge_script_files_overlays_updates_and_preserves_existing(): + existing = [ + {"name": "Code", "type": "SERVER_JS", "source": "old code"}, + {"name": "appsscript", "type": "JSON", "source": "{}"}, + ] + updates = [{"name": "Code", "type": "SERVER_JS", "source": "new code"}] + + merged = _merge_script_files(existing, updates) + + assert len(merged) == 2 + by_name = {file["name"]: file for file in merged} + assert by_name["Code"]["source"] == "new code" + assert by_name["appsscript"]["source"] == "{}" + + +def test_merge_script_files_adds_new_file(): + existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] + updates = [{"name": "Utils", "type": "SERVER_JS", "source": "function util() {}"}] + + merged = _merge_script_files(existing, updates) + + assert len(merged) == 2 + by_name = {file["name"]: file for file in merged} + assert by_name["Utils"]["source"] == "function util() {}" + assert by_name["Code"]["source"] == "code" + + +@pytest.mark.asyncio +async def test_update_script_content_merge_fetches_existing_files(): + """Test merge=True overlays updates onto the current project.""" + mock_service = Mock() + existing_files = [ + {"name": "Code", "type": "SERVER_JS", "source": "old code"}, + {"name": "appsscript", "type": "JSON", "source": "{}"}, + ] + files_to_update = [ + {"name": "Code", "type": "SERVER_JS", "source": "new code"} + ] + merged_files = _merge_script_files(existing_files, files_to_update) + + mock_service.projects().getContent().execute.return_value = { + "files": existing_files + } + mock_service.projects().updateContent().execute.return_value = { + "files": merged_files + } + + result = await _update_script_content_impl( + service=mock_service, + user_google_email="test@example.com", + script_id="test123", + files=files_to_update, + merge=True, + ) + + update_body = mock_service.projects().updateContent.call_args.kwargs["body"] + assert update_body == {"files": merged_files} + assert "merged into project" in result + assert "Code" in result + assert "appsscript" in result @pytest.mark.asyncio From 2d882f3572518555f9756f87210157733f9d3357 Mon Sep 17 00:00:00 2001 From: Taylor Wilsdon Date: Thu, 30 Jul 2026 14:11:58 -0400 Subject: [PATCH 2/6] refac --- gappsscript/apps_script_tools.py | 38 +++++++++++---- tests/gappsscript/test_apps_script_tools.py | 52 +++++++++++++++++++-- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/gappsscript/apps_script_tools.py b/gappsscript/apps_script_tools.py index 3bf72dc21..5fa539d70 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -17,21 +17,29 @@ def _normalize_script_file(file: Dict[str, Any]) -> Dict[str, str]: - """Return the Script API file fields used for updateContent requests.""" - return { - "name": file.get("name", ""), - "type": file.get("type", ""), - "source": file.get("source", ""), - } + """Return the Script API file fields used for updateContent requests. + + Output-only fields returned by getContent (createTime, functionSet, ...) + are dropped. Fields the caller omitted stay omitted so a merge can fall + back to the existing value instead of blanking it. + """ + return {key: file[key] for key in ("name", "type", "source") if key in file} def _merge_script_files( existing_files: List[Dict[str, Any]], updated_files: List[Dict[str, Any]], ) -> List[Dict[str, str]]: - """Overlay updated files onto the current project by file name.""" + """Overlay updated files onto the current project. + + Files are keyed by (name, type) because Script API names exclude the + extension, so one project may hold both Code.gs and Code.html as "Code". + An update that omits `type` falls back to matching by name, but only when + exactly one existing file carries that name; otherwise the type cannot be + inferred and a ValueError is raised rather than pushing an untyped file. + """ merged = { - file.get("name"): _normalize_script_file(file) + (file["name"], file.get("type")): _normalize_script_file(file) for file in existing_files if file.get("name") } @@ -40,7 +48,17 @@ def _merge_script_files( name = file.get("name") if not name: continue - merged[name] = _normalize_script_file(file) + key = (name, file.get("type")) + if key not in merged and file.get("type") is None: + same_name = [existing for existing in merged if existing[0] == name] + if len(same_name) != 1: + raise ValueError( + f"File '{name}' is missing 'type'; it must be one of " + "SERVER_JS, HTML, or JSON because the existing project " + "does not identify a single file with that name." + ) + key = same_name[0] + merged[key] = {**merged.get(key, {}), **_normalize_script_file(file)} return list(merged.values()) @@ -386,7 +404,7 @@ async def _update_script_content_impl( output.append(f"- {file_name} ({file_type})") logger.info( - f"[update_script_content] Updated {len(files_to_push)} files in {script_id}" + f"[update_script_content] Pushed {len(files_to_push)} files to {script_id}" ) return "\n".join(output) diff --git a/tests/gappsscript/test_apps_script_tools.py b/tests/gappsscript/test_apps_script_tools.py index 29f516394..79c624dfc 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -191,17 +191,60 @@ def test_merge_script_files_adds_new_file(): assert by_name["Code"]["source"] == "code" +def test_merge_script_files_keeps_existing_fields_when_omitted(): + existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] + updates = [{"name": "Code", "source": "new code"}] + + merged = _merge_script_files(existing, updates) + + assert merged == [{"name": "Code", "type": "SERVER_JS", "source": "new code"}] + + +def test_merge_script_files_keeps_same_name_different_type(): + """Script API names exclude extensions, so Code.gs and Code.html collide.""" + existing = [ + {"name": "Code", "type": "SERVER_JS", "source": "server"}, + {"name": "Code", "type": "HTML", "source": ""}, + ] + updates = [{"name": "Code", "type": "SERVER_JS", "source": "new server"}] + + merged = _merge_script_files(existing, updates) + + assert merged == [ + {"name": "Code", "type": "SERVER_JS", "source": "new server"}, + {"name": "Code", "type": "HTML", "source": ""}, + ] + + +def test_merge_script_files_does_not_guess_when_name_is_ambiguous(): + """An update without a type must not clobber one of two same-name files.""" + existing = [ + {"name": "Code", "type": "SERVER_JS", "source": "server"}, + {"name": "Code", "type": "HTML", "source": ""}, + ] + updates = [{"name": "Code", "source": "new source"}] + + with pytest.raises(ValueError, match="missing 'type'"): + _merge_script_files(existing, updates) + + +def test_merge_script_files_requires_type_for_new_file(): + existing = [{"name": "Code", "type": "SERVER_JS", "source": "server"}] + updates = [{"name": "Utils", "source": "function util() {}"}] + + with pytest.raises(ValueError, match="missing 'type'"): + _merge_script_files(existing, updates) + + @pytest.mark.asyncio async def test_update_script_content_merge_fetches_existing_files(): - """Test merge=True overlays updates onto the current project.""" + """Test the default merge mode overlays updates onto the current project.""" mock_service = Mock() existing_files = [ {"name": "Code", "type": "SERVER_JS", "source": "old code"}, {"name": "appsscript", "type": "JSON", "source": "{}"}, ] - files_to_update = [ - {"name": "Code", "type": "SERVER_JS", "source": "new code"} - ] + files_to_update = [{"name": "Code", "type": "SERVER_JS", "source": "new code"}] merged_files = _merge_script_files(existing_files, files_to_update) mock_service.projects().getContent().execute.return_value = { @@ -216,7 +259,6 @@ async def test_update_script_content_merge_fetches_existing_files(): user_google_email="test@example.com", script_id="test123", files=files_to_update, - merge=True, ) update_body = mock_service.projects().updateContent.call_args.kwargs["body"] From 037851ea3ef2ff036db51de931ba8dd985ee7169 Mon Sep 17 00:00:00 2001 From: Taylor Wilsdon Date: Thu, 30 Jul 2026 14:22:01 -0400 Subject: [PATCH 3/6] refac --- gappsscript/apps_script_tools.py | 6 ++++-- tests/gappsscript/test_apps_script_tools.py | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/gappsscript/apps_script_tools.py b/gappsscript/apps_script_tools.py index 5fa539d70..d8dfd1939 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -44,10 +44,12 @@ def _merge_script_files( if file.get("name") } - 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'." + ) key = (name, file.get("type")) if key not in merged and file.get("type") is None: same_name = [existing for existing in merged if existing[0] == name] diff --git a/tests/gappsscript/test_apps_script_tools.py b/tests/gappsscript/test_apps_script_tools.py index 79c624dfc..5120de78d 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -191,6 +191,13 @@ def test_merge_script_files_adds_new_file(): assert by_name["Code"]["source"] == "code" +def test_merge_script_files_rejects_update_without_name(): + existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] + + with pytest.raises(ValueError, match="index 0.*non-empty 'name'"): + _merge_script_files(existing, [{"type": "SERVER_JS", "source": "new code"}]) + + def test_merge_script_files_keeps_existing_fields_when_omitted(): existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] updates = [{"name": "Code", "source": "new code"}] From a200635b01104f3fd96eb0b46f23d6f3e1b130dd Mon Sep 17 00:00:00 2001 From: Taylor Wilsdon Date: Thu, 30 Jul 2026 15:45:38 -0400 Subject: [PATCH 4/6] refac --- gappsscript/apps_script_tools.py | 44 +++++++---- tests/gappsscript/test_apps_script_tools.py | 86 ++++++++++++++++++++- 2 files changed, 112 insertions(+), 18 deletions(-) diff --git a/gappsscript/apps_script_tools.py b/gappsscript/apps_script_tools.py index d8dfd1939..48a3a117e 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -4,17 +4,28 @@ This module provides MCP tools for interacting with Google Apps Script API. """ -import logging import asyncio -from typing import List, Dict, Any, Optional +import logging +import weakref +from typing import Any, Dict, List, Optional + +from mcp.types import ToolAnnotations from auth.service_decorator import require_google_service from core.server import server -from mcp.types import ToolAnnotations -from core.utils import handle_http_errors, ObjectList +from core.utils import ObjectList, handle_http_errors logger = logging.getLogger(__name__) +_SCRIPT_UPDATE_LOCKS: weakref.WeakValueDictionary[str, asyncio.Lock] = ( + weakref.WeakValueDictionary() +) + + +def _get_script_update_lock(script_id: str) -> asyncio.Lock: + """Return the in-process lock that orders updates for one script.""" + return _SCRIPT_UPDATE_LOCKS.setdefault(script_id, asyncio.Lock()) + def _normalize_script_file(file: Dict[str, Any]) -> Dict[str, str]: """Return the Script API file fields used for updateContent requests. @@ -379,19 +390,22 @@ async def _update_script_content_impl( files_to_push = [_normalize_script_file(file) for file in files] - if merge: - current_content = await asyncio.to_thread( - service.projects().getContent(scriptId=script_id).execute - ) - files_to_push = _merge_script_files( - current_content.get("files", []), files_to_push - ) + async with _get_script_update_lock(script_id): + if merge: + current_content = await asyncio.to_thread( + service.projects().getContent(scriptId=script_id).execute + ) + files_to_push = _merge_script_files( + current_content.get("files", []), files_to_push + ) - request_body = {"files": files_to_push} + request_body = {"files": files_to_push} - updated_content = await asyncio.to_thread( - service.projects().updateContent(scriptId=script_id, body=request_body).execute - ) + updated_content = await asyncio.to_thread( + service.projects() + .updateContent(scriptId=script_id, body=request_body) + .execute + ) mode = "merged into project" if merge else "replaced entire project" output = [ diff --git a/tests/gappsscript/test_apps_script_tools.py b/tests/gappsscript/test_apps_script_tools.py index 5120de78d..1347ba468 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -4,11 +4,14 @@ Tests all Apps Script tools with mocked API responses """ -import pytest +import asyncio +import os +import sys +import threading from typing import get_type_hints from unittest.mock import Mock -import sys -import os + +import pytest from pydantic import TypeAdapter @@ -275,6 +278,83 @@ async def test_update_script_content_merge_fetches_existing_files(): assert "appsscript" in result +@pytest.mark.asyncio +async def test_update_script_content_orders_concurrent_merges_for_same_script(): + """A later merge must fetch content after an earlier update completes.""" + content = [{"name": "Base", "type": "SERVER_JS", "source": "base"}] + first_update_started = threading.Event() + allow_first_update = threading.Event() + first_update_applied = threading.Event() + second_get_started = threading.Event() + update_count = 0 + get_count = 0 + + class Request: + def __init__(self, execute): + self._execute = execute + + def execute(self): + return self._execute() + + class Projects: + def getContent(self, scriptId): + nonlocal get_count + get_count += 1 + if get_count == 2: + second_get_started.set() + snapshot = [file.copy() for file in content] + return Request(lambda: {"files": snapshot}) + + def updateContent(self, scriptId, body): + nonlocal update_count + update_count += 1 + update_number = update_count + + def execute(): + nonlocal content + if update_number == 1: + first_update_started.set() + assert allow_first_update.wait(timeout=1) + else: + assert first_update_applied.wait(timeout=1) + content = [file.copy() for file in body["files"]] + if update_number == 1: + first_update_applied.set() + return {"files": content} + + return Request(execute) + + projects = Projects() + service = Mock() + service.projects.side_effect = lambda: projects + + first_update = asyncio.create_task( + _update_script_content_impl( + service=service, + user_google_email="test@example.com", + script_id="test123", + files=[{"name": "First", "type": "SERVER_JS", "source": "first"}], + ) + ) + assert await asyncio.to_thread(first_update_started.wait, 1) + + second_update = asyncio.create_task( + _update_script_content_impl( + service=service, + user_google_email="test@example.com", + script_id="test123", + files=[{"name": "Second", "type": "SERVER_JS", "source": "second"}], + ) + ) + await asyncio.sleep(0) + second_get_raced = second_get_started.is_set() + allow_first_update.set() + await asyncio.gather(first_update, second_update) + + assert not second_get_raced + assert {file["name"] for file in content} == {"Base", "First", "Second"} + + @pytest.mark.asyncio async def test_run_script_function(): """Test executing script function""" From dd663ac4ac7bb7acb5416ce2f43b7fd2a2046c01 Mon Sep 17 00:00:00 2001 From: Taylor Wilsdon Date: Thu, 30 Jul 2026 18:20:22 -0400 Subject: [PATCH 5/6] add locks to serialize updates only within same process --- gappsscript/apps_script_tools.py | 10 ++++++---- tests/gappsscript/test_apps_script_tools.py | 8 +++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/gappsscript/apps_script_tools.py b/gappsscript/apps_script_tools.py index 48a3a117e..0eb47ecad 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -13,10 +13,12 @@ from auth.service_decorator import require_google_service from core.server import server -from core.utils import ObjectList, handle_http_errors +from core.utils import ObjectList, UserInputError, handle_http_errors logger = logging.getLogger(__name__) +# These locks serialize updates only within this process. Other worker +# processes or service instances can still race while merging the same script_id. _SCRIPT_UPDATE_LOCKS: weakref.WeakValueDictionary[str, asyncio.Lock] = ( weakref.WeakValueDictionary() ) @@ -47,7 +49,7 @@ def _merge_script_files( extension, so one project may hold both Code.gs and Code.html as "Code". An update that omits `type` falls back to matching by name, but only when exactly one existing file carries that name; otherwise the type cannot be - inferred and a ValueError is raised rather than pushing an untyped file. + inferred and a UserInputError is raised rather than pushing an untyped file. """ merged = { (file["name"], file.get("type")): _normalize_script_file(file) @@ -58,14 +60,14 @@ def _merge_script_files( for index, file in enumerate(updated_files): name = file.get("name") if not name: - raise ValueError( + raise UserInputError( f"File at index {index} is missing a non-empty 'name'." ) key = (name, file.get("type")) if key not in merged and file.get("type") is None: same_name = [existing for existing in merged if existing[0] == name] if len(same_name) != 1: - raise ValueError( + raise UserInputError( f"File '{name}' is missing 'type'; it must be one of " "SERVER_JS, HTML, or JSON because the existing project " "does not identify a single file with that name." diff --git a/tests/gappsscript/test_apps_script_tools.py b/tests/gappsscript/test_apps_script_tools.py index 1347ba468..a0be01337 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -17,6 +17,8 @@ sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) +from core.utils import UserInputError + # Import the internal implementation functions (not the decorated ones) from gappsscript.apps_script_tools import ( _list_script_projects_impl, @@ -197,7 +199,7 @@ def test_merge_script_files_adds_new_file(): def test_merge_script_files_rejects_update_without_name(): existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] - with pytest.raises(ValueError, match="index 0.*non-empty 'name'"): + with pytest.raises(UserInputError, match="index 0.*non-empty 'name'"): _merge_script_files(existing, [{"type": "SERVER_JS", "source": "new code"}]) @@ -234,7 +236,7 @@ def test_merge_script_files_does_not_guess_when_name_is_ambiguous(): ] updates = [{"name": "Code", "source": "new source"}] - with pytest.raises(ValueError, match="missing 'type'"): + with pytest.raises(UserInputError, match="missing 'type'"): _merge_script_files(existing, updates) @@ -242,7 +244,7 @@ def test_merge_script_files_requires_type_for_new_file(): existing = [{"name": "Code", "type": "SERVER_JS", "source": "server"}] updates = [{"name": "Utils", "source": "function util() {}"}] - with pytest.raises(ValueError, match="missing 'type'"): + with pytest.raises(UserInputError, match="missing 'type'"): _merge_script_files(existing, updates) From 1ce4931f9f1818dcf0f2ce4baabae5f9b2514966 Mon Sep 17 00:00:00 2001 From: Taylor Wilsdon Date: Thu, 30 Jul 2026 19:01:53 -0400 Subject: [PATCH 6/6] refac --- gappsscript/apps_script_tools.py | 24 ++++++++++++++++--- tests/gappsscript/test_apps_script_tools.py | 26 +++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/gappsscript/apps_script_tools.py b/gappsscript/apps_script_tools.py index 0eb47ecad..64ebc96eb 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -17,6 +17,8 @@ logger = logging.getLogger(__name__) +_VALID_SCRIPT_FILE_TYPES = frozenset({"SERVER_JS", "HTML", "JSON"}) + # These locks serialize updates only within this process. Other worker # processes or service instances can still race while merging the same script_id. _SCRIPT_UPDATE_LOCKS: weakref.WeakValueDictionary[str, asyncio.Lock] = ( @@ -36,7 +38,11 @@ def _normalize_script_file(file: Dict[str, Any]) -> Dict[str, str]: are dropped. Fields the caller omitted stay omitted so a merge can fall back to the existing value instead of blanking it. """ - 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 + } def _merge_script_files( @@ -63,8 +69,20 @@ def _merge_script_files( raise UserInputError( f"File at index {index} is missing a non-empty 'name'." ) - key = (name, file.get("type")) - if key not in merged and file.get("type") is None: + file_type = file.get("type") + if file_type is not None: + if file_type not in _VALID_SCRIPT_FILE_TYPES: + raise UserInputError( + f"File '{name}' has unsupported type '{file_type}'; it must " + "be one of SERVER_JS, HTML, or JSON." + ) + if file_type == "JSON" and name != "appsscript": + raise UserInputError( + f"JSON file '{name}' must use the manifest name 'appsscript'." + ) + + key = (name, file_type) + if key not in merged and file_type is None: same_name = [existing for existing in merged if existing[0] == name] if len(same_name) != 1: raise UserInputError( diff --git a/tests/gappsscript/test_apps_script_tools.py b/tests/gappsscript/test_apps_script_tools.py index a0be01337..dfbe66d4c 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -212,6 +212,32 @@ def test_merge_script_files_keeps_existing_fields_when_omitted(): assert merged == [{"name": "Code", "type": "SERVER_JS", "source": "new code"}] +def test_merge_script_files_keeps_existing_type_when_update_type_is_none(): + existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] + updates = [{"name": "Code", "type": None, "source": "new code"}] + + merged = _merge_script_files(existing, updates) + + assert merged == [{"name": "Code", "type": "SERVER_JS", "source": "new code"}] + + +@pytest.mark.parametrize("file_type", ["TEXT", "", 123]) +def test_merge_script_files_rejects_unsupported_explicit_type(file_type): + with pytest.raises(UserInputError, match="unsupported type"): + _merge_script_files( + [], + [{"name": "Code", "type": file_type, "source": "source"}], + ) + + +def test_merge_script_files_rejects_json_file_without_manifest_name(): + with pytest.raises(UserInputError, match="manifest name 'appsscript'"): + _merge_script_files( + [], + [{"name": "config", "type": "JSON", "source": "{}"}], + ) + + def test_merge_script_files_keeps_same_name_different_type(): """Script API names exclude extensions, so Code.gs and Code.html collide.""" existing = [