GoogleSheetsAgent - googleSheetsLoginTool#1380
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExtracts Google Sheets tool retrieval into getGoogleSheetsTools(accountId), adds authentication-aware logic that returns toolkit tools when a connected account is ACTIVE or a Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Google Sheets Agent
participant Tools as getGoogleSheetsTools(accountId)
participant Composio as Composio Client
participant Accounts as getConnectedAccount(accountId)
participant Auth as authenticateGoogleSheetsToolkit(userId)
participant Login as googleSheetsLoginTool
Agent->>Tools: request tools(accountId)
Tools->>Composio: init client
Tools->>Accounts: listConnectedAccounts(accountId, GOOGLE_SHEETS_TOOLKIT_SLUG)
rect `#EEF8F0`
Note over Accounts,Composio: inspect connected account(s) and status
Accounts->>Composio: listConnectedAccounts(...)
end
alt found and ACTIVE
Accounts-->>Tools: active account
Tools->>Composio: fetch toolkit tools
Tools-->>Agent: return toolkit tools (isAuthenticated=true)
Agent->>Agent: prepareStep: first step toolChoice="required" (others unchanged), stopWhen=111
else none or not ACTIVE
Accounts->>Auth: initiate auth(userId)
Auth->>Composio: connectedAccounts.initiate(userId, configId)
Auth-->>Accounts: connection request
Accounts->>Composio: retry listConnectedAccounts(...)
alt became ACTIVE
Tools-->>Agent: return toolkit tools (isAuthenticated=true)
Agent->>Agent: prepareStep: first step toolChoice="required" (others unchanged), stopWhen=111
else still unauthenticated
Tools-->>Agent: return { googleSheetsLoginTool } (isAuthenticated=false)
Agent->>Agent: prepareStep: first step toolChoice="required" (others unchanged), stopWhen=111
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Inline comments on changed lines:
- lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts:6 — 🚨 Critical: Throwing at import time will crash the app if the env var is missing; move this check inside the function and fail gracefully.
- lib/composio/googleSheets/getConnectedAccount.ts:13 —
⚠️ Logic: Auto-initiating auth in a getter has side effects; return a not-connected status and let a dedicated tool start auth. - lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts:21 —
⚠️ Logic: ‘toolChoice: "required"’ may force unnecessary tool calls; consider ‘auto’ to keep responses simple when no tool is needed. - lib/tools/composio/googleSheetsLoginTool.ts:19 — 🔒 Security: Don’t return the full connected-accounts object to the model; return a minimal status (e.g., pending/active and URL) to avoid leaking sensitive fields.
- lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts:13 —
⚠️ Logic: Using the first account via items[0] is ambiguous; handle multiple accounts or select the active one deterministically. - package.json:23–24 — 🚨 Critical: Upgrading to @composio/core 0.2.x/@composio/vercel 0.2.x may include breaking API changes; verify compatibility for connectedAccounts.* and tools.get and cover with tests.
Summary:
- Focused changes add Composio Google Sheets auth and tool gating. Biggest risk is a top-level env check that will crash on import; second is side-effectful auth in a getter and potential data leakage from the login tool. Please address the above to keep behavior safe and predictable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts (2)
12-12: Remove unnecessaryawaiton synchronous function.Based on the code snippet from
lib/composio/client.ts,getComposioClient()returns aComposioinstance synchronously, so theawaitis unnecessary.Apply this diff:
- const composio = await getComposioClient(); + const composio = getComposioClient();
11-18: Add return type annotation and consider error handling for the external API call.The function lacks a return type annotation, which reduces type safety. Additionally, the
connectedAccounts.initiate()call could fail, but there's no explicit error handling or documentation about what errors might be thrown.Consider adding a return type and documenting potential errors:
+/** + * Initiates authentication flow for Google Sheets toolkit. + * @param userId - The user ID to authenticate + * @returns Connection request object + * @throws Error if authentication initiation fails + */ -async function authenticateGoogleSheetsToolkit(userId: string) { +async function authenticateGoogleSheetsToolkit( + userId: string +): Promise<Awaited<ReturnType<typeof composio.connectedAccounts.initiate>>> {Alternatively, if the Composio SDK provides specific types for the connection request, use those instead.
lib/composio/googleSheets/getConnectedAccount.ts (2)
7-7: Remove unnecessaryawaiton synchronous function.Based on the code snippet from
lib/composio/client.ts,getComposioClient()returns aComposioinstance synchronously.Apply this diff:
- const composio = await getComposioClient(); + const composio = getComposioClient();
6-22: Add return type annotation for type safety.The function lacks a return type annotation. Based on the Composio SDK usage, this should return the type from
connectedAccounts.list().Add type annotation:
+/** + * Retrieves connected Google Sheets accounts for a user. + * Initiates authentication if no accounts found. + * @param accountId - The user's account ID + * @returns Connected accounts list + */ -export default async function getConnectedAccount(accountId: string) { +export default async function getConnectedAccount( + accountId: string +): Promise<Awaited<ReturnType<typeof composio.connectedAccounts.list>>> {lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts (1)
14-15: Consider cleaner formatting for multi-line instructions.The instructions formatting could be improved for consistency.
- const instructions = `You are a Google Sheets agent. - account_id: ${accountId}`; + const instructions = `You are a Google Sheets agent. +account_id: ${accountId}`;Or use array join for clearer multi-line text:
- const instructions = `You are a Google Sheets agent. - account_id: ${accountId}`; + const instructions = [ + "You are a Google Sheets agent.", + `account_id: ${accountId}` + ].join("\n");lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts (3)
11-11: Remove unnecessaryawaiton synchronous function.Based on the code snippet from
lib/composio/client.ts,getComposioClient()returns synchronously.Apply this diff:
- const composio = await getComposioClient(); + const composio = getComposioClient();
13-13: Consider more explicit authentication status checking.The optional chaining silently treats missing or malformed account data as unauthenticated. While this provides a safe fallback, it might mask data structure issues.
Consider adding explicit checks or logging:
- const isAuthenticated = userAccounts.items[0]?.data?.status === "ACTIVE"; + const firstAccount = userAccounts.items[0]; + const isAuthenticated = + firstAccount?.data?.status === "ACTIVE"; + + // Optional: Add logging for debugging + if (!firstAccount) { + console.debug("No connected accounts found for user"); + } else if (firstAccount.data?.status !== "ACTIVE") { + console.debug(`Account status: ${firstAccount.data?.status || "unknown"}`); + }
8-24: Optimize client initialization to occur only when needed.The Composio client is initialized before checking authentication, but it's only used when the user is authenticated. Consider lazy initialization to avoid unnecessary client creation.
export default async function getGoogleSheetsTools( accountId: string ): Promise<ToolSet> { - const composio = getComposioClient(); const userAccounts = await getConnectedAccount(accountId); const isAuthenticated = userAccounts.items[0]?.data?.status === "ACTIVE"; - const tools = isAuthenticated - ? await composio.tools.get(accountId, { - toolkits: [GOOGLE_SHEETS_TOOLKIT_SLUG], - }) - : { - googleSheetsLoginTool, - }; + if (!isAuthenticated) { + return { + googleSheetsLoginTool, + }; + } + + const composio = getComposioClient(); + const tools = await composio.tools.get(accountId, { + toolkits: [GOOGLE_SHEETS_TOOLKIT_SLUG], + }); return tools; }This is a minor optimization and the current code is perfectly readable, so this is optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.env.exampleis excluded by none and included by nonepackage.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (5)
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts(1 hunks)lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts(1 hunks)lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts(1 hunks)lib/composio/googleSheets/getConnectedAccount.ts(1 hunks)lib/tools/composio/googleSheetsLoginTool.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.ts
⚙️ CodeRabbit configuration file
lib/**/*.ts: For utility functions, ensure:
- Pure functions when possible
- Single responsibility per function
- Proper error handling
- Use TypeScript for type safety
- Avoid side effects
- Keep functions under 50 lines
- DRY: Consolidate similar logic into shared utilities
- KISS: Prefer simple, readable implementations over clever optimizations
Files:
lib/composio/googleSheets/getConnectedAccount.tslib/composio/googleSheets/authenticateGoogleSheetsToolkit.tslib/agents/googleSheetsAgent/getGoogleSheetsTools.tslib/tools/composio/googleSheetsLoginTool.tslib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts
🧬 Code graph analysis (5)
lib/composio/googleSheets/getConnectedAccount.ts (1)
lib/composio/client.ts (1)
getComposioClient(11-16)
lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts (1)
lib/composio/client.ts (1)
getComposioClient(11-16)
lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts (2)
lib/composio/client.ts (1)
getComposioClient(11-16)lib/composio/googleSheets/getConnectedAccount.ts (2)
getConnectedAccount(6-22)GOOGLE_SHEETS_TOOLKIT_SLUG(4-4)
lib/tools/composio/googleSheetsLoginTool.ts (1)
lib/composio/googleSheets/getConnectedAccount.ts (1)
getConnectedAccount(6-22)
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts (3)
lib/chat/types.ts (2)
ChatRequest(14-25)RoutingDecision(27-35)lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts (1)
getGoogleSheetsTools(8-24)lib/consts.ts (1)
DEFAULT_MODEL(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vercel Agent Review
- GitHub Check: code-review
🔇 Additional comments (1)
lib/tools/composio/googleSheetsLoginTool.ts (1)
18-20: Review comment is incorrect - no changes needed.The tool correctly initiates authentication (via
getConnectedAccount) and returns the connection status object, which downstream code depends on. IngetGoogleSheetsTools.ts, the code directly accessesuserAccounts.items[0]?.data?.statusto determine authentication state. Converting this return to a success/error message string would break this dependency.The tool's description and implementation are aligned: it initiates the authentication flow when no connected accounts exist, then returns the updated account status. The raw API response structure is intentional and necessary for consumers to verify the authentication result.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Review
Below are high-severity findings on changed lines. One issue per item; actionable and concise.
-
🚨 Critical —
lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts:5-9
Throwing at module import ifCOMPOSIO_GOOGLE_SHEETS_AUTH_CONFIG_IDis unset will crash builds/runtimes; move this check insideauthenticateGoogleSheetsToolkit()and fail at call time instead. -
⚠️ Logic —lib/composio/googleSheets/getConnectedAccount.ts:13
Only re-initiating when there are 0 accounts misses cases where an account exists but is notACTIVE(e.g.,PENDING); handle non-ACTIVE statuses by (re)initiating auth or returning a next-step URL. -
🔒 Security —
lib/tools/composio/googleSheetsLoginTool.ts:19
Returning the full connected account payload to the model may expose sensitive metadata; return a minimal status + approval URL or opaque handle instead. -
⚠️ Logic —lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts:15
stepCountIs(... : 2)may stop the loop before the login tool roundtrip completes; consider a higher cap (e.g., 4–6) to allow tool execution + follow-up. -
⚠️ Logic —.env.example:27
This variable appears required by code and currently triggers a top-level throw when missing; add a comment that it’s required and ensure environments set it (also add final newline). -
⚠️ Logic —lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts:13
Selectingitems[0]risks ignoring anotherACTIVEaccount; prefer finding the first item withstatus === "ACTIVE".
Summary: Focus fixes on avoiding top-level crashes from env checks, robust handling of non-ACTIVE accounts, minimizing sensitive data returned to the model, and ensuring the agent has enough steps to complete login.
There was a problem hiding this comment.
Inline notes on changed lines:\n\n- lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts:21 —
There was a problem hiding this comment.
Inline notes on changed lines:
- lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts:21 —
⚠️ Logic: stepCountIs(...) returns a predicate, so this condition is always truthy;toolChoicewill always be 'required'. Derive a real boolean (e.g., from authentication). - lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts:6 — 🚨 Critical: Throwing at module load will crash any import when the env var is missing. Move this check inside the function and handle it gracefully.
Summary:
- 🔒 Ensure env handling is lazy and fail-soft.
- ✨ After Composio bumps, verify compatibility and run integration tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts (1)
21-21: Authentication flow remains architecturally broken despite dynamic toolChoice.The past review correctly identified a critical issue that this change doesn't resolve. While the dynamic
toolChoiceforces the login tool to be called on step 1, the fundamental problem persists:
Static tool set: Tools are resolved once at line 11 and never refreshed during agent execution
Broken flow for unauthenticated users:
- Step 1:
toolChoice: "required"forcesgoogleSheetsLoginToolcall- Login tool initiates async OAuth via
authenticateGoogleSheetsToolkit- Tool returns immediately with connection request (before user completes OAuth)
- Step 2+:
toolChoice: "auto"but tools are still{ googleSheetsLoginTool }- Agent cannot access real Google Sheets tools even after authentication completes
No completion mechanism: The OAuth flow happens outside the agent loop without any way to signal completion or refresh tools
Required architectural changes:
- Return the OAuth URL/connection request to the user immediately rather than wrapping it in an agent tool
- After user completes OAuth externally, re-invoke
getGoogleSheetsAgentwith refreshed authentication state- Alternatively, implement a mechanism to dynamically refresh tools mid-loop (requires changes to ToolLoopAgent or a polling pattern)
- Add early termination when authentication is required: set
stopWhento exit after login initiation
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.ts
⚙️ CodeRabbit configuration file
lib/**/*.ts: For utility functions, ensure:
- Pure functions when possible
- Single responsibility per function
- Proper error handling
- Use TypeScript for type safety
- Avoid side effects
- Keep functions under 50 lines
- DRY: Consolidate similar logic into shared utilities
- KISS: Prefer simple, readable implementations over clever optimizations
Files:
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts
🧬 Code graph analysis (1)
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts (3)
lib/chat/types.ts (2)
ChatRequest(14-25)RoutingDecision(27-35)lib/agents/googleSheetsAgent/getGoogleSheetsTools.ts (1)
getGoogleSheetsTools(8-24)lib/consts.ts (1)
DEFAULT_MODEL(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vercel Agent Review
- GitHub Check: code-review
🔇 Additional comments (2)
lib/agents/googleSheetsAgent/getGoogleSheetsAgent.ts (2)
4-4: LGTM: Clean extraction of tool retrieval logic.Extracting tool retrieval into a dedicated function improves separation of concerns and testability.
14-15: Clarify the purpose of embeddingaccount_idin instructions.The
account_idis already passed programmatically togetGoogleSheetsToolsand available to tools. Including it as text in the LLM instructions is unusual unless the model needs to reference it in responses to the user.Is there a specific reason the LLM needs to see the
account_idin its instructions? If not, consider removing this line to avoid redundancy and potential confusion.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
…stopWhen conditional.
There was a problem hiding this comment.
🚨 High-severity findings (see file/line refs):
- 🚨 lib/composio/googleSheets/authenticateGoogleSheetsToolkit.ts:7: Throwing at module load will crash the app if the env is unset; move this check into the function and return a clear error instead.
⚠️ lib/composio/googleSheets/getConnectedAccount.ts:15: Initiating auth on every call when no accounts exist can spam requests; return a login tool and trigger auth explicitly, or add dedupe/backoff.
✅ No prior issues on this PR appear outstanding after recent changes.
1 participant
Summary by CodeRabbit