Add native Google Sheets tools (list, read, write, create, info)

[RafaelPo]

Summary

• 5 new MCP tools for Google Sheets: sheets_list, sheets_read, sheets_write, sheets_create, sheets_info
• Async Google Sheets/Drive API client with OAuth token management (stored in Redis from Supabase provider tokens)
• Google token passthrough in the OAuth flow so Sheets tools work after user login
• output_spreadsheet_title option on everyrow_results (both stdio and HTTP) to write results directly to a new Google Sheet
• Authenticated Google URL fetches in fetch_csv_from_url for private sheets
• Sheets tools removed in stdio mode (require Google OAuth, HTTP mode only)

Test plan

• Model validation tests for spreadsheet ID extraction, input models (7 classes)
• Converter tests for values_to_records and records_to_values
• Client tests with mocked httpx for all Sheets API operations
• Tool integration tests with mocked token + httpx for all 5 tools
• E2E MCP protocol test verifying 15 registered tools
• Schema tests verifying output_spreadsheet_title in both ResultsInput models
• All 259 tests pass, pre-commit hooks (format, lint, typecheck) pass

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 52s —— View job

---

Code Review: Add native Google Sheets tools

• Read CLAUDE.md for repo conventions
• Review sheets_client.py (new — 284 lines)
• Review sheets_models.py (new — 134 lines)
• Review sheets_tools.py (new — 251 lines)
• Review changes to tools.py, utils.py, auth.py, models.py, server.py
• Review test files
• Review manifest changes
• Post review summary

---

Overall this is a well-structured PR. The separation into sheets_client.py / sheets_models.py / sheets_tools.py is clean, the OAuth token passthrough through the existing auth flow is solid, and the test coverage is thorough (model validation, converters, client mocks, tool integration, E2E). A few items worth addressing:

---

Issues

1. No error handling in sheets tools — API errors surface as raw exceptions
sheets_tools.py — All 5 tool functions call the Google API without catching httpx.HTTPStatusError. A 403 (no permission), 404 (spreadsheet not found), or 429 (rate limit) will propagate as an unhandled exception with a raw traceback rather than a user-friendly MCP error message. The other everyrow tools in tools.py all wrap their API calls in try/except and return clean TextContent errors.

Fix this →

2. range parameter shadows Python builtin
sheets_client.py:150, :165, :180 — The parameter name range shadows the Python builtin. This won't cause bugs today, but it's a code quality issue that linters typically flag. Consider renaming to cell_range or a1_range.

Fix this →

3. Accessing private _tools dict to remove tools in stdio mode
server.py:127 — mcp._tool_manager._tools.pop(name, None) reaches into the MCP SDK's private internals. If the SDK changes this internal structure, the stdio tool removal will break silently (tools would just remain registered). This is the same pattern used on line 82 (remove_tool) which appears to be a public API — consider using remove_tool consistently here too.

# Current (fragile)
mcp._tool_manager._tools.pop(name, None)

# Suggested (uses the public API like line 82)
mcp._tool_manager.remove_tool(name)

Fix this →

4. Single-tenant token key "current" in multi-user HTTP mode
sheets_client.py:45 — get_google_token() uses the hardcoded key build_key("google_token", "current"), and store_google_token in auth.py:448 also stores with "current". In HTTP mode with multiple concurrent users, the last user to authenticate overwrites the token for everyone. If multi-tenancy is planned, this will need per-session or per-user keying. If single-tenant is intentional, a brief comment would help future readers.

---

Nits (non-blocking)

• sheets_client.py:228 — The single-quote escaping query.replace("'", "\\'") is correct for the Drive API query syntax but the backslash-escaped quote may not be necessary — the Drive API also accepts '' (doubled single quotes) as an escape. Either works.

• test_sheets_tools.py — The tool integration tests mock get_google_token at the sheets_tools import path, which is correct. The tests are well-organized with clear separation between model, converter, client, and integration layers.

• manifest.json — The 5 new sheets tools are correctly listed. The manifest doesn't currently distinguish HTTP-only tools, which is fine since it's a static registry.

---

Summary

The architecture is sound — clean layering with sheets_client for API calls, sheets_models for validation, and sheets_tools for MCP registration. The OAuth integration reuses the existing Supabase flow well. The main actionable items are adding error handling in the tool functions (#1) and using the public remove_tool API (#3). The builtin shadowing (#2) and token keying (#4) are lower priority.

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 46s —— View job

---

Code Review: Add native Google Sheets tools

• Read CLAUDE.md for repo conventions
• Review sheets_client.py (new — 305 lines)
• Review sheets_models.py (new — 160 lines)
• Review sheets_tools.py (new — 357 lines)
• Review changes to server.py, auth.py, config.py, models.py, tools.py, utils.py
• Review test files
• Post review summary

---

This is a well-structured PR since the last review. The issues from the previous review have been largely addressed: error handling is now in all tool functions, range → cell_range rename in the client methods, and the token keying uses per-user IDs. One confirmed bug remains from the Sentry review, plus a few new items:

---

Issues

1. Bug (confirmed): None values become literal "None" strings in Google Sheets
sheets_client.py:304 — The Sentry review correctly identified this. str(record.get(h, "")) only uses the default "" when the key is missing from the dict. When the key exists with value None, str(None) produces the string "None".

This affects two code paths:

• Direct sheets_write/sheets_create when input data contains None values
• _write_results_to_sheet in tools.py:138 which explicitly converts NaN → None via df.where(pd.notna(df), None)

# Current (line 304)
rows.append([str(record.get(h, "")) for h in headers])

# Fix
rows.append([str(v) if v is not None else "" for v in (record.get(h, "") for h in headers)])
# Or more readable:
rows.append(["" if record.get(h) is None else str(record.get(h, "")) for h in headers])

Fix this →

2. _write_results_to_sheet bypasses sheets rate limiter
tools.py:101-159 — The _write_results_to_sheet function calls the Sheets API directly (create + write) without going through _check_sheets_rate_limit(). All 5 tool functions in sheets_tools.py enforce rate limits, but this code path in the results tool doesn't. A user could trigger unbounded Sheets API calls by repeatedly calling everyrow_results with output_spreadsheet_title. The duplicate-name guard mitigates this somewhat, but doesn't prevent repeated attempts with different titles.

3. google_sheets_credentials_json config field is unused
config.py:135-139 — The google_sheets_credentials_json field is defined with a description saying "Required for Google Sheets tools in stdio mode", but it's never read anywhere in the codebase. The PR description says sheets tools are HTTP-only, and server.py:84 confirms they're only registered when settings.is_http. This dead config field could confuse users into thinking stdio-mode sheets is supported.

Fix this →

---

Nits (non-blocking)

• sheets_models.py:59,87 — The model field name range still shadows the Python builtin at the class attribute level. This is fine for Pydantic models (it's the user-facing MCP schema name), and the client methods were correctly renamed to cell_range. Just noting for awareness — no change needed.

• sheets_tools.py:282 — write_range is called with a bare string "Sheet1" rather than using the cell_range keyword argument. Consistent kwarg usage would be clearer: await client.write_range(spreadsheet_id, cell_range="Sheet1", values=values).

• Test coverage is solid: model validation, converters, client mocks, tool integration, E2E, range validation, error sanitization, and Drive query injection. The autouse _no_rate_limit fixture cleanly disables rate limiting across all tool tests.

---

Previously flagged items — status

#	Issue	Status
1	Error handling in tools	Fixed — all 5 tools have try/except with _error_message()
2	range builtin shadowing in client	Fixed — renamed to cell_range in sheets_client.py
3	Private _tools.pop() API	Fixed — server.py now uses remove_tool() consistently
4	Single-tenant token key "current"	Fixed — now uses per-user keying via build_key("google_token", user_id)

---

Summary

The main actionable item is the None → "None" bug (#1), which is a real data integrity issue in the Sheets export path. The rate limit bypass (#2) is worth considering but lower priority. The unused config field (#3) is a quick cleanup.

[sentry]

Bug: In _issue_token_response, a failure to store the Google token is silently ignored. This allows authentication to succeed but causes subsequent calls to Sheets tools to fail.
_{Severity: MEDIUM}

Suggested Fix

The authentication flow should fail if the Google token cannot be stored. Modify _issue_token_response to re-raise the exception or return an error instead of just logging a warning. This ensures the user is immediately aware of the problem and prevents them from entering a state where they are authenticated but the associated functionality is broken. Alternatively, provide a more accurate error message at the tool level that distinguishes between a missing token and a storage failure.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: everyrow-mcp/src/everyrow_mcp/auth.py#L472-L484

Potential issue: During the OAuth flow in `_issue_token_response`, if
`store_google_token` fails (e.g., due to a temporary Redis issue), the exception is
caught and only a warning is logged. The authentication process continues and returns a
valid token to the user. However, when the user later attempts to use a Google Sheets
tool, the application will fail to retrieve the token, raising a `RuntimeError`. This
results in a confusing user experience where the user appears authenticated but cannot
use the related features, and receives a misleading error message suggesting they need
to log in again.

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 49s —— View job

---

Code Review: Add native Google Sheets tools

• Read CLAUDE.md for repo conventions
• Review PR diff against origin/main
• Review sheets_client.py (new — 310 lines)
• Review sheets_models.py (new — 165 lines)
• Review sheets_tools.py (new — 389 lines)
• Review changes to server.py, auth.py, config.py, models.py, tools.py, utils.py
• Review test files
• Post review summary

---

Good progress since the last two reviews. The None → "None" bug is fixed, error handling is in place on all tools, the private _tools.pop() API is replaced, and token keying is per-user. Two previously flagged items remain, plus one new observation:

---

Issues

1. Silent failure of Google token storage allows broken auth state
auth.py:489-490 — When store_google_token fails in _issue_token_response, the exception is caught and only a warning is logged, but authentication completes successfully. The user then appears logged in but every Sheets tool call will fail with "No Google token available. The user must log in…" — a confusing UX.

This is the same issue the Sentry review flagged. Since store_google_token now properly re-raises (sheets_client.py:127,143), the catch here swallows it. Consider either: (a) letting the exception propagate to fail the token exchange, or (b) returning a degraded token response that signals Sheets tools won't work, or at minimum (c) upgrading to logger.error and adding a comment explaining why this is intentionally non-fatal.

# auth.py:489 — current
except Exception:
    logger.warning("Could not store Google token during token issue")

# Option A: fail the token exchange (recommended if Sheets is critical)
# Just remove the try/except — store_google_token already raises

# Option B: at minimum, make the failure auditable
except Exception:
    logger.error(
        "Google token storage failed for user=%s — Sheets tools will be unavailable",
        jwt_claims.get("sub", "unknown"),
        exc_info=True,
    )

Fix this →

2. google_sheets_credentials_json config field is unused (carried over from review #2)
config.py:139-143 — This field is defined with a description saying "Required for Google Sheets tools in stdio mode", but it's never referenced anywhere in the codebase. Since sheets tools are HTTP-only (server.py:84), this is dead config that could mislead users.

Fix this →

3. _write_results_to_sheet bypasses sheets rate limiter (carried over from review #2)
tools.py:110-168 — This function calls the Sheets API (list + create + write) without going through _check_sheets_rate_limit(). All 5 tool functions in sheets_tools.py enforce rate limits, but the everyrow_results → output_spreadsheet_title code path doesn't. The duplicate-name guard limits repeat-with-same-title, but different titles can be used indefinitely.

A simple fix: call _check_sheets_rate_limit() at the top of _write_results_to_sheet, or in everyrow_results_http before calling it.

Fix this →

---

Previously flagged items — status

#	Issue	Status
1	Error handling in tools	Fixed — all 5 tools have try/except with _error_message()
2	range builtin shadowing in client	Fixed — renamed to cell_range in sheets_client.py
3	Private _tools.pop() API	Fixed — server.py now uses remove_tool() consistently
4	Single-tenant token key "current"	Fixed — now uses per-user keying via build_key("google_token", user_id)
5	None → "None" bug	Fixed — records_to_values now checks v is not None (line 306) with test coverage
6	Unused google_sheets_credentials_json	Open — still present in config.py:139
7	_write_results_to_sheet bypasses rate limiter	Open — still no rate limit check in tools.py:110
8	Silent Google token storage failure (Sentry)	Open — auth.py:489 still swallows the exception

---

Summary

The architecture is clean and the major bugs from previous reviews are resolved. Three items remain open: the silent token storage failure (#1, medium — UX issue), the unused config field (#2, low — cleanup), and the rate limit bypass (#3, low — defence-in-depth). Test coverage is thorough across all layers.
|