fix: address Copilot review feedback on PR #416

- Use consistent terminology in confirmation message ("background color"
  to match "text color")
- Tighten test assertions for invalid parameter validation to check
  parameter name and all allowed values instead of loose substring matches
- Remove unrelated calendar_tools.py changes (lambda reformatting, guest
  permissions, query_freebusy) that belong in separate PRs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: 2 people

gcalendar/calendar_tools.py

@@ -554,9 +554,6 @@ async def create_event(
     use_default_reminders: bool = True,
     transparency: Optional[str] = None,
     visibility: Optional[str] = None,
-    guests_can_modify: Optional[bool] = None,
-    guests_can_invite_others: Optional[bool] = None,
-    guests_can_see_other_guests: Optional[bool] = None,
 ) -> str:
     """
     Creates a new event.
@@ -577,9 +574,6 @@ async def create_event(
         use_default_reminders (bool): Whether to use calendar's default reminders. If False, uses custom reminders. Defaults to True.
         transparency (Optional[str]): Event transparency for busy/free status. "opaque" shows as Busy (default), "transparent" shows as Available/Free. Defaults to None (uses Google Calendar default).
         visibility (Optional[str]): Event visibility. "default" uses calendar default, "public" is visible to all, "private" is visible only to attendees, "confidential" is same as private (legacy). Defaults to None (uses Google Calendar default).
-        guests_can_modify (Optional[bool]): Whether attendees other than the organizer can modify the event. Defaults to None (uses Google Calendar default of False).
-        guests_can_invite_others (Optional[bool]): Whether attendees other than the organizer can invite others to the event. Defaults to None (uses Google Calendar default of True).
-        guests_can_see_other_guests (Optional[bool]): Whether attendees other than the organizer can see who the event's attendees are. Defaults to None (uses Google Calendar default of True).
 
     Returns:
         str: Confirmation message of the successful event creation with event link.
@@ -639,21 +633,6 @@ async def create_event(
     # Handle visibility validation
     _apply_visibility_if_valid(event_body, visibility, "create_event")
 
-    # Handle guest permissions
-    if guests_can_modify is not None:
-        event_body["guestsCanModify"] = guests_can_modify
-        logger.info(f"[create_event] Set guestsCanModify to {guests_can_modify}")
-    if guests_can_invite_others is not None:
-        event_body["guestsCanInviteOthers"] = guests_can_invite_others
-        logger.info(
-            f"[create_event] Set guestsCanInviteOthers to {guests_can_invite_others}"
-        )
-    if guests_can_see_other_guests is not None:
-        event_body["guestsCanSeeOtherGuests"] = guests_can_see_other_guests
-        logger.info(
-            f"[create_event] Set guestsCanSeeOtherGuests to {guests_can_see_other_guests}"
-        )
-
     if add_google_meet:
         request_id = str(uuid.uuid4())
         event_body["conferenceData"] = {
@@ -671,69 +650,61 @@ async def create_event(
         event_body["attachments"] = []
         drive_service = None
         try:
-            try:
-                drive_service = service._http and build(
-                    "drive", "v3", http=service._http
+            drive_service = service._http and build("drive", "v3", http=service._http)
+        except Exception as e:
+            logger.warning(f"Could not build Drive service for MIME type lookup: {e}")
+        for att in attachments:
+            file_id = None
+            if att.startswith("https://"):
+                # Match /d/<id>, /file/d/<id>, ?id=<id>
+                match = re.search(r"(?:/d/|/file/d/|id=)([\w-]+)", att)
+                file_id = match.group(1) if match else None
+                logger.info(
+                    f"[create_event] Extracted file_id '{file_id}' from attachment URL '{att}'"
                 )
-            except Exception as e:
-                logger.warning(
-                    f"Could not build Drive service for MIME type lookup: {e}"
+            else:
+                file_id = att
+                logger.info(
+                    f"[create_event] Using direct file_id '{file_id}' for attachment"
                 )
-            for att in attachments:
-                file_id = None
-                if att.startswith("https://"):
-                    # Match /d/<id>, /file/d/<id>, ?id=<id>
-                    match = re.search(r"(?:/d/|/file/d/|id=)([\w-]+)", att)
-                    file_id = match.group(1) if match else None
-                    logger.info(
-                        f"[create_event] Extracted file_id '{file_id}' from attachment URL '{att}'"
-                    )
-                else:
-                    file_id = att
-                    logger.info(
-                        f"[create_event] Using direct file_id '{file_id}' for attachment"
-                    )
-                if file_id:
-                    file_url = f"https://drive.google.com/open?id={file_id}"
-                    mime_type = "application/vnd.google-apps.drive-sdk"
-                    title = "Drive Attachment"
-                    # Try to get the actual MIME type and filename from Drive
-                    if drive_service:
-                        try:
-                            file_metadata = await asyncio.to_thread(
-                                lambda: drive_service.files()
-                                .get(
-                                    fileId=file_id,
-                                    fields="mimeType,name",
-                                    supportsAllDrives=True,
-                                )
-                                .execute()
+            if file_id:
+                file_url = f"https://drive.google.com/open?id={file_id}"
+                mime_type = "application/vnd.google-apps.drive-sdk"
+                title = "Drive Attachment"
+                # Try to get the actual MIME type and filename from Drive
+                if drive_service:
+                    try:
+                        file_metadata = await asyncio.to_thread(
+                            lambda: drive_service.files()
+                            .get(
+                                fileId=file_id,
+                                fields="mimeType,name",
+                                supportsAllDrives=True,
                             )
-                            mime_type = file_metadata.get("mimeType", mime_type)
-                            filename = file_metadata.get("name")
-                            if filename:
-                                title = filename
-                                logger.info(
-                                    f"[create_event] Using filename '{filename}' as attachment title"
-                                )
-                            else:
-                                logger.info(
-                                    "[create_event] No filename found, using generic title"
-                                )
-                        except Exception as e:
-                            logger.warning(
-                                f"Could not fetch metadata for file {file_id}: {e}"
+                            .execute()
+                        )
+                        mime_type = file_metadata.get("mimeType", mime_type)
+                        filename = file_metadata.get("name")
+                        if filename:
+                            title = filename
+                            logger.info(
+                                f"[create_event] Using filename '{filename}' as attachment title"
                             )
-                    event_body["attachments"].append(
-                        {
-                            "fileUrl": file_url,
-                            "title": title,
-                            "mimeType": mime_type,
-                        }
-                    )
-        finally:
-            if drive_service:
-                drive_service.close()
+                        else:
+                            logger.info(
+                                "[create_event] No filename found, using generic title"
+                            )
+                    except Exception as e:
+                        logger.warning(
+                            f"Could not fetch metadata for file {file_id}: {e}"
+                        )
+                event_body["attachments"].append(
+                    {
+                        "fileUrl": file_url,
+                        "title": title,
+                        "mimeType": mime_type,
+                    }
+                )
         created_event = await asyncio.to_thread(
             lambda: service.events()
             .insert(
@@ -824,9 +795,6 @@ async def modify_event(
     transparency: Optional[str] = None,
     visibility: Optional[str] = None,
     color_id: Optional[str] = None,
-    guests_can_modify: Optional[bool] = None,
-    guests_can_invite_others: Optional[bool] = None,
-    guests_can_see_other_guests: Optional[bool] = None,
 ) -> str:
     """
     Modifies an existing event.
@@ -848,9 +816,6 @@ async def modify_event(
         transparency (Optional[str]): Event transparency for busy/free status. "opaque" shows as Busy, "transparent" shows as Available/Free. If None, preserves existing transparency setting.
         visibility (Optional[str]): Event visibility. "default" uses calendar default, "public" is visible to all, "private" is visible only to attendees, "confidential" is same as private (legacy). If None, preserves existing visibility setting.
         color_id (Optional[str]): Event color ID (1-11). If None, preserves existing color.
-        guests_can_modify (Optional[bool]): Whether attendees other than the organizer can modify the event. If None, preserves existing setting.
-        guests_can_invite_others (Optional[bool]): Whether attendees other than the organizer can invite others to the event. If None, preserves existing setting.
-        guests_can_see_other_guests (Optional[bool]): Whether attendees other than the organizer can see who the event's attendees are. If None, preserves existing setting.
 
     Returns:
         str: Confirmation message of the successful event modification with event link.
@@ -939,21 +904,6 @@ async def modify_event(
     # Handle visibility validation
     _apply_visibility_if_valid(event_body, visibility, "modify_event")
 
-    # Handle guest permissions
-    if guests_can_modify is not None:
-        event_body["guestsCanModify"] = guests_can_modify
-        logger.info(f"[modify_event] Set guestsCanModify to {guests_can_modify}")
-    if guests_can_invite_others is not None:
-        event_body["guestsCanInviteOthers"] = guests_can_invite_others
-        logger.info(
-            f"[modify_event] Set guestsCanInviteOthers to {guests_can_invite_others}"
-        )
-    if guests_can_see_other_guests is not None:
-        event_body["guestsCanSeeOtherGuests"] = guests_can_see_other_guests
-        logger.info(
-            f"[modify_event] Set guestsCanSeeOtherGuests to {guests_can_see_other_guests}"
-        )
-
     if timezone is not None and "start" not in event_body and "end" not in event_body:
         # If timezone is provided but start/end times are not, we need to fetch the existing event
         # to apply the timezone correctly. This is a simplification; a full implementation
@@ -1123,111 +1073,3 @@ async def delete_event(
     confirmation_message = f"Successfully deleted event (ID: {event_id}) from calendar '{calendar_id}' for {user_google_email}."
     logger.info(f"Event deleted successfully for {user_google_email}. ID: {event_id}")
     return confirmation_message
-
-
-@server.tool()
-@handle_http_errors("query_freebusy", is_read_only=True, service_type="calendar")
-@require_google_service("calendar", "calendar_read")
-async def query_freebusy(
-    service,
-    user_google_email: str,
-    time_min: str,
-    time_max: str,
-    calendar_ids: Optional[List[str]] = None,
-    group_expansion_max: Optional[int] = None,
-    calendar_expansion_max: Optional[int] = None,
-) -> str:
-    """
-    Returns free/busy information for a set of calendars.
-
-    Args:
-        user_google_email (str): The user's Google email address. Required.
-        time_min (str): The start of the interval for the query in RFC3339 format (e.g., '2024-05-12T10:00:00Z' or '2024-05-12').
-        time_max (str): The end of the interval for the query in RFC3339 format (e.g., '2024-05-12T18:00:00Z' or '2024-05-12').
-        calendar_ids (Optional[List[str]]): List of calendar identifiers to query. If not provided, queries the primary calendar. Use 'primary' for the user's primary calendar or specific calendar IDs obtained from `list_calendars`.
-        group_expansion_max (Optional[int]): Maximum number of calendar identifiers to be provided for a single group. Optional. An error is returned for a group with more members than this value. Maximum value is 100.
-        calendar_expansion_max (Optional[int]): Maximum number of calendars for which FreeBusy information is to be provided. Optional. Maximum value is 50.
-
-    Returns:
-        str: A formatted response showing free/busy information for each requested calendar, including busy time periods.
-    """
-    logger.info(
-        f"[query_freebusy] Invoked. Email: '{user_google_email}', time_min: '{time_min}', time_max: '{time_max}'"
-    )
-
-    # Format time parameters
-    formatted_time_min = _correct_time_format_for_api(time_min, "time_min")
-    formatted_time_max = _correct_time_format_for_api(time_max, "time_max")
-
-    # Default to primary calendar if no calendar IDs provided
-    if not calendar_ids:
-        calendar_ids = ["primary"]
-
-    # Build the request body
-    request_body: Dict[str, Any] = {
-        "timeMin": formatted_time_min,
-        "timeMax": formatted_time_max,
-        "items": [{"id": cal_id} for cal_id in calendar_ids],
-    }
-
-    if group_expansion_max is not None:
-        request_body["groupExpansionMax"] = group_expansion_max
-    if calendar_expansion_max is not None:
-        request_body["calendarExpansionMax"] = calendar_expansion_max
-
-    logger.info(
-        f"[query_freebusy] Request body: timeMin={formatted_time_min}, timeMax={formatted_time_max}, calendars={calendar_ids}"
-    )
-
-    # Execute the freebusy query
-    freebusy_result = await asyncio.to_thread(
-        lambda: service.freebusy().query(body=request_body).execute()
-    )
-
-    # Parse the response
-    calendars = freebusy_result.get("calendars", {})
-    time_min_result = freebusy_result.get("timeMin", formatted_time_min)
-    time_max_result = freebusy_result.get("timeMax", formatted_time_max)
-
-    if not calendars:
-        return f"No free/busy information found for the requested calendars for {user_google_email}."
-
-    # Format the output
-    output_lines = [
-        f"Free/Busy information for {user_google_email}:",
-        f"Time range: {time_min_result} to {time_max_result}",
-        "",
-    ]
-
-    for cal_id, cal_data in calendars.items():
-        output_lines.append(f"Calendar: {cal_id}")
-
-        # Check for errors
-        errors = cal_data.get("errors", [])
-        if errors:
-            output_lines.append("  Errors:")
-            for error in errors:
-                domain = error.get("domain", "unknown")
-                reason = error.get("reason", "unknown")
-                output_lines.append(f"    - {domain}: {reason}")
-            output_lines.append("")
-            continue
-
-        # Get busy periods
-        busy_periods = cal_data.get("busy", [])
-        if not busy_periods:
-            output_lines.append("  Status: Free (no busy periods)")
-        else:
-            output_lines.append(f"  Busy periods: {len(busy_periods)}")
-            for period in busy_periods:
-                start = period.get("start", "Unknown")
-                end = period.get("end", "Unknown")
-                output_lines.append(f"    - {start} to {end}")
-
-        output_lines.append("")
-
-    result_text = "\n".join(output_lines)
-    logger.info(
-        f"[query_freebusy] Successfully retrieved free/busy information for {len(calendars)} calendar(s)"
-    )
-    return result_text

gsheets/sheets_tools.py

@@ -574,7 +574,7 @@ async def _format_sheet_range_impl(
     # Build confirmation message
     applied_parts = []
     if bg_color_parsed:
-        applied_parts.append(f"background {background_color}")
+        applied_parts.append(f"background color {background_color}")
     if text_color_parsed:
         applied_parts.append(f"text color {text_color}")
     if number_format:

tests/gsheets/test_format_sheet_range.py

@@ -338,8 +338,11 @@ async def test_format_invalid_wrap_strategy():
             wrap_strategy="INVALID",
         )
 
-    error_msg = str(exc_info.value).lower()
-    assert "wrap_strategy" in error_msg or "wrap" in error_msg
+    error_msg = str(exc_info.value)
+    assert "wrap_strategy" in error_msg
+    assert "CLIP" in error_msg
+    assert "OVERFLOW_CELL" in error_msg
+    assert "WRAP" in error_msg
 
 
 @pytest.mark.asyncio
@@ -357,8 +360,11 @@ async def test_format_invalid_horizontal_alignment():
             horizontal_alignment="INVALID",
         )
 
-    error_msg = str(exc_info.value).lower()
-    assert "horizontal" in error_msg or "left" in error_msg
+    error_msg = str(exc_info.value)
+    assert "horizontal_alignment" in error_msg
+    assert "CENTER" in error_msg
+    assert "LEFT" in error_msg
+    assert "RIGHT" in error_msg
 
 
 @pytest.mark.asyncio
@@ -376,8 +382,11 @@ async def test_format_invalid_vertical_alignment():
             vertical_alignment="INVALID",
         )
 
-    error_msg = str(exc_info.value).lower()
-    assert "vertical" in error_msg or "top" in error_msg
+    error_msg = str(exc_info.value)
+    assert "vertical_alignment" in error_msg
+    assert "BOTTOM" in error_msg
+    assert "MIDDLE" in error_msg
+    assert "TOP" in error_msg
 
 
 @pytest.mark.asyncio