WIP: Generic Tool Abstraction #210

aluzzardi · 2025-07-09T20:30:36Z

Experiment. Only converted EnvironmentCreateTool and EnvironmentFileListTool

Signed-off-by: Andrea Luzzardi <[email protected]>

tiborvass · 2025-07-10T11:55:15Z

mcpserver/tools.go

+	ActualHandler func(ctx context.Context, request *R) (*ToolResponse[V], error)
+	Handler       server.ToolHandlerFunc


Suggested change

ActualHandler func(ctx context.Context, request *R) (*ToolResponse[V], error)

Handler server.ToolHandlerFunc

Handler func(ctx context.Context, request *R) (*ToolResponse[V], error)

MCPHandler server.ToolHandlerFunc

aluzzardi · 2025-07-31T15:15:29Z

mcpserver/tools.go

aluzzardi

test

aluzzardi

test2

DaggerAssistant

🤖 Code Review

Summary

This PR introduces a generic tool abstraction for the MCP server, refactoring EnvironmentCreateTool and EnvironmentFileListTool to use the new pattern. The changes include new request/response types, generic Tool struct, and helper functions for consistent argument parsing.

Assessment

⚠️ The refactoring is a good architectural improvement that will provide consistency and type safety. However, there are several critical issues that need to be addressed before merging: missing import statements that will cause compilation failures, variable shadowing issues, potential runtime panics from unsafe type assertions, and inconsistent error handling patterns. The typo in 'explainationArgument' should also be fixed. Once these issues are resolved, this provides a solid foundation for further tool migrations.

DaggerAssistant · 2025-08-07T13:00:34Z

mcpserver/mcpserver.go

@@ -0,0 +1,107 @@
+package mcpserver
+
+import (


🐛 Bug (High): Missing import for 'fmt' package. The file uses fmt.Errorf and fmt.Sprintf functions but doesn't import the fmt package.

Suggested change

import (

import (

"context"

"fmt"

"dagger.io/dagger"

"github.com/dagger/container-use/environment"

"github.com/dagger/container-use/repository"

"github.com/mark3labs/mcp-go/mcp"

)

DaggerAssistant · 2025-08-07T13:00:37Z

mcpserver/args.go

+import "github.com/mark3labs/mcp-go/mcp"
+
+var (
+	explainationArgument = mcp.WithString("explanation",


🎨 Style (Low): Typo in variable name 'explainationArgument' should be 'explanationArgument'

Suggested change

explainationArgument = mcp.WithString("explanation",

explanationArgument = mcp.WithString("explanation",

DaggerAssistant · 2025-08-07T13:00:41Z

mcpserver/args.go

+func newRepositoryTool(name string, description string, args ...mcp.ToolOption) mcp.Tool {
+	opts := []mcp.ToolOption{
+		mcp.WithDescription(description),
+		explainationArgument,


🎨 Style (Low): Reference to typo variable name should be corrected to 'explanationArgument'

Suggested change

explainationArgument,

explanationArgument,

DaggerAssistant · 2025-08-07T13:00:44Z

mcpserver/args.go

+func newEnvironmentTool(name string, description string, args ...mcp.ToolOption) mcp.Tool {
+	opts := []mcp.ToolOption{
+		mcp.WithDescription(description),
+		explainationArgument,


🎨 Style (Low): Reference to typo variable name should be corrected to 'explanationArgument'

Suggested change

explainationArgument,

explanationArgument,

DaggerAssistant · 2025-08-07T13:00:49Z

mcpserver/mcpserver.go

+}
+
+func parseBaseRequest(request mcp.CallToolRequest) (BaseRequest, error) {
+	explanation, err := request.RequireString("explanation")


🐛 Bug (Medium): The parseBaseRequest function tries to use RequireString for 'explanation' but based on the args.go file, explanation is not required. This should use GetString with a default value instead.

Suggested change

explanation, err := request.RequireString("explanation")

func parseBaseRequest(request mcp.CallToolRequest) (BaseRequest, error) {

explanation := request.GetString("explanation", "")

return BaseRequest{

Explanation: explanation,

}, nil

}

DaggerAssistant · 2025-08-07T13:01:15Z

mcpserver/tools.go

⚡ Performance (Low): Inefficient slice allocation and appending in a loop. Pre-allocate the slice with known capacity to avoid multiple allocations.

Suggested change

if portList, ok := request.GetArguments()["ports"].([]any); ok {

req.Ports = make([]int, len(portList))

for i, port := range portList {

req.Ports[i] = int(port.(float64))

}

}

DaggerAssistant · 2025-08-07T13:01:20Z

mcpserver/tools.go

+		if portList, ok := request.GetArguments()["ports"].([]any); ok {
+			for _, port := range portList {
+				req.Ports = append(req.Ports, int(port.(float64)))
 			}


🐛 Bug (Medium): Potential panic from type assertion without checking. The type assertion port.(float64) could panic if the port value is not a float64.

Suggested change

}

for _, port := range portList {

if portFloat, ok := port.(float64); ok {

req.Ports = append(req.Ports, int(portFloat))

} else {

return nil, fmt.Errorf("invalid port type: expected number, got %T", port)

}

}

DaggerAssistant · 2025-08-07T13:01:26Z

mcpserver/tools.go

🔧 Maintainability (Low): Missing return value in the ToolResponse. The Data field is not populated for non-background command execution, which could be inconsistent with the generic pattern.

Suggested change

return &ToolResponse[environment.EndpointMappings]{

Data: environment.EndpointMappings{}, // Empty mappings for non-background commands

Message: fmt.Sprintf("%s\n\nAny changes to the container workdir (%s) have been committed and pushed to container-use/ remote", stdout, env.Config.Workdir),

}, nil

DaggerAssistant · 2025-08-07T13:01:32Z

mcpserver/tools.go

🔧 Maintainability (Medium): EnvironmentOpenTool is using the old Tool struct definition (Tool[Request, Response]) but hasn't been converted to the new generic pattern like EnvironmentCreateTool and EnvironmentFileListTool. This creates inconsistency in the codebase.

Suggested change

// This tool should be converted to the new generic pattern like EnvironmentCreateTool

// Consider creating a specific request struct and migrating to the new pattern

DaggerAssistant · 2025-08-07T13:01:38Z

mcpserver/tools.go

🎨 Style (Low): Inconsistent indentation in multi-line string. The description should maintain consistent indentation.

Suggested change

`Creates a new development environment.

The environment is the result of a the setups commands on top of the base image.

Read carefully the instructions to understand the environment.

DO NOT manually install toolchains inside the environment, instead explicitly call environment_update`,

aluzzardi · 2025-08-07T13:54:50Z

/cc @dagger

aluzzardi · 2025-08-07T14:04:20Z

/cc @dagger

aluzzardi · 2025-08-08T12:24:43Z

@dagger /review

aluzzardi · 2025-08-08T12:27:01Z

@dagger /review

aluzzardi · 2025-08-08T12:41:35Z

@dagger /review

aluzzardi · 2025-08-08T12:52:43Z

@dagger /review

aluzzardi · 2025-08-08T12:58:25Z

@dagger /review

aluzzardi · 2025-08-08T15:48:47Z

@dagger review

aluzzardi · 2025-08-08T15:49:33Z

@dagger review

aluzzardi · 2025-08-08T15:49:53Z

@dagger review

aluzzardi · 2025-08-08T16:16:54Z

@dagger review

aluzzardi · 2025-08-18T11:02:15Z

@dagger review

aluzzardi · 2025-08-18T11:05:12Z

@dagger keep the typos

aluzzardi · 2025-08-18T11:06:39Z

@dagger you're wrong about the missing json import

aluzzardi · 2025-08-18T14:54:12Z

@dagger please review

aluzzardi · 2025-08-18T14:55:57Z

@dagger keep the typo

aluzzardi · 2025-08-18T14:56:44Z

@dagger review

aluzzardi · 2025-08-18T14:58:18Z

@dagger review

aluzzardi · 2025-08-18T16:54:43Z

Test

dagger

🤖 Code Review

Summary

This PR introduces a generic tool abstraction pattern for the MCP server, refactoring only two tools (EnvironmentCreateTool and EnvironmentFileListTool) to demonstrate the new architecture. The changes include creating type-safe request/response structures, separating concerns through different files (args.go, mcpserver.go), and introducing generic handler types.

Assessment

⚠️ This is a well-structured refactoring that introduces better type safety and separation of concerns. However, there are several issues that need attention:

Positives:

Good architectural direction: The generic Tool[R Request, V Response] pattern provides better type safety than the previous untyped approach
Better separation of concerns: Moving common argument definitions to args.go and shared utilities to mcpserver.go improves code organization
Improved error handling: Using fmt.Errorf with %w verb for proper error wrapping

Issues Found:

Critical bug: Variable shadowing of json package import in tools.go:112
Missing import: mcpserver.go uses fmt.Errorf but doesn't import fmt
Typo: "explainationArgument" should be "explanationArgument" throughout
Required field validation bug: Title field is marked as required but uses optional extraction

The refactoring pattern looks solid but needs these fixes before merging.

dagger · 2025-08-20T10:20:36Z

mcpserver/tools.go

+				return mcp.NewToolResultError(err.Error()), nil
+			}
+
+			json, err := json.Marshal(response.Data)


🐛 Bug (High): Variable shadowing of json package import: The variable json on line 112 shadows the imported encoding/json package, which will cause compilation errors or unexpected behavior.

// Current problematic code: json, err := json.Marshal(response.Data) // 'json' shadows the package

This prevents access to the json.Marshal function and will likely cause a compilation error.

Suggested change

json, err := json.Marshal(response.Data)

data, err := json.Marshal(response.Data)

if err != nil {

return mcp.NewToolResultErrorFromErr("failed to marshal response", err), nil

}

body := string(data)

dagger · 2025-08-20T10:20:36Z

mcpserver/mcpserver.go

@@ -0,0 +1,107 @@
+package mcpserver
+
+import (


🐛 Bug (High): Missing import for fmt package: The file uses fmt.Errorf in multiple error handling locations (lines where functions return formatted errors), but the fmt package is not imported.

For example, the code uses fmt.Errorf("unable to open the repository: %w", err) pattern in the refactored tools, but fmt is not available in this file's scope.

Suggested change

import (

import (

"context"

"fmt"

"dagger.io/dagger"

"github.com/dagger/container-use/environment"

"github.com/dagger/container-use/repository"

"github.com/mark3labs/mcp-go/mcp"

)

dagger · 2025-08-20T10:20:36Z

mcpserver/args.go

+import "github.com/mark3labs/mcp-go/mcp"
+
+var (
+	explainationArgument = mcp.WithString("explanation",


🎨 Style (Medium): Typo in variable name: explainationArgument should be explanationArgument (missing 'n'). This typo is used consistently throughout the file on lines 6, 22, 33.

Suggested change

explainationArgument = mcp.WithString("explanation",

explanationArgument = mcp.WithString("explanation",

mcp.Description("One sentence explanation for why this directory is being listed."),

)

dagger · 2025-08-20T10:20:37Z

mcpserver/tools.go

-			return mcp.NewToolResultErrorFromErr("dagger client not found in context", nil), nil
+		return &EnvironmentCreateToolRequest{
+			BaseRepositoryRequest: base,
+			Title:                 request.GetString("title", ""),


🐛 Bug (Medium): Required field validation inconsistency: The title field is defined as mcp.Required() in the tool definition (line 262), but the ParseRequest function uses request.GetString("title", "") which returns an empty string if the field is missing, rather than request.RequireString("title") which would return an error.

This means required field validation is bypassed, potentially allowing empty titles when they should be mandatory.

Suggested change

Title: request.GetString("title", ""),

title, err := request.RequireString("title")

if err != nil {

return nil, err

}

return &EnvironmentCreateToolRequest{

BaseRepositoryRequest: base,

Title: title,

}, nil

WIP: Generic Tool Abstraction

282173e

Signed-off-by: Andrea Luzzardi <[email protected]>

aluzzardi force-pushed the tools-refactoring branch from eadc258 to 282173e Compare July 9, 2025 20:39

convert EnvironmentRunCmdTool

29f8377

Signed-off-by: Andrea Luzzardi <[email protected]>

tiborvass reviewed Jul 10, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

aluzzardi commented Jul 31, 2025

View reviewed changes

mcpserver/tools.go

Copy link

Member Author

aluzzardi Jul 31, 2025

Choose a reason for hiding this comment

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

test

		ActualHandler func(ctx context.Context, request R) (ToolResponse[V], error)
		Handler server.ToolHandlerFunc

-import (
+import (
+	"context"
+	"fmt"
+	"dagger.io/dagger"
+	"github.com/dagger/container-use/environment"
+	"github.com/dagger/container-use/repository"
+	"github.com/mark3labs/mcp-go/mcp"
+)

	explainationArgument = mcp.WithString("explanation",
	explanationArgument = mcp.WithString("explanation",

-	explanation, err := request.RequireString("explanation")
+func parseBaseRequest(request mcp.CallToolRequest) (BaseRequest, error) {
+	explanation := request.GetString("explanation", "")
+	return BaseRequest{
+		Explanation: explanation,
+	}, nil
+}

+		if portList, ok := request.GetArguments()["ports"].([]any); ok {
+			req.Ports = make([]int, len(portList))
+			for i, port := range portList {
+				req.Ports[i] = int(port.(float64))
+			}
+		}

-			}
+			for _, port := range portList {
+				if portFloat, ok := port.(float64); ok {
+					req.Ports = append(req.Ports, int(portFloat))
+				} else {
+					return nil, fmt.Errorf("invalid port type: expected number, got %T", port)
+				}
+			}

+		return &ToolResponse[environment.EndpointMappings]{
+			Data:    environment.EndpointMappings{}, // Empty mappings for non-background commands
+			Message: fmt.Sprintf("%s\n\nAny changes to the container workdir (%s) have been committed and pushed to container-use/ remote", stdout, env.Config.Workdir),
+		}, nil


	// This tool should be converted to the new generic pattern like EnvironmentCreateTool
	// Consider creating a specific request struct and migrating to the new pattern

+		`Creates a new development environment.
+The environment is the result of a the setups commands on top of the base image.
+Read carefully the instructions to understand the environment.
+DO NOT manually install toolchains inside the environment, instead explicitly call environment_update`,

-			json, err := json.Marshal(response.Data)
+			data, err := json.Marshal(response.Data)
+			if err != nil {
+				return mcp.NewToolResultErrorFromErr("failed to marshal response", err), nil
+			}
+			body := string(data)

-	explainationArgument = mcp.WithString("explanation",
+	explanationArgument = mcp.WithString("explanation",
+		mcp.Description("One sentence explanation for why this directory is being listed."),
+	)

WIP: Generic Tool Abstraction #210

Are you sure you want to change the base?

WIP: Generic Tool Abstraction #210

Uh oh!

Conversation

aluzzardi commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

aluzzardi left a comment

Choose a reason for hiding this comment

Uh oh!

aluzzardi left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

DaggerAssistant left a comment

Choose a reason for hiding this comment

🤖 Code Review

Summary

Assessment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aluzzardi commented Aug 7, 2025

Uh oh!

aluzzardi commented Jul 9, 2025 •

edited

Loading