Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions mcpserver/args.go
Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable usage - should be 'explanationArgument' instead of 'explainationArgument'

Suggested change
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable usage - should be 'explanationArgument' instead of 'explainationArgument'

Suggested change
explanationArgument,

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package mcpserver

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

var (
explainationArgument = mcp.WithString("explanation",
Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name: 'explainationArgument' should be 'explanationArgument' (missing 'n' in 'explanation').

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

Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Low): Typo in variable name: 'explainationArgument' should be 'explanationArgument'

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Low): Typo in variable name: 'explainationArgument' should be 'explanationArgument'

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Low): Typo in variable name 'explainationArgument' should be 'explanationArgument'

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This typo is used consistently throughout the codebase and affects the argument definition for explanation fields.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name 'explainationArgument' should be 'explanationArgument'

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This typo is propagated throughout the codebase.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable name: 'explainationArgument' should be 'explanationArgument' (missing 'a'). This typo is used throughout the codebase and affects consistency.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This typo is used throughout the codebase and could cause confusion.

Suggested change
explainationArgument = mcp.WithString("explanation",
explainationArgument = mcp.WithString("explanation",

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This typo propagates throughout the codebase.

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Critical spelling error in variable name 'explainationArgument' - should be 'explanationArgument'. This will cause a mismatch between the tool definition and the parsing logic, breaking functionality.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name: 'explainationArgument' should be 'explanationArgument'

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This typo is used consistently but may cause confusion.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Critical spelling error: Variable name explainationArgument should be explanationArgument (missing 'n'). This typo is used throughout the codebase and could cause confusion.

Suggested change
explainationArgument = mcp.WithString("explanation",
explainationArgument = mcp.WithString("explanation",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Typo in variable name: explainationArgument

The variable name should be explanationArgument (missing 'n'). This is a minor spelling error but affects code readability.

Evidence: The variable is used consistently throughout the file but contains a typo. The word should be "explanation" not "explaination".

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

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Typo: explainationArgument should be explanationArgument (missing 'n'). This typo is used consistently across lines 6, 22, and 33.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable name: explainationArgument should be explanationArgument. This typo is used consistently throughout the file, which could lead to confusion and potential bugs.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually like the typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagger i actually like the typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagger keep the typo

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo: Variable name explainationArgument should be explanationArgument (missing 'n'). This creates inconsistent naming throughout the codebase.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagger keep the typo

Copy link

Choose a reason for hiding this comment

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

🎨 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."),
)

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

Choose a reason for hiding this comment

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

🔧 Maintainability (Low): Generic description: The description 'One sentence explanation for why this directory is being listed.' is specific to directory listing but is used for all tools. Consider making it more generic like 'One sentence explanation for why this action is being performed.'

Copy link

Choose a reason for hiding this comment

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

🔧 Maintainability (Low): The description for explainationArgument mentions 'directory is being listed' which is too specific. This argument is used by multiple tools, so it should have a more generic description.

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

)
environmentSourceArgument = mcp.WithString("environment_source",
mcp.Description("Absolute path to the source git repository for the environment."),
mcp.Required(),
)
environmentIDArgument = mcp.WithString("environment_id",
mcp.Description("The ID of the environment for this command. Must call `environment_create` first."),
mcp.Required(),
)
)

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

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Reference to misspelled variable: This should use the corrected variable name 'explanationArgument' to maintain consistency.

Suggested change
explainationArgument,
explanationArgument,

Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Low): Using misspelled variable name: should be 'explanationArgument' instead of 'explainationArgument'

Suggested change
explainationArgument,
explanationArgument,

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Low): Fix reference to misspelled variable name

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Low): Using typo'd variable name 'explainationArgument' instead of corrected 'explanationArgument'

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Consistent typo: 'explainationArgument' should be 'explanationArgument' to fix the spelling error.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This is the same typo from line 6.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): The typo continues here - 'explainationArgument' should be 'explanationArgument' to maintain consistency and correct spelling.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable usage: 'explainationArgument' should be 'explanationArgument'.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Fix variable reference to match corrected spelling

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Update reference to use corrected variable name

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Using the misspelled 'explainationArgument' variable. This needs to be corrected to match the fix above.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Reference to misspelled variable name should be corrected to match the fix above

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Fix reference to renamed variable (from explainationArgument to explanationArgument)

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Reference to misspelled variable name should be corrected

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Use of misspelled variable name due to typo above

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Use the corrected variable name for consistency

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Using misspelled variable name. Should use 'explanationArgument' instead of 'explainationArgument' for consistency with the corrected spelling.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Update reference to corrected variable name

This reference needs to be updated to match the corrected explanationArgument variable name.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Use corrected variable name for consistency.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Fix typo: Update usage of the misspelled variable name.

Suggested change
explainationArgument,
explanationArgument,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagger keep the typo

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Reference to typo: Update reference to use correct variable name.

Suggested change
explainationArgument,
explanationArgument,

environmentSourceArgument,
}
opts = append(opts, args...)

return mcp.NewTool(name, opts...)
}

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

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Reference to misspelled variable: This should use the corrected variable name 'explanationArgument' to maintain consistency.

Suggested change
explainationArgument,
explanationArgument,

Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Low): Using misspelled variable name: should be 'explanationArgument' instead of 'explainationArgument'

Suggested change
explainationArgument,
explanationArgument,

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Low): Fix reference to misspelled variable name

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Low): Using typo'd variable name 'explainationArgument' instead of corrected 'explanationArgument'

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Consistent typo: 'explainationArgument' should be 'explanationArgument' to fix the spelling error.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Typo in variable name: 'explainationArgument' should be 'explanationArgument'. This is the same typo from line 6.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): The typo continues here - 'explainationArgument' should be 'explanationArgument'.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Typo in variable usage: 'explainationArgument' should be 'explanationArgument'.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Fix variable reference to match corrected spelling

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Update reference to use corrected variable name

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Using the misspelled 'explainationArgument' variable. This needs to be corrected to match the fix above.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Reference to misspelled variable name should be corrected to match the fix above

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Fix reference to renamed variable (from explainationArgument to explanationArgument)

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Reference to misspelled variable name should be corrected

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Use of misspelled variable name due to typo above

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Use the corrected variable name for consistency

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Using misspelled variable name. Should use 'explanationArgument' instead of 'explainationArgument' for consistency with the corrected spelling.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Update reference to corrected variable name

This reference needs to be updated to match the corrected explanationArgument variable name.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Use corrected variable name for consistency.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Fix typo: Update usage of the misspelled variable name.

Suggested change
explainationArgument,
explanationArgument,

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Reference to typo: Update reference to use correct variable name.

Suggested change
explainationArgument,
explanationArgument,

environmentSourceArgument,
environmentIDArgument,
}
opts = append(opts, args...)

return mcp.NewTool(name, opts...)
}
107 changes: 107 additions & 0 deletions mcpserver/mcpserver.go
Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Simplify return statement: Since we removed the error handling, we can simplify this return statement and remove the error return path.

Suggested change
return BaseRequest{
Explanation: explanation,
}, nil

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Using panic() for missing dagger client is inappropriate for a server context. This will crash the entire server instead of gracefully handling the error. Should return an error instead.

Suggested change
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Missing import: The code uses fmt.Errorf on lines 278, 283, 292 but the fmt package is not imported. This will cause compilation errors.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package mcpserver
Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Missing fmt import. The dagFromContext function suggestion above requires fmt package, and error wrapping patterns throughout the file would benefit from it.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Missing encoding/json import - The code uses json.Marshal in tools.go line 112 within the createHandler function, but encoding/json is not imported in mcpserver.go. This will cause a compilation error.

Evidence: Line 112 in tools.go shows json, err := json.Marshal(response.Data) but the import is missing from this file.

Suggested change
package mcpserver
package mcpserver
import (
"context"
"encoding/json"
"dagger.io/dagger"
"github.com/dagger/container-use/environment"
"github.com/dagger/container-use/repository"
"github.com/mark3labs/mcp-go/mcp"
)


import (
Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 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"
)

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Missing 'fmt' import. The file uses fmt.Errorf and fmt.Sprintf functions but doesn't import the fmt package, which will cause compilation errors.

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"
)

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Missing fmt import - the file uses fmt.Errorf and fmt.Sprintf but doesn't import the fmt package. This will cause compilation errors.

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"
)

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Missing import: The fmt package is used in line 278+ but not imported, causing compilation errors.

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"
)

Copy link

Choose a reason for hiding this comment

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

🐛 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"
)

"context"
Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Missing fmt import. The file uses fmt.Errorf in multiple places but doesn't import the fmt package.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Missing import: The code uses fmt.Errorf in multiple places (lines 278, 283, 292, 512, 517, 529, 551, 646, 651) but doesn't import the "fmt" package. This will cause compilation failure.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): Missing fmt import: The code uses fmt.Errorf in multiple locations (lines 278, 283, 292, 517, 529, 551, 646, 651) but the fmt package is not imported. This will cause compilation failures.

Suggested change
"context"
import (
"context"
"fmt"
"dagger.io/dagger"


"dagger.io/dagger"
"github.com/dagger/container-use/environment"
"github.com/dagger/container-use/repository"
"github.com/mark3labs/mcp-go/mcp"
)

func dagFromContext(ctx context.Context) *dagger.Client {
Copy link

Choose a reason for hiding this comment

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

🔧 Maintainability (Low): If dagFromContext returns an error instead of panicking, the function signature needs to be updated to return (*dagger.Client, error).

Suggested change
func dagFromContext(ctx context.Context) *dagger.Client {
func dagFromContext(ctx context.Context) (*dagger.Client, error) {

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Using panic() for missing dagger client is too harsh. This should return an error instead to allow proper error handling by callers.

Suggested change
func dagFromContext(ctx context.Context) *dagger.Client {
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

🎨 Style (Low): Function name 'dagFromContext' should be 'daggerClientFromContext' or 'clientFromContext' for better clarity about what it returns.

dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
panic("dagger client not found in context")
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 Maintainability (Medium): Panic on missing context: The dagFromContext function panics when the dagger client is not found in context. While this might be intentional, consider returning an error instead to provide more graceful error handling and better debugging information.

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Using panic for missing dagger client is too aggressive and can crash the entire application. Should return an error instead to allow graceful error handling.

Suggested change
panic("dagger client not found in context")
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

🔒 Security (Medium): The dagFromContext function panics when the dagger client is not found in context instead of returning an error. This could cause the entire service to crash. Consider returning an error instead of panicking to provide better error handling and recovery.

Suggested change
panic("dagger client not found in context")
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

🔧 Maintainability (Medium): The panic in dagFromContext() could be hard to debug in production. Consider returning an error instead of panicking to allow for more graceful error handling.

Suggested change
panic("dagger client not found in context")
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

Performance (Low): Using panic() for missing dagger client is harsh and could crash the entire server. Consider returning an error instead to allow for graceful error handling.

Suggested change
panic("dagger client not found in context")
return nil, fmt.Errorf("dagger client not found in context")

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Using panic for missing context value is harsh and could crash the server. Consider returning an error instead for more graceful error handling.

Suggested change
panic("dagger client not found in context")
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

🔒 Security (Low): Using panic for missing dagger client could cause service disruption. Consider returning an error instead to allow graceful error handling.

Suggested change
panic("dagger client not found in context")
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Using panic() for missing dagger client is too aggressive and will crash the entire server. Consider returning an error instead.

Suggested change
panic("dagger client not found in context")
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Panic on missing dagger client instead of returning proper error. This breaks the error handling pattern and could crash the server unexpectedly.

Suggested change
panic("dagger client not found in context")
func dagFromContext(ctx context.Context) (*dagger.Client, error) {
dag, ok := ctx.Value(daggerClientKey{}).(*dagger.Client)
if !ok {
return nil, fmt.Errorf("dagger client not found in context")
}
return dag, nil
}

}
return dag
}

type Request interface {
isRequest()
}

type BaseRequest struct {
Explanation string `json:"explanation"`
}

func (BaseRequest) isRequest() {}

type BaseRepositoryRequest struct {
BaseRequest

EnvironmentSource string `json:"environment_source"`
}

type BaseEnvironmentRequest struct {
BaseRepositoryRequest

EnvironmentID string `json:"environment_id"`
}

type Response any
Copy link

Choose a reason for hiding this comment

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

🔧 Maintainability (Low): Type alias 'Response any' is too broad and provides no type safety. Consider using a more specific constraint or interface to ensure response types are serializable.


type ToolResponse[T Response] struct {
Message string
Data T
}

func openRepositoryFromRequest(ctx context.Context, request BaseRepositoryRequest) (*repository.Repository, error) {
repo, err := repository.Open(ctx, request.EnvironmentSource)
if err != nil {
return nil, err
}
return repo, nil
}

func openEnvironmentFromRequest(ctx context.Context, request BaseEnvironmentRequest) (*repository.Repository, *environment.Environment, error) {
repo, err := openRepositoryFromRequest(ctx, request.BaseRepositoryRequest)
if err != nil {
return nil, nil, err
}
env, err := repo.Get(ctx, dagFromContext(ctx), request.EnvironmentID)
Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Call to dagFromContext() can panic but error is not handled. With the panic-based implementation, this could crash the application.

Suggested change
env, err := repo.Get(ctx, dagFromContext(ctx), request.EnvironmentID)
dag, err := dagFromContext(ctx)
if err != nil {
return nil, nil, err
}
env, err := repo.Get(ctx, dag, request.EnvironmentID)

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Critical): If dagFromContext() is changed to return an error, this line needs to be updated to handle the error properly, otherwise the code will not compile.

Suggested change
env, err := repo.Get(ctx, dagFromContext(ctx), request.EnvironmentID)
dag, err := dagFromContext(ctx)
if err != nil {
return nil, nil, err
}
env, err := repo.Get(ctx, dag, request.EnvironmentID)

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): If dagFromContext is changed to return an error, this call needs to be updated to handle the error.

Suggested change
env, err := repo.Get(ctx, dagFromContext(ctx), request.EnvironmentID)
dag, err := dagFromContext(ctx)
if err != nil {
return nil, nil, err
}
env, err := repo.Get(ctx, dag, request.EnvironmentID)

if err != nil {
return nil, nil, err
}
return repo, env, nil
}

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

Choose a reason for hiding this comment

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

🔧 Maintainability (Low): parseBaseRequest function inconsistency: calls RequireString for explanation but args.go defines it without mcp.Required(). Either make explanation optional or mark it as required in the argument definition.

Suggested change
func parseBaseRequest(request mcp.CallToolRequest) (BaseRequest, error) {
In args.go, either add mcp.Required() to explanationArgument or change parseBaseRequest to use GetString() with a default value

explanation, err := request.RequireString("explanation")
Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Inconsistent parameter handling: The 'explanation' parameter is being treated as required with RequireString(), but in the args.go file, the explanationArgument is defined without mcp.Required(). This should use GetString() with a default value instead to match the optional nature of the parameter.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Medium): parseBaseRequest uses RequireString but 'explanation' should be optional based on other tools. This could cause parsing failures for tools that don't require explanation.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 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
}

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): The parseBaseRequest function calls request.RequireString("explanation") but the explanation argument is not marked as required in the argument definitions in args.go. This inconsistency could cause runtime errors. Either make the explanation argument required or use GetString with a default value.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Function tries to use RequireString on explanation field but explanation is not marked as required in the argument definition

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): parseBaseRequest tries to use RequireString() for 'explanation' but according to the argument definition in args.go, the explanation argument is not marked as Required(). This inconsistency could cause runtime errors when explanation is optional but the parser treats it as required.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): The parseBaseRequest function uses RequireString but explanation is actually optional in the tools. This could cause unnecessary errors when explanation is not provided.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): The parseBaseRequest function uses RequireString for 'explanation' but the explainationArgument in args.go is not marked as Required(). This creates an inconsistency - either explanation should be required in the tool definition or should use GetString() with a default value here.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Inconsistent error handling: The function calls RequireString for explanation but explanation is not marked as required in the argument definitions. This will fail when explanation is not provided.

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Inconsistency: parseBaseRequest uses RequireString for 'explanation' field, but the argument definition in args.go doesn't mark it as required. This could cause runtime errors if explanation is optional in some tools.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (High): Missing import for fmt package - code uses fmt.Errorf but doesn't import fmt. This will cause compilation errors.

Suggested change
explanation, err := request.RequireString("explanation")
import (
"context"
"fmt"
"dagger.io/dagger"
"github.com/dagger/container-use/environment"
"github.com/dagger/container-use/repository"
"github.com/mark3labs/mcp-go/mcp"
)

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): parseBaseRequest uses RequireString for 'explanation' but the args.go shows it's optional (no Required() call). This inconsistency could cause runtime errors.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")
return BaseRequest{
Explanation: explanation,
}, nil

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Inconsistent error handling: parseBaseRequest calls RequireString for explanation field, but this should be optional based on the tool definitions using GetString with default values

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

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Using RequireString for 'explanation' field but it's not required according to args.go. This will cause errors when explanation is not provided.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Inconsistent parsing behavior: parseBaseRequest uses RequireString for explanation, but in args.go, explainationArgument is defined without Required(). This creates inconsistency between tool definition and parsing logic.

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

Copy link

Choose a reason for hiding this comment

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

🔧 Maintainability (Low): Inconsistent error handling: parseBaseRequest() uses RequireString() for explanation field, but the args.go file defines it as optional (no mcp.Required()). Consider making this consistent - either make explanation required in both places or use GetString() with a default value here.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")
return BaseRequest{
Explanation: explanation,
}, nil

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Inconsistent parameter requirement: The explanation parameter is now treated as required with RequireString, but in the original implementation it was optional (using GetString with a default). This changes the API behavior and could break existing clients. Consider using GetString instead to maintain backward compatibility.

Suggested change
explanation, err := request.RequireString("explanation")
explanation := request.GetString("explanation", "")

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Remove error handling: Since explanation is now optional, we don't need error handling for it.

Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Remove the error handling since explanation should be optional.

return BaseRequest{}, err
Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Remove the error return since explanation should be optional.

}
return BaseRequest{
Copy link

Choose a reason for hiding this comment

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

🐛 Bug (Medium): Update the return statement to match the simplified explanation handling.

Suggested change
return BaseRequest{
return BaseRequest{
Explanation: explanation,
}, nil

Explanation: explanation,
}, nil
}

func parseBaseRepositoryRequest(request mcp.CallToolRequest) (BaseRepositoryRequest, error) {
base, err := parseBaseRequest(request)
if err != nil {
return BaseRepositoryRequest{}, err
}
environmentSource, err := request.RequireString("environment_source")
if err != nil {
return BaseRepositoryRequest{}, err
}
return BaseRepositoryRequest{
BaseRequest: base,
EnvironmentSource: environmentSource,
}, nil
}

func parseBaseEnvironmentRequest(request mcp.CallToolRequest) (BaseEnvironmentRequest, error) {
base, err := parseBaseRepositoryRequest(request)
if err != nil {
return BaseEnvironmentRequest{}, err
}
environmentID, err := request.RequireString("environment_id")
if err != nil {
return BaseEnvironmentRequest{}, err
}
return BaseEnvironmentRequest{
BaseRepositoryRequest: base,
EnvironmentID: environmentID,
}, nil
}
Loading