feat(sheets): add Sheets write tools#326
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.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
@allenhutchison I think we should get this one in this week and then cut a release as we'll have both sheets and slides write tools added in |
matheusrmorgado
left a comment
There was a problem hiding this comment.
Hi @tuannvm, nice work on this PR. The exact request-shape assertions in the tests and the off-by-default feature gating make this easy to trust, and the centralized handleError with isError: true is a clean response to the earlier review round.
We've been building and battle-testing similar Sheets write tooling for LLM agents internally, and we hit a few edge cases on exactly these API paths. Sharing them in case they're useful. None of these are blocking, and most are one-line description tweaks:
1. appendRange: consider exposing insertDataOption
It's currently hardcoded to INSERT_ROWS. The other mode, OVERWRITE, fills the rows below the detected table without inserting new rows, so it doesn't shift down unrelated content that lives below the table. Both modes are useful in practice: template-filling flows usually want OVERWRITE, while log-style appends want INSERT_ROWS. An optional enum defaulting to INSERT_ROWS would keep current behavior with no breaking change.
2. USER_ENTERED coercion is a real trap for agent-written data
Defaulting to USER_ENTERED is the right call, but agents corrupt data with it more often than you'd expect: "03/04" becomes a date, "0123" loses its leading zero, "TRUE" becomes a boolean. Adding one sentence to the valueInputOption description ("strings that look like dates, numbers, or booleans will be converted; use RAW to store text verbatim") measurably reduced these incidents for us, since models do read tool descriptions.
3. A1 quoting rules in the range descriptions
Sheet names containing spaces or special characters must be single-quoted, and literal apostrophes doubled: 'It''s data'!A1:B2. Models get this wrong surprisingly often. A short example in the range description helped a lot on our side.
4. appendRange table-detection semantics are worth a sentence
values.append appends after the last row of the contiguous data block it detects within range, which is not necessarily the last used row of the sheet. With disjoint data blocks the write can land somewhere unexpected. A brief note in the tool description helps agents anchor the range to the intended table.
5. deleteSheet.sheetId: consider z.number().int().nonnegative()
A float currently passes the schema and only fails at the API with a less actionable message.
Happy to elaborate on any of these. Thanks for pushing Sheets write support forward, we're looking forward to using it.
Summary
Fixes #253 by adding six Google Sheets write tools behind the off-by-default
sheets.writefeature group.Sheets Write Tools
Adds:
sheets.updateRangesheets.appendRangesheets.clearRangesheets.createSpreadsheetsheets.addSheetsheets.deleteSheetWhen
sheets.writeis enabled, the extension requests thespreadsheetsOAuth scope and registers all six write tools.Use Cases
Write data
sheets.updateRangewithspreadsheetId,range: "Sheet1!A1:B2", andvalues: [["Name", "Score"], ["Alice", 95]]sheets.appendRangeto add rows after the last data rowsheets.clearRangeto clear a specific A1 rangeManage Sheets structure
sheets.createSpreadsheetwithtitleand optionalsheetTitlessheets.addSheetandsheets.deleteSheetto manage tabsEnabling Sheets Write Tools
Write tools are disabled by default, consistent with other non-published or experimental write scopes. To enable:
See Feature Configuration for details.
Changes
isError: true, URL ID extraction, and non-overclaiming delete confirmationsheets.writefeature groupTesting
Local verification:
npm run lintnpm run format:checknpm run test:ci— 528 tests passednpm run buildOpen Maintainer Decision
This PR overlaps with #342 on the
sheets.writegroup and append naming:sheets.appendRangehere vssheets.appendRowsthere. The final naming should be reconciled before merge.