A2a integrate #58
A2a integrate #58
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @shikharbsar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've integrated the Agent-to-Agent (A2A) SDK into our project, enabling the creation and exposure of a ThoughtSpot agent. This agent is designed to process user queries, including a 'ping' functionality to verify ThoughtSpot connectivity and authentication, and will eventually support more complex data queries.
Highlights
- A2A SDK Dependency: I've added the @a2a-js/sdk to our project dependencies, laying the groundwork for agent-to-agent communication.
- ThoughtSpot Agent Definition: I've defined a ThoughtSpot AgentCard that describes the agent's capabilities, security requirements (OAuth2.0), and available skills like "ping" and "get-answer-from-thoughtspot", making it discoverable by other A2A-compatible systems.
- Agent Execution Logic: I've implemented the core logic for the ThoughtSpot Agent in agent-executor.ts, including handling incoming requests, managing task states, and providing a "ping" skill that checks the agent's connection and authentication status with ThoughtSpot.
- Agent Card Endpoint: I've exposed the ThoughtSpot AgentCard via a new /agent-card GET endpoint, allowing external systems to retrieve the agent's metadata.
- A2A Request Routing: I've configured the application to route A2A-specific requests to the new agent executor via the /a2a path, establishing the communication channel for agent interactions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request integrates the A2A SDK by adding a new agent card, an agent executor, and related handlers. The changes look good overall, but there are a few areas for improvement. I've identified some hardcoded URLs that should be moved to environment variables, suggested improvements for type safety by removing as any casts, and pointed out a potential bug in error handling. I've also included suggestions for refactoring and using more standard APIs for better maintainability and security.
| name: "ThoughtSpot Agent", | ||
| description: | ||
| "An agent that can answer users data queries from ThoughtSpot.", | ||
| url: "http://myserver.com:8787/a2a", |
There was a problem hiding this comment.
Hardcoded URLs like http://myserver.com:8787 are used here and for authorizationUrl and tokenUrl in this file. These should be configured via environment variables to support different deployment environments (dev, staging, prod) without requiring code changes. This improves both security and maintainability.
| controller.close(); | ||
| } catch (error) { | ||
| // Handle errors by enqueueing error event and closing | ||
| const errorEvent = `id: ${new Date().getTime()}\nevent: error\ndata: ${JSON.stringify(error)}\n\n`; |
There was a problem hiding this comment.
JSON.stringify(error) for a standard Error object will produce an empty JSON object '{}', which isn't useful for debugging on the client. You should explicitly serialize properties like name and message to provide meaningful error information.
| const errorEvent = `id: ${new Date().getTime()}\nevent: error\ndata: ${JSON.stringify(error)}\n\n`; | |
| const errorEvent = `id: ${new Date().getTime()}\nevent: error\ndata: ${JSON.stringify({ name: error instanceof Error ? error.name : 'Error', message: error instanceof Error ? error.message : String(error) })}\n\n`; |
| private generateUuid(): string { | ||
| return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => { | ||
| const r = Math.random() * 16 | 0; | ||
| const v = c === 'x' ? r : (r & 0x3 | 0x8); | ||
| return v.toString(16); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This custom generateUuid implementation is not cryptographically secure and may not guarantee uniqueness as effectively as standard methods. The Cloudflare Workers environment provides crypto.randomUUID(), which should be used instead to generate standard and secure v4 UUIDs. You can replace the body of this function with return crypto.randomUUID();.
| private async handlePingRequest(taskId: string, contextId: string, eventBus: ExecutionEventBus): Promise<void> { | ||
| let pingResult: string; | ||
| let authStatus: string; | ||
|
|
||
| if (this.thoughtSpotProps?.accessToken && this.thoughtSpotProps?.instanceUrl) { | ||
| try { | ||
| // Try to make an authenticated call to verify connection | ||
| const thoughtSpotService = this.getThoughtSpotService(this.thoughtSpotProps); | ||
| const sessionInfo = await thoughtSpotService.getSessionInfo(); | ||
|
|
||
| authStatus = "✅ Authenticated"; | ||
| pingResult = `Pong! 🏓 | ||
| Authentication Status: ${authStatus} | ||
| ThoughtSpot Instance: ${this.thoughtSpotProps.instanceUrl} | ||
| Connected User: ${sessionInfo?.userName || 'Unknown'} | ||
| User GUID: ${sessionInfo?.userGUID || 'N/A'} | ||
| Cluster: ${sessionInfo?.clusterName || 'N/A'} | ||
| Connection test successful!`; | ||
| } catch (error) { | ||
| authStatus = "❌ Authentication Failed"; | ||
| pingResult = `Pong! 🏓 | ||
| Authentication Status: ${authStatus} | ||
| ThoughtSpot Instance: ${this.thoughtSpotProps.instanceUrl} | ||
| Error: ${error instanceof Error ? error.message : 'Unknown error'} | ||
| Connection test failed!`; | ||
| } | ||
| } else { | ||
| authStatus = "❌ Not Authenticated"; | ||
| pingResult = `Pong! 🏓 | ||
| Authentication Status: ${authStatus} | ||
| ThoughtSpot Instance: Not configured | ||
| Error: Missing ThoughtSpot credentials | ||
| Please ensure you are properly authenticated through OAuth.`; | ||
| } | ||
|
|
||
| // Publish working status | ||
| const workingStatusUpdate = { | ||
| kind: "status-update" as const, | ||
| taskId: taskId, | ||
| contextId: contextId, | ||
| status: { | ||
| state: "working" as const, | ||
| message: { | ||
| kind: "message" as const, | ||
| role: "agent" as const, | ||
| messageId: this.generateUuid(), | ||
| parts: [{ kind: "text" as const, text: "Testing authentication..." }], | ||
| taskId: taskId, | ||
| contextId: contextId, | ||
| }, | ||
| timestamp: new Date().toISOString(), | ||
| }, | ||
| final: false, | ||
| }; | ||
| eventBus.publish(workingStatusUpdate); | ||
|
|
||
| // Small delay to simulate processing | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
|
|
||
| // Publish artifact with ping result | ||
| const artifactUpdate = { | ||
| kind: "artifact-update" as const, | ||
| taskId: taskId, | ||
| contextId: contextId, | ||
| artifact: { | ||
| artifactId: "ping-result", | ||
| name: "Ping Test Result", | ||
| parts: [{ kind: "text" as const, text: pingResult }], | ||
| }, | ||
| append: false, | ||
| lastChunk: true, | ||
| }; | ||
| eventBus.publish(artifactUpdate); | ||
|
|
||
| // Publish final status | ||
| const finalUpdate = { | ||
| kind: "status-update" as const, | ||
| taskId: taskId, | ||
| contextId: contextId, | ||
| status: { | ||
| state: "completed" as const, | ||
| message: { | ||
| kind: "message" as const, | ||
| role: "agent" as const, | ||
| messageId: this.generateUuid(), | ||
| parts: [{ kind: "text" as const, text: "Ping test completed!" }], | ||
| taskId: taskId, | ||
| contextId: contextId, | ||
| }, | ||
| timestamp: new Date().toISOString(), | ||
| }, | ||
| final: true, | ||
| }; | ||
| eventBus.publish(finalUpdate); | ||
| eventBus.finished(); | ||
| } |
There was a problem hiding this comment.
The handlePingRequest method is quite long and contains repetitive logic for creating and publishing events (workingStatusUpdate, artifactUpdate, finalUpdate). Consider refactoring this by extracting the event creation and publishing logic into smaller, reusable helper functions. This would improve the method's readability and maintainability.
| // Extract the text content from the user message | ||
| const userText = userMessage.parts | ||
| ?.filter(part => part.kind === 'text') | ||
| ?.map(part => (part as any).text) |
There was a problem hiding this comment.
Using (part as any).text bypasses TypeScript's type system, which can lead to runtime errors if the part object doesn't have a text property. It's better to use a type assertion to a more specific type to maintain type safety. The filter on the previous line should already narrow the type.
| ?.map(part => (part as any).text) | |
| ?.map(part => (part as { text: string }).text) |
| '/openai/sse': ThoughtSpotOpenAIDeepResearchMCP.serveSSE("/openai/sse", { | ||
| binding: "OPENAI_DEEP_RESEARCH_MCP_OBJECT" | ||
| }) as any, // TODO: Remove 'any' | ||
| "/a2a": a2aHandler as any, // TODO: Remove 'any' |
There was a problem hiding this comment.
Using as any for the a2aHandler bypasses TypeScript's type safety checks. While there is a TODO comment, it's important to resolve this by providing the correct type for the handler to prevent potential runtime errors and improve code quality. The same applies to other as any casts in this file.
No description provided.