refactor(chatops-lark): update MCP and OpenAI dependencies and switch to HTTP streaming MCP client

[wuhuizuo]

No description provided.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign wuhuizuo for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gemini-code-assist]

Summary of Changes

Hello @wuhuizuo, I'm Gemini Code Assist¹! 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!

This pull request focuses on modernizing the project's foundational components by upgrading the Go language version and critical third-party libraries. A significant change involves migrating the MCP client from an SSE-based approach to a more efficient HTTP streaming method, aiming to enhance performance and reliability in inter-service communication. These updates ensure the application remains compatible with the latest ecosystem standards and leverages improved functionalities from its dependencies.

Highlights

• Go Version Update: The Go language version has been updated from 1.23.4 to 1.25.0, along with the addition of toolchain go1.25.1.
• Dependency Updates: Key dependencies, including 'github.com/mark3labs/mcp-go' and 'github.com/openai/openai-go', have been significantly updated to their latest stable versions. 'github.com/spf13/cast' also received a minor update.
• MCP Client Switch: The MCP (Multi-Cloud Platform) client implementation has been refactored from using Server-Sent Events (SSE) to an HTTP streaming client, improving communication efficiency.
• New Indirect Dependencies: Several new indirect dependencies have been introduced, such as 'github.com/bahlo/generic-list-go', 'github.com/buger/jsonparser', 'github.com/invopop/jsonschema', 'github.com/mailru/easyjson', and 'github.com/wk8/go-ordered-map/v2', to support the updated libraries.

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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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

1. 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. ↩

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary

This pull request upgrades dependencies for the chatops-lark module, including updates to MCP and OpenAI libraries, and refactors the MCP client to use an HTTP streaming approach instead of SSE (Server-Sent Events). The implementation modifies client initialization, introduces new library methods, and updates related logging statements. Overall, the changes are straightforward, well-contained, and align with the stated goal, but there are some issues with error handling and potential missed edge cases.

---

Critical Issues

1. Error Handling in newStreamHttpMcpClient (File: ask_mcp.go, Lines: 17-43)

   • Issue: The Start method of the MCP client is invoked directly after client creation without verifying if the client is nil. If client.NewStreamableHttpClient returns a nil client, this will cause a runtime panic.
   • Suggestion:

     if mcpClient == nil {
         return nil, fmt.Errorf("Failed to create MCP client: client instance is nil")
     }
     if err := mcpClient.Start(ctx); err != nil {
         return nil, fmt.Errorf("Failed to start client: %v", err)
     }

2. Logging Context in initializeMCPClient (File: ask_mcp.go, Lines: 94-103)

   • Issue: If newStreamHttpMcpClient fails, the error is logged but not propagated correctly. This could lead to silent failures if the error is ignored upstream.
   • Suggestion: Ensure the error is logged and returned consistently:

     logger := log.With().Str("name", name).Str("url", url).Logger()
     c, err := newStreamHttpMcpClient(ctx, url)
     if err != nil {
         logger.Err(err).Msg("failed to create MCP client")
         return nil, nil, fmt.Errorf("failed to initialize MCP client: %v", err)
     }

---

Code Improvements

1. Refactoring Tool Declaration Handling (File: ask.go, Lines: 160-172)

   • Issue: The loop for initializing MCP clients appends both clients and tool declarations separately, which could be refactored for better readability and reduced duplication.
   • Suggestion:

     for name, cfg := range cfg.Ask.LLM.MCPServers {
         log.Debug().Str("name", name).Str("url", cfg.BaseURL).Msg("initializing MCP HTTP stream client")
         client, declarations, err := initializeMCPClient(ctx, name, cfg.BaseURL)
         if err != nil {
             log.Err(err).Str("name", name).Str("url", cfg.BaseURL).Msg("failed to initialize MCP HTTP stream client")
             continue
         }
         mcpClients = append(mcpClients, client)
         toolDeclarations = append(toolDeclarations, declarations...)
     }
     log.Debug().Msgf("Successfully initialized %d MCP HTTP stream clients", len(mcpClients))

2. Improving MCP Initialization Request Construction (File: ask_mcp.go, Lines: 17-43)

   • Issue: The InitializeRequest construction is spread across multiple lines. This can be made more concise for readability.
   • Suggestion:

     initRequest := mcp.InitializeRequest{
         Params: mcp.InitializeParams{
             ProtocolVersion: mcp.LATEST_PROTOCOL_VERSION,
             ClientInfo: mcp.Implementation{Name: McpClientName, Version: McpClientVersion},
         },
     }

---

Best Practices

1. Testing Coverage for MCP Client Refactor

   • Issue: There is no description or evidence of added tests to ensure the MCP client switch to HTTP streaming works as intended or handles edge cases (e.g., invalid URLs, connectivity issues).
   • Suggestion: Add unit tests for newStreamHttpMcpClient to verify the client creation, initialization, and error handling. For example:

     func TestNewStreamHttpMcpClient_InvalidURL(t *testing.T) {
         ctx := context.Background()
         _, err := newStreamHttpMcpClient(ctx, "invalid-url")
         if err == nil {
             t.Fatalf("expected error for invalid URL, got nil")
         }
     }

2. Documentation Updates

   • Issue: The refactored function newStreamHttpMcpClient lacks inline comments explaining the switch from SSE to HTTP streaming and its benefits.
   • Suggestion: Add a comment explaining the rationale:

     // newStreamHttpMcpClient initializes an MCP client using HTTP streaming.
     // HTTP streaming provides better reliability and scalability compared to SSE.

3. Dependency Audit

   • Issue: Several new indirect dependencies were added (e.g., generic-list-go, jsonparser, easyjson). It's unclear if these are strictly required or could lead to potential bloat.
   • Suggestion: Verify whether all new dependencies are necessary or if unused dependencies can be removed to minimize the project footprint.

---

Conclusion

The refactor aligns well with the stated goals but leaves room for improvement in error handling, testing coverage, and documentation clarity. Addressing these points will ensure increased robustness and maintainability.

[gemini-code-assist]

Code Review

This pull request refactors the chatops-lark service to update MCP and OpenAI dependencies and switches from an SSE-based MCP client to an HTTP streaming client. The changes are mostly dependency updates and adjustments to the client initialization logic. I've found a critical bug in the new client initialization where an error from client creation is ignored due to variable shadowing, which could lead to a panic. I've also suggested a minor improvement for error wrapping to align with Go best practices.

[gemini-code-assist]

There's a variable shadowing bug here. The err returned from client.NewStreamableHttpClient on line 22 is shadowed by a new err variable in the if statement on line 23. This means that if NewStreamableHttpClient fails and returns an error, it will be ignored. The code will then attempt to call mcpClient.Start(ctx) on a potentially nil client, which will cause a panic.

You should check the error from NewStreamableHttpClient before proceeding. I've also updated the error wrapping to use %w which is a Go best practice to preserve the error chain.

Here's a suggested fix:

	mcpClient, err := client.NewStreamableHttpClient(baseURL)
	if err != nil {
		return nil, fmt.Errorf("failed to create new streamable http client: %w", err)
	}

	if err := mcpClient.Start(ctx); err != nil {
		return nil, fmt.Errorf("failed to start client: %w", err)
	}

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary

This PR updates dependencies (MCP and OpenAI) and transitions from an SSE-based MCP client to an HTTP streaming MCP client. The approach involves replacing the client implementation while preserving the initialization logic and relevant configurations. Overall, the changes are well-structured but introduce some areas for improvement regarding error handling, code clarity, and testing coverage.

---

Critical Issues

1. Error Wrapping (ask_mcp.go:56):

   • The error in mcpClient.Initialize is wrapped using %v instead of %w, preventing proper usage with errors.Is or errors.As.
   • Why it's an issue: Not using %w can lead to difficulties in error inspection and propagation in Go's error handling ecosystem.
   • Suggested Fix:

     return nil, fmt.Errorf("Failed to initialize: %w", err)

2. Potential Connection Handling Issue (ask_mcp.go:25):

   • The Start(ctx) call on the MCP client might not handle timeouts or context cancellation effectively, as no timeout configuration is evident in the new implementation.
   • Why it's an issue: Lack of timeout handling can cause the application to hang indefinitely if the server is unresponsive.
   • Suggested Fix:
     • Add a timeout to the context before passing it to Start(ctx):

       ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
       defer cancel()
       if err := mcpClient.Start(ctx); err != nil {
           return nil, fmt.Errorf("Failed to start client: %w", err)
       }

---

Code Improvements

1. Consistency in Naming (ask_mcp.go:16-104):

   • The function newStreamHttpMcpClient uses "StreamHttp" in its name, which is inconsistent with Go's naming conventions. Consider renaming to newHTTPStreamMcpClient for clarity.
   • Suggested Fix:

     func newHTTPStreamMcpClient(ctx context.Context, baseURL string) (*client.Client, error) {

2. Reduce Code Duplication (ask.go:162-174):

   • Logging statements like log.Debug() and log.Err() are repeated multiple times for initializing MCP clients. This could be extracted into a helper function for cleaner code.
   • Suggested Fix:

     func logMcpClientStatus(logger zerolog.Logger, name, url, action string, err error) {
         if err != nil {
             logger.Err(err).Str("name", name).Str("url", url).Msg(fmt.Sprintf("failed to %s MCP client", action))
         } else {
             logger.Debug().Str("name", name).Str("url", url).Msg(fmt.Sprintf("%s MCP client successfully", action))
         }
     }

3. Improved Error Messaging (ask.go:164):

   • Error messages like "failed to initialize MCP HTTP stream client" could include more specific context (e.g., which exact step failed—connection or initialization).
   • Suggested Fix: Enhance messages by specifying the failed operation.

---

Best Practices

1. Testing Coverage:

   • The switch from SSE to HTTP streaming introduces new client behavior (newStreamHttpMcpClient). Ensure proper unit tests are added to validate:
     • Connection establishment.
     • Initialization logic.
     • Error scenarios (e.g., invalid URLs, server unavailability).

2. Documentation Updates:

   • No documentation was provided for the new HTTP streaming client usage (ask_mcp.go). Add comments describing its purpose and differences from the SSE client.
   • Suggested Fix:

     // newHTTPStreamMcpClient creates a new MCP client using HTTP streaming.
     // This replaces the previous SSE-based implementation for improved scalability.

3. Dependency Management (go.mod):

   • A large number of indirect dependencies were added. Verify if all these dependencies are necessary or if they can be minimized to reduce the application’s complexity.

4. Style Consistency:

   • Use consistent capitalization in log messages (HTTP stream client vs. HTTP Stream Client). This ensures uniformity across logs.

---

Conclusion

The transition to an HTTP streaming MCP client is a meaningful improvement but introduces minor issues in error handling, naming, and logging. Addressing these will enhance maintainability, robustness, and clarity. Additionally, ensure adequate tests are added for the new client functionality.