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/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..64ebc96eb 100644 --- a/gappsscript/apps_script_tools.py +++ b/gappsscript/apps_script_tools.py @@ -4,17 +4,97 @@ 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, UserInputError, handle_http_errors 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] = ( + 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. + + 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 and file[key] is not None + } + + +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. + + 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 UserInputError is raised rather than pushing an untyped file. + """ + merged = { + (file["name"], file.get("type")): _normalize_script_file(file) + for file in existing_files + if file.get("name") + } + + for index, file in enumerate(updated_files): + name = file.get("name") + if not name: + raise UserInputError( + f"File at index {index} is missing a non-empty 'name'." + ) + 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( + 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()) + # Internal implementation functions for testing async def _list_script_projects_impl( @@ -320,26 +400,48 @@ 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] - updated_content = await asyncio.to_thread( - service.projects().updateContent(scriptId=script_id, body=request_body).execute - ) + 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 + ) - output = [f"Updated script project: {script_id}", "", "Modified files:"] + request_body = {"files": files_to_push} + + 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 = [ + 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] Pushed {len(files_to_push)} files to {script_id}" + ) return "\n".join(output) @@ -359,21 +461,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..dfbe66d4c 100644 --- a/tests/gappsscript/test_apps_script_tools.py +++ b/tests/gappsscript/test_apps_script_tools.py @@ -4,22 +4,28 @@ 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 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, _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 +146,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 +160,227 @@ 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" + + +def test_merge_script_files_rejects_update_without_name(): + existing = [{"name": "Code", "type": "SERVER_JS", "source": "code"}] + + with pytest.raises(UserInputError, 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"}] + + merged = _merge_script_files(existing, updates) + + 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 = [ + {"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(UserInputError, 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(UserInputError, match="missing 'type'"): + _merge_script_files(existing, updates) + + +@pytest.mark.asyncio +async def test_update_script_content_merge_fetches_existing_files(): + """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"}] + 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, + ) + + 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 +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