feat(sheets,calendar): add Sheets write tools and fix Calendar updateEvent to use patch#326
feat(sheets,calendar): add Sheets write tools and fix Calendar updateEvent to use patch#326tuannvm wants to merge 6 commits into
Conversation
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Code Review
This pull request significantly expands the Google Workspace integration by adding comprehensive Google Sheets management capabilities and enhancing Google Calendar event handling. New Sheets tools include updating, appending, and clearing ranges, as well as creating spreadsheets and managing individual sheets. The Google Sheets API scope has been updated to allow write access. For Google Calendar, the service now supports adding Google Meet links and file attachments during event creation and updates. A critical improvement was identified regarding the use of the update method in the Calendar service, which performs a full resource replacement and may inadvertently delete existing event data; switching to a patch operation is recommended to ensure partial updates.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com> # Conflicts: # workspace-server/src/__tests__/services/CalendarService.test.ts # workspace-server/src/index.ts # workspace-server/src/services/CalendarService.ts # workspace-server/src/services/SheetsService.ts
…y default Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
@allenhutchison is this something we can consider now with custom scoping for consumers of the server being available? |
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
@allenhutchison i see that slides writes made it in awesome! what do we need to push this one through? getting slides and sheets writes through makes this server very robust and covers most of our users' use case requests |
allenhutchison
left a comment
There was a problem hiding this comment.
Review summary
The six Sheets write methods are clean and faithfully follow the existing SheetsService conventions (ID extraction, logging, result shape), the feature-gating is correct (off-by-default sheets.write), and the tests assert exact request shapes including valueInputOption and insertDataOption. Nice work. I've left inline notes on error-signalling and a couple of scope/coordination items; grouping the rest here.
Needs a maintainer decision first
This PR overlaps directly with open #342 on the sheets.write group and on append-tool naming (appendRange here vs appendRows there) — see the inline note on feature-config.ts. Worth resolving before merge so the two PRs don't fight.
Docs parity (please update before merge)
The repo enumerates every tool per feature group, but the 6 new write tools aren't listed:
docs/feature-configuration.md— has a### sheets.readsection (line 194) but no### sheets.write; the scope table (line 25) lists onlysheets | read. Add asheets.writerow + section listing the 6 tools.docs/index.md(lines 39-42) — lists only the 3 read tools; add the 6 write tools.
(Same parity expectation that came up on #342.)
Test coverage
Solid — every method has a success test with full request-shape assertions plus an error test. Three low-effort gaps worth closing:
- URL-vs-raw-ID extraction is never exercised (every test passes a raw id, so the
extractDocId(...)branch ofextractDocId(spreadsheetId) || spreadsheetIdis untested). One test passing a full Sheets URL and asserting the bare id reaches the API covers the shared pattern. appendRangewithupdatesundefined — asserts the?.path resolves without throwing.addSheetwith emptyreplies— asserts{ sheetId: undefined }rather than a throw.
Optional hardening
The Zod schemas validate types but not emptiness — .min(1) on range/title/values and sheetId → z.number().int().nonnegative() would turn late, opaque Google API errors into immediate validation errors. Not a blocker.
Strengths
- Methods mirror existing
SheetsServiceconventions exactly. valueInputOptionand the cell-value union are expressed identically in the TS types and Zod schemas — no drift.createSpreadsheet's optional-sheetTitlesbranches are both covered.
Review assisted by automated analysis agents; findings verified against the diff.
| logToFile( | ||
| `[SheetsService] Error during sheets.updateRange: ${errorMessage}`, | ||
| ); | ||
| return { |
There was a problem hiding this comment.
These six write methods return errors as a normal tool result whose only failure signal is an { error } body — none set isError: true. The MCP client treats the call as success unless it parses the JSON, so a failed write (permission denied, bad range, quota) can be reported to the user as if the mutation succeeded. DriveService (handleError, line 38) and DocsService already set isError: true; the new Sheets methods follow the older read-method convention instead. Recommend adding isError: true to all six catch blocks — ideally extracting a shared handleError like Drive's rather than repeating the block six times. (Same pattern at :295, :342, :397, :448, :499.)
| { | ||
| type: 'text' as const, | ||
| text: JSON.stringify({ | ||
| message: `Successfully deleted sheet ${sheetId}`, |
There was a problem hiding this comment.
deleteSheet discards the API response and always returns Successfully deleted sheet ${sheetId}. The Sheets batchUpdate API does throw on a genuinely invalid delete (unknown sheetId, deleting the last sheet), so the catch covers the common failures — but the success message is fabricated regardless of what the response actually contained. Combined with the missing isError (see the updateRange thread), any non-throwing edge case yields a confident false confirmation. At minimum, base the success claim on the call completing rather than asserting it unconditionally.
| type: 'text' as const, | ||
| text: JSON.stringify({ | ||
| updates: { | ||
| updatedRange: response.data.updates?.updatedRange, |
There was a problem hiding this comment.
response.data.updates?.updatedRange (and the three sibling fields) will silently produce an all-undefined payload if updates is absent — a "success" that reports zero identifiable cells written. addSheet (replies?.[0]?.addSheet) and createSpreadsheet have the same shape. In practice the API populates these on a real 200, so this is defensive hardening rather than a live bug, but consider returning isError with a "write may not have taken effect" message when the key field is missing, instead of an empty success.
| * Attachments are fully replaced (not appended) when provided. | ||
| */ | ||
| interface EventAttachment { | ||
| export interface EventAttachment { |
There was a problem hiding this comment.
Two things in this file look like out-of-scope churn for a Sheets-write PR:
export interface EventAttachment—EventAttachmentis only referenced insideCalendarService.ts(verified: no external importers), so the newexportis dead public surface.- The
updateParams→patchParamsrename (line 656/669) is purely cosmetic —mainalready callscalendar.events.patch(...), so this PR doesn't actually changeupdate→patchas the description claims.
Recommend dropping both so the diff stays scoped to the Sheets tools, and updating the PR description (the "fix Calendar updateEvent to use patch" headline is already true on main).
| scopes: scopes('spreadsheets'), | ||
| tools: [], | ||
| tools: [ | ||
| 'sheets.updateRange', |
There was a problem hiding this comment.
Heads-up on a collision: open PR #342 also populates this same sheets.write tools array — with sheets.appendRows, whereas this PR adds sheets.appendRange. So (a) whichever merges first will force a conflict on the other here, and (b) the two introduce overlapping append functionality under different names. This needs a maintainer decision to reconcile (pick one append tool / one naming) before either lands.
Summary
Fixes #253 — Adds write tools to the Google Sheets service and fixes the Calendar
updateEventmethod to useevents.patch(partial update) instead ofevents.update(full replacement).Sheets Write Tools
Six new tools:
sheets.updateRange,sheets.appendRange,sheets.clearRange,sheets.createSpreadsheet,sheets.addSheet, andsheets.deleteSheet. Upgrades the OAuth scope fromspreadsheets.readonlytospreadsheetswhen enabled.Calendar Fix
updateEventnow usesevents.patchinstead ofevents.update, preventing existing event fields (attendees, description, etc.) from being wiped when only updating specific fields.Use Case
Sheets — write data:
sheets.updateRangewithspreadsheetId,range: "Sheet1!A1:B2", andvalues: [["Name", "Score"], ["Alice", 95]]sheets.appendRangeto add rows after the last data rowSheets — manage structure:
sheets.createSpreadsheetwithtitleand optionalsheetTitlessheets.addSheet/sheets.deleteSheetto manage tabsCalendar — safe partial updates:
calendar.updateEventwitheventIdand only the fields to change — other fields are preservedEnabling Sheets Write Tools
Write tools are disabled by default (consistent with
slides.write,tasks.read,tasks.write). To enable:This adds the
spreadsheetsscope to the OAuth flow and registers all 6 write tools. See Feature Configuration for details.Changes
updateRange,appendRange,clearRange,createSpreadsheet,addSheet, anddeleteSheetmethodsupdateEventto useevents.patchinstead ofevents.updatefor partial update semanticsregisterToolhelpersheets.writefeature group with tool names (defaultEnabled: false)sheets.writeremains in the off-by-default groupevents.patchmockTesting
All 107 relevant tests pass (CalendarService, SheetsService, feature-config).