Skip to content

feat(sheets): add Sheets write tools#326

Open
tuannvm wants to merge 7 commits into
gemini-cli-extensions:mainfrom
tuannvm:feat/sheets-write-tools
Open

feat(sheets): add Sheets write tools#326
tuannvm wants to merge 7 commits into
gemini-cli-extensions:mainfrom
tuannvm:feat/sheets-write-tools

Conversation

@tuannvm
Copy link
Copy Markdown
Contributor

@tuannvm tuannvm commented Apr 7, 2026

Summary

Fixes #253 by adding six Google Sheets write tools behind the off-by-default sheets.write feature group.

Sheets Write Tools

Adds:

  • sheets.updateRange
  • sheets.appendRange
  • sheets.clearRange
  • sheets.createSpreadsheet
  • sheets.addSheet
  • sheets.deleteSheet

When sheets.write is enabled, the extension requests the spreadsheets OAuth scope and registers all six write tools.

Use Cases

Write data

  1. sheets.updateRange with spreadsheetId, range: "Sheet1!A1:B2", and values: [["Name", "Score"], ["Alice", 95]]
  2. sheets.appendRange to add rows after the last data row
  3. sheets.clearRange to clear a specific A1 range

Manage Sheets structure

  1. sheets.createSpreadsheet with title and optional sheetTitles
  2. sheets.addSheet and sheets.deleteSheet to manage tabs

Enabling Sheets Write Tools

Write tools are disabled by default, consistent with other non-published or experimental write scopes. To enable:

export WORKSPACE_FEATURE_OVERRIDES="sheets.write:on"

See Feature Configuration for details.

Changes

  • SheetsService.ts: Added write methods, shared write-error handling with isError: true, URL ID extraction, and non-overclaiming delete confirmation
  • index.ts: Registered six Sheets write tools with Zod input schemas
  • feature-config.ts: Populated the off-by-default sheets.write feature group
  • docs/feature-configuration.md and docs/index.md: Documented the new Sheets write tools
  • SheetsService.test.ts: Added success, option, error, URL extraction, and defensive response-shape coverage
  • CalendarService.ts: Removed out-of-scope public-surface/cosmetic churn from the PR diff

Testing

Local verification:

  • npm run lint
  • npm run format:check
  • npm run test:ci — 528 tests passed
  • npm run build

Open Maintainer Decision

This PR overlaps with #342 on the sheets.write group and append naming: sheets.appendRange here vs sheets.appendRows there. The final naming should be reconciled before merge.

tuannvm added 2 commits March 2, 2026 11:15
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread workspace-server/src/services/CalendarService.ts Outdated
tuannvm added 2 commits April 7, 2026 08:29
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>
@tuannvm tuannvm marked this pull request as ready for review April 7, 2026 15:52
@tuannvm tuannvm changed the title Feat/sheets write tools feat(sheets,calendar): add Sheets write tools and fix Calendar updateEvent to use patch Apr 7, 2026
@bdoyle0182
Copy link
Copy Markdown

@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>
@bdoyle0182
Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Contributor

@allenhutchison allenhutchison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.read section (line 194) but no ### sheets.write; the scope table (line 25) lists only sheets | read. Add a sheets.write row + 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 of extractDocId(spreadsheetId) || spreadsheetId is untested). One test passing a full Sheets URL and asserting the bare id reaches the API covers the shared pattern.
  • appendRange with updates undefined — asserts the ?. path resolves without throwing.
  • addSheet with empty replies — asserts { sheetId: undefined } rather than a throw.

Optional hardening

The Zod schemas validate types but not emptiness — .min(1) on range/title/values and sheetIdz.number().int().nonnegative() would turn late, opaque Google API errors into immediate validation errors. Not a blocker.

Strengths

  • Methods mirror existing SheetsService conventions exactly.
  • valueInputOption and the cell-value union are expressed identically in the TS types and Zod schemas — no drift.
  • createSpreadsheet's optional-sheetTitles branches are both covered.

Review assisted by automated analysis agents; findings verified against the diff.

logToFile(
`[SheetsService] Error during sheets.updateRange: ${errorMessage}`,
);
return {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things in this file look like out-of-scope churn for a Sheets-write PR:

  1. export interface EventAttachmentEventAttachment is only referenced inside CalendarService.ts (verified: no external importers), so the new export is dead public surface.
  2. The updateParamspatchParams rename (line 656/669) is purely cosmetic — main already calls calendar.events.patch(...), so this PR doesn't actually change updatepatch as 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',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@tuannvm tuannvm changed the title feat(sheets,calendar): add Sheets write tools and fix Calendar updateEvent to use patch feat(sheets): add Sheets write tools Jun 6, 2026
@tuannvm tuannvm requested a review from allenhutchison June 6, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth scope upgrade needed: spreadsheets.readonly → spreadsheets for write tools

5 participants