Model Context Protocol (MCP) server implementation #1444
Conversation
WalkthroughAdds a new "mcp" CLI subcommand wired into the root command, exposes Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Root as rootCmd
participant McpCmd as Mcp Command
participant Runner as runMcp()
participant Ctx as Context
participant MCP as MCPServer
participant Lock as DirLock
participant Stdio as StdioTransport
participant API as Local API
User->>Root: run "mcp"
Root->>McpCmd: execute
McpCmd->>Runner: invoke runMcp(ctx)
Runner->>Ctx: NewMcpServer()
Ctx->>MCP: New(cfg)
MCP-->>Runner: MCPServer instance
Runner->>MCP: Start(ctx)
MCP->>Lock: acquire lock on DAGs dir
alt lock acquired
MCP->>Stdio: serve over stdio transport
Stdio->>MCP: deliver tool requests
MCP->>API: fetch DAGs / DAG runs / DAG details
API-->>MCP: return data
MCP-->>Stdio: return tool responses
MCP-->>Runner: Start returns nil
else lock failed
MCP-->>Runner: Start returns error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/service/mcpserver/mcpserver.go (2)
20-27: Unused struct fields.The
logDir,running, andconfigfields are declared but never used. Either remove them or implement their intended functionality. Therunningfield withatomic.Boolsuggests there may be missing lifecycle management (e.g., graceful shutdown, status checks).type MCPServer struct { server *mcp.Server - logDir string - running atomic.Bool - config *config.Config dirLock dirlock.DirLock location *time.Location }
44-75: Arbitrary MaxLength constraint and unclear input schema.The
MaxLength: jsonschema.Ptr(10)constraint for thenameproperty seems too restrictive for DAG names. Additionally, the input schemas define anameproperty but the handlers never read it fromctr.Arguments.Consider removing the arbitrary length restriction or setting a more reasonable value, and ensure handlers actually use the input:
s.AddTool(&mcp.Tool{ Name: "List DAGs", Description: "List all the Direct Acyclic Graphs in the server", InputSchema: &jsonschema.Schema{ Type: "object", - Properties: map[string]*jsonschema.Schema{ - "name": {Type: "string", MaxLength: jsonschema.Ptr(10)}, - }, + Properties: map[string]*jsonschema.Schema{}, }, }, listDags)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/main.go(1 hunks)go.mod(2 hunks)internal/cmd/context.go(2 hunks)internal/cmd/mcp.go(1 hunks)internal/service/mcpserver/mcpserver.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
cmd/main.gointernal/cmd/mcp.gointernal/cmd/context.gointernal/service/mcpserver/mcpserver.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/cmd/context.go
🧬 Code graph analysis (4)
cmd/main.go (1)
internal/cmd/mcp.go (1)
Mcp(9-18)
internal/cmd/mcp.go (4)
internal/common/logger/tag/tag.go (1)
Args(408-410)internal/cmd/context.go (1)
Context(39-54)internal/common/logger/context.go (1)
Errorf(75-77)internal/common/config/path.go (1)
Paths(13-30)
internal/cmd/context.go (1)
internal/service/mcpserver/mcpserver.go (1)
MCPServer(20-27)
internal/service/mcpserver/mcpserver.go (3)
internal/common/config/config.go (1)
Global(38-69)internal/common/config/path.go (1)
Paths(13-30)internal/common/logger/context.go (1)
Info(40-42)
🔇 Additional comments (4)
cmd/main.go (1)
48-48: LGTM!The new
cmd.Mcp()command is registered following the established pattern for other commands.internal/cmd/context.go (2)
32-32: LGTM!The import for the mcpserver package is correctly added.
235-238: LGTM!The
NewMcpServer()method follows the established pattern of other factory methods inContext(e.g.,NewServer,NewScheduler).go.mod (1)
370-370: MCP Go SDK dependency is current. Thegithub.com/modelcontextprotocol/go-sdk v1.1.0is the latest stable release (October 30, 2025), so no version update is needed.
| func executeDag(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| // host := os.Getenv("DAGU_HOST") | ||
| host := "localhost" | ||
| // port := os.Getenv("DAGU_PORT") | ||
| port := "8080" | ||
| api_base_url := os.Getenv("DAGU_API_BASE_URL") | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) | ||
|
|
||
| return &mcp.CallToolResult{ | ||
| StructuredContent: resp.Body, | ||
| IsError: err == nil, | ||
| }, nil | ||
| } | ||
|
|
||
| func createDag(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| // host := os.Getenv("DAGU_HOST") | ||
| host := "localhost" | ||
| // port := os.Getenv("DAGU_PORT") | ||
| port := "8080" | ||
| api_base_url := os.Getenv("DAGU_API_BASE_URL") | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) | ||
|
|
||
| return &mcp.CallToolResult{ | ||
| StructuredContent: resp.Body, | ||
| IsError: err == nil, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
All handlers are identical placeholders with the same bugs.
executeDag and createDag have the same issues as listDags (nil dereference, inverted error logic, resource leak) and call the same GET endpoint. These should be distinct implementations:
executeDagshould POST to execute a specific DAGcreateDagshould POST to create a new DAG
Both handlers also ignore the ctr *mcp.CallToolRequest parameter which should contain the input arguments (e.g., DAG name).
Would you like me to generate proper implementations for these handlers that use the correct HTTP methods and parse input arguments from ctr?
…lowed + fixing the `err!=nil`
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
internal/service/mcpserver/mcpserver.go (4)
84-98: Critical: Nil pointer dereference and resource leak remain unresolved.This handler still has the same critical issues identified in the previous review:
- Line 95:
resp.Bodyis accessed without first checking iferr != nil, causing a nil pointer dereference when HTTP fails- Line 95:
resp.Bodyis anio.ReadCloser, not structured content—this will cause type errors- The response body is never closed, causing a resource leak
- The
ctxandctrparameters are unused (context for cancellation,ctrfor tool input arguments)Apply this diff:
func listDags(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // host := os.Getenv("DAGU_HOST") - host := "localhost" - // port := os.Getenv("DAGU_PORT") - port := "8080" - //api_base_url := os.Getenv("DAGU_API_BASE_URL") - api_base_url := "api/v2" + host := os.Getenv("DAGU_HOST") + if host == "" { + host = "localhost" + } + port := os.Getenv("DAGU_PORT") + if port == "" { + port = "8080" + } + apiBaseURL := os.Getenv("DAGU_API_BASE_URL") + if apiBaseURL == "" { + apiBaseURL = "api/v2" + } - resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://%s:%s/%s/dags", host, port, apiBaseURL), nil) + if err != nil { + return &mcp.CallToolResult{ + Content: []mcp.Content{mcp.TextContent{Text: fmt.Sprintf("Failed to create request: %v", err)}}, + IsError: true, + }, nil + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return &mcp.CallToolResult{ + Content: []mcp.Content{mcp.TextContent{Text: fmt.Sprintf("HTTP request failed: %v", err)}}, + IsError: true, + }, nil + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return &mcp.CallToolResult{ + Content: []mcp.Content{mcp.TextContent{Text: fmt.Sprintf("Failed to read response: %v", err)}}, + IsError: true, + }, nil + } return &mcp.CallToolResult{ - StructuredContent: resp.Body, - IsError: err != nil, + Content: []mcp.Content{mcp.TextContent{Text: string(body)}}, + IsError: false, }, nil }Note: Add
ioimport at the top of the file.
100-113: Critical: Inverted error logic and identical placeholder code.This handler still has the critical issues from the previous review:
- Line 111:
IsError: err == nilis inverted—this setsIsErrortotruewhen there's no error andfalsewhen there is an error- Line 107: Uses
http.Getbut should usePOSTto execute a DAG- Same nil pointer dereference, resource leak, and type mismatch issues as
listDags- The
ctrparameter is ignored but should contain the DAG name/parameters to executeThis remains a non-functional placeholder.
Would you like me to generate a proper implementation that:
- Parses the DAG name from
ctr.Arguments- Uses
POSTto the correct execution endpoint- Handles errors correctly
- Reads and closes the response body properly
115-128: Critical: Same inverted error logic and placeholder issues.This handler has identical critical issues to
executeDag:
- Line 126:
IsError: err == nilis inverted- Line 122: Uses
http.Getbut should usePOSTto create a DAG- Same nil pointer dereference, resource leak, and type mismatch issues
- Ignores
ctrwhich should contain the DAG definition/specAll three handlers (
listDags,executeDag,createDag) call the same GET endpoint and have similar critical bugs.Consider implementing these handlers properly or removing them entirely until they can be completed. Shipping non-functional placeholder code that appears to work but has inverted error handling is dangerous.
130-147: Critical: Multiple issues in Start() remain unaddressed.All the issues from the previous review are still present:
- Line 140: Log says "Acquired scheduler lock" but should be "Acquired mcp server lock"
- Line 142: Uses
context.Background()instead of the passedctx, completely ignoring cancellation—the server won't shut down whenctxis canceled- Line 143: Uses
log.Printfinstead of the project's structured logger (see coding guidelines requiringlogger.Error)- The directory lock is never released—missing
defer s.dirLock.Unlock()after line 138- Line 146: Always returns
nileven whenserver.Runfails at line 142Apply this diff:
func (s *MCPServer) Start(ctx context.Context) error { ctx, cancel := context.WithCancel(ctx) defer cancel() // Acquire directory lock first to prevent multiple mcp server instances logger.Info(ctx, "Waiting to acquire mcp server lock") if err := s.dirLock.Lock(ctx); err != nil { return fmt.Errorf("failed to acquire mcp server lock: %w", err) } + defer s.dirLock.Unlock() - logger.Info(ctx, "Acquired scheduler lock") + logger.Info(ctx, "Acquired mcp server lock") - if err := s.server.Run(context.Background(), &mcp.StdioTransport{}); err != nil { - log.Printf("Server failed: %v", err) + if err := s.server.Run(ctx, &mcp.StdioTransport{}); err != nil { + logger.Error(ctx, "MCP server failed", slog.String("error", err.Error())) + return fmt.Errorf("mcp server failed: %w", err) } return nil }Note: Add
log/slogimport forslog.String.As per coding guidelines: Use structured logging helpers in
internal/common, avoidlog.Printf.
🧹 Nitpick comments (3)
internal/service/mcpserver/mcpserver.go (3)
20-27: Multiple struct fields are unused.The
logDir,running, andconfigfields are initialized but never referenced after construction. This suggests either incomplete implementation or unnecessary fields.Consider:
- Remove unused fields if they're not needed
- Complete the implementation if these fields are intended for future use (e.g., the
runningatomic flag could be used for graceful shutdown)
35-38: Using scheduler lock configuration for MCP server locks.The MCP server reuses
cfg.Scheduler.LockStaleThresholdandcfg.Scheduler.LockRetryIntervalfor its own directory lock. This creates a semantic mismatch—MCP server lock behavior is coupled to scheduler tuning parameters.Consider adding dedicated MCP server lock configuration fields to
config.Config, or documenting that these scheduler settings apply system-wide to all locking mechanisms.
47-49: Empty input schema provides no validation.The
List_DAGstool has an empty object schema with no properties or validation rules. This means any input (or no input) will be accepted, which may not match the tool's requirements.If
List_DAGstruly requires no parameters, consider adding a comment explaining this. Otherwise, define the expected input properties and constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/mcpserver/mcpserver.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/mcpserver/mcpserver.go
🧬 Code graph analysis (1)
internal/service/mcpserver/mcpserver.go (3)
internal/common/config/config.go (1)
Global(38-69)internal/common/config/path.go (1)
Paths(13-30)internal/common/logger/context.go (1)
Info(40-42)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/cmd/mcp.go (1)
9-21: Cobra command wiring looks solid; consider tighteningArgsif no operands are expectedThe
Mcpcommand construction viaNewCommandandrunMcpis clean and idiomatic. If the MCP server does not currently accept positional operands, you might want to switchArgsfromcobra.ArbitraryArgstocobra.NoArgs(or a stricter validator) to fail fast on accidental extra arguments; otherwise this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cmd/mcp.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/cmd/mcp.go
🧬 Code graph analysis (1)
internal/cmd/mcp.go (2)
internal/cmd/context.go (1)
Context(39-54)internal/common/config/path.go (1)
Paths(13-30)
🔇 Additional comments (1)
internal/cmd/mcp.go (1)
23-36: runMcp flow and error wrapping look correct
runMcpcleanly separates initialization and startup, wraps errors with context (including DAGs directory), and passes the commandContextas acontext.ContexttoStart, which aligns with the embedded-context design ofContext. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/service/mcpserver/mcpserver.go (2)
52-75: Remove large commented‑out tool implementations before merging.The commented
executeDag/createDagtools and theirAddToolregistrations are effectively dead code and add noise to this new package. Version control already preserves these snippets if you need them later.Either:
- Implement them fully now, or
- Track follow‑ups in an issue and delete the commented blocks from this file.
This keeps the MCP server implementation focused and easier to maintain.
Also applies to: 108-140
141-157: Release dir lock, honor caller context, and propagate server errors inStart.
Startstill has a few behavioral issues:
- The directory lock is never released – once acquired,
s.dirLock.Lockis effectively permanent, blocking future MCP server instances.context.WithCancel(ctx)is created but the derivedctxis not passed tos.server.Run; insteadcontext.Background()is used, so caller cancellation is ignored.- Errors from
s.server.Runare logged but then discarded;Startalways returnsnil, so callers can’t tell that startup/serve failed.A minimal fix:
func (s *MCPServer) Start(ctx context.Context) error { ctx, cancel := context.WithCancel(ctx) defer cancel() // Acquire directory lock first to prevent multiple mcp server instances logger.Info(ctx, "Waiting to acquire mcp server lock") if err := s.dirLock.Lock(ctx); err != nil { return fmt.Errorf("failed to acquire mcp server lock: %w", err) } - logger.Info(ctx, "Acquired mcp server lock") - - if err := s.server.Run(context.Background(), &mcp.StdioTransport{}); err != nil { - logger.Error(ctx, "MCP server failed", slog.String("error", err.Error())) - } - - return nil + defer s.dirLock.Unlock() + + logger.Info(ctx, "Acquired mcp server lock") + + if err := s.server.Run(ctx, &mcp.StdioTransport{}); err != nil { + logger.Error(ctx, "MCP server failed", slog.String("error", err.Error())) + return fmt.Errorf("mcp server failed: %w", err) + } + + return nil }This way the lock is always released, the server respects the caller’s context, and failures are visible to the caller.
🧹 Nitpick comments (1)
internal/service/mcpserver/mcpserver.go (1)
20-27: Align MCPServer fields with constructor usage (avoid unused config/logDir).
MCPServerdeclaresconfigandlogDir, butNewonly initializeslogDirand never setsconfig. Neither field is used anywhere yet. This is misleading and makes it harder to see what state the server actually relies on.Consider either:
- Wiring
cfginto the struct (e.g.,config: cfg) and actually using it for things like host/port/API base URL, or- Dropping the unused fields until they’re needed.
Also, for consistency with Go naming conventions and the rest of the file, prefer lowerCamelCase for locals (e.g.,
apiBaseURLinstead ofapi_base_url) when you do start threading config into the handlers.Also applies to: 29-42, 76-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/mcpserver/mcpserver.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/mcpserver/mcpserver.go
🧬 Code graph analysis (1)
internal/service/mcpserver/mcpserver.go (4)
internal/common/config/config.go (1)
Global(38-69)internal/common/config/path.go (1)
Paths(13-30)internal/common/logger/tag/tag.go (3)
Name(271-273)Type(266-268)String(15-17)internal/common/logger/context.go (2)
Info(40-42)Errorf(75-77)
🔇 Additional comments (1)
internal/service/mcpserver/mcpserver.go (1)
84-106: Tighten HTTP error handling and IsError semantics inlistDags.The handler currently:
- Ignores
errfromio.ReadAllexcept viaIsError: err != nil, but still returns a successful result with a possibly empty/partial body.- Treats all HTTP status codes the same—non‑2xx responses are surfaced as success (unless
io.ReadAllitself fails).- Returns
nilGo error even whenIsErroristrue, which is OK for MCP but only if theCallToolResultactually contains a clear error message.This can easily lead to silent failures from the client's perspective.
A safer pattern is to treat HTTP/read failures and non‑2xx responses as tool‑level errors and return an explicit error message in
Content, while keeping the Go errornil. For example:func listDags(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - host := "localhost" - port := "8080" - api_base_url := "api/v2" - - resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) + host := "localhost" + port := "8080" + apiBaseURL := "api/v2" + + url := fmt.Sprintf("http://%s:%s/%s/dags", host, port, apiBaseURL) + resp, err := http.Get(url) if err != nil { - return nil, err + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("failed to GET %s: %v", url, err)}, + }, + }, nil } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) - - return &mcp.CallToolResult{ - Content: []mcp.Content{ - &mpc.TextContent{Text: string(body)}, - }, - IsError: err != nil, - }, nil + if err != nil { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("failed to read DAG list: %v", err)}, + }, + }, nil + } + + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{ + &mcp.TextContent{ + Text: fmt.Sprintf("list_DAGs HTTP %d: %s", resp.StatusCode, string(body)), + }, + }, + }, nil + } + + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: string(body)}, + }, + IsError: false, + }, nil }Also use
http.NewRequestWithContextso the HTTP call respectsctx.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/service/mcpserver/mcpserver.go (2)
52-75: Remove large commented-out tool and handler code before merging.There are two sizeable commented blocks (the
execute_DAG/create_DAGtool registrations and their handler implementations). Keeping this in the main source file makes it harder to see what’s actually supported and clutters the MCP surface.Either:
- Implement these tools fully in this PR, or
- Track them in an issue and delete the commented-out code (VCS history preserves it).
This aligns with the earlier review feedback about commented-out MCP tools.
Also applies to: 108-140
141-159: HaveStartpass the caller’s context intos.server.Run(notcontext.Background).
Startcorrectly usesctxfor logging and acquiring the directory lock, buts.server.Runis invoked withcontext.Background(). That means CLI cancellation (or any upstream context timeout) won’t stop the MCP server; only process termination will.Given that you already wrap the incoming
ctxwithcontext.WithCancel, you should pass that samectxintoRun:func (s *MCPServer) Start(ctx context.Context) error { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ - if err := s.server.Run(context.Background(), &mcp.StdioTransport{}); err != nil { + if err := s.server.Run(ctx, &mcp.StdioTransport{}); err != nil { logger.Error(ctx, "MCP server failed", slog.String("error", err.Error())) return fmt.Errorf("mcp server failed: %w", err) }This aligns the MCP server’s lifecycle with the CLI context and with the earlier review suggestion for this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/mcpserver/mcpserver.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/mcpserver/mcpserver.go
🧬 Code graph analysis (1)
internal/service/mcpserver/mcpserver.go (3)
internal/common/config/config.go (1)
Global(38-69)internal/common/config/path.go (1)
Paths(13-30)internal/common/logger/context.go (1)
Info(40-42)
🔇 Additional comments (1)
internal/service/mcpserver/mcpserver.go (1)
44-50:list_DAGstool registration looks correct.The tool registration and minimal input schema (
Type: "object"with no properties) are consistent with a zero-argument tool and line up with the handler signature. No issues here.
| func listDags(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| // host := os.Getenv("DAGU_HOST") | ||
| host := "localhost" | ||
| // port := os.Getenv("DAGU_PORT") | ||
| port := "8080" | ||
| //api_base_url := os.Getenv("DAGU_API_BASE_URL") | ||
| api_base_url := "api/v2" | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
|
|
||
| return &mcp.CallToolResult{ | ||
| Content: []mcp.Content{ | ||
| &mcp.TextContent{Text: string(body)}, | ||
| }, | ||
| IsError: err != nil, | ||
| }, nil |
There was a problem hiding this comment.
Handle io.ReadAll errors and HTTP status properly in listDags.
listDags has a couple of correctness issues:
- The error from
io.ReadAllis ignored except for being used to setIsError, but the function still returns an apparently “successful” result with possibly empty/partial content. - HTTP status codes are not checked, so a 404/500 response will be treated as
IsError == falseas long asio.ReadAllsucceeds.
A safer version is:
func listDags(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
@@
- resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url))
+ resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url))
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to call dags endpoint: %w", err)
}
defer resp.Body.Close()
- body, err := io.ReadAll(resp.Body)
-
- return &mcp.CallToolResult{
- Content: []mcp.Content{
- &mcp.TextContent{Text: string(body)},
- },
- IsError: err != nil,
- }, nil
+ body, err := io.ReadAll(resp.Body)
+ if err != nil {
+ return nil, fmt.Errorf("failed to read dags response body: %w", err)
+ }
+
+ if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+ return &mcp.CallToolResult{
+ Content: []mcp.Content{
+ &mcp.TextContent{Text: fmt.Sprintf("dags request failed with status %d: %s", resp.StatusCode, string(body))},
+ },
+ IsError: true,
+ }, nil
+ }
+
+ return &mcp.CallToolResult{
+ Content: []mcp.Content{
+ &mcp.TextContent{Text: string(body)},
+ },
+ IsError: false,
+ }, nil
}(Optional) You may also want to switch to an HTTP request bound to ctx so MCP cancellation can cancel the HTTP call:
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
...
resp, err := http.DefaultClient.Do(req)🤖 Prompt for AI Agents
In internal/service/mcpserver/mcpserver.go around lines 84 to 105, the function
listDags currently ignores and mishandles errors from io.ReadAll and does not
check HTTP response status codes; update it to create the GET request with
context (http.NewRequestWithContext(ctx,...)), execute it with
http.DefaultClient.Do, check resp.StatusCode and return an error for non-2xx
responses (closing body), then call io.ReadAll and if that returns an error
propagate that error instead of returning a successful CallToolResult; only
build and return the CallToolResult when the HTTP status is OK and the body read
succeeded, setting IsError=false, otherwise return nil and the error.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
internal/service/mcpserver/mcpserver.go (4)
129-151: Apply the same fixes aslistDags.This function has identical issues to
listDags(lines 105-127):
- Hardcoded configuration values
- No HTTP status code validation
- Incomplete error handling for
io.ReadAllApply the same refactoring pattern suggested for
listDags.
153-183: Apply the same fixes aslistDags.This function has the same issues as
listDagsandlistDagRuns:
- Hardcoded configuration values (lines 158-163)
- No HTTP status code validation
- Incomplete error handling for
io.ReadAll(line 175-181)Apply the same refactoring pattern, ensuring the context-aware request is used and all errors are properly handled.
97-102: Fix field nameLogDir→LogsDirand initializeconfigfield.Two critical issues:
Line 99 uses
cfg.Paths.LogDir, but thePathsstruct definesLogsDirnotLogDir(seeinternal/common/config/path.go). This will cause a compilation error.The
configfield is not initialized, leaving itnil. If any future code attempts to accesss.config, it will panic.Apply this diff:
return &MCPServer{ server: s, - logDir: cfg.Paths.LogDir, + logDir: cfg.Paths.LogsDir, location: timeLoc, dirLock: dirLock, + config: cfg, }, nilBased on PR objectives feedback from reviewer jyroscoped.
105-127: Replace hardcoded configuration and improve error handling.Several issues in this handler:
Hardcoded values (lines 106-111):
host,port, andapi_base_urlare hardcoded with commented-out environment variable lookups. This prevents users from configuring the server endpoint.Missing HTTP status check: The function doesn't verify
resp.StatusCode, so a 404 or 500 response will be treated as success as long asio.ReadAlldoesn't fail.Incomplete error handling (line 119-125): When
io.ReadAllfails, the function returns a result withIsError: truebut includes potentially empty content. It should return an error instead.Consider this approach:
func listDags(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // host := os.Getenv("DAGU_HOST") - host := "localhost" - // port := os.Getenv("DAGU_PORT") - port := "8080" - //api_base_url := os.Getenv("DAGU_API_BASE_URL") - api_base_url := "api/v2" + host := os.Getenv("DAGU_HOST") + if host == "" { + host = "localhost" + } + port := os.Getenv("DAGU_PORT") + if port == "" { + port = "8080" + } + apiBaseURL := os.Getenv("DAGU_API_BASE_URL") + if apiBaseURL == "" { + apiBaseURL = "api/v2" + } - resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) + url := fmt.Sprintf("http://%s:%s/%s/dags", host, port, apiBaseURL) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + + resp, err := http.DefaultClient.Do(req) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to call dags endpoint: %w", err) } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("request failed with status %d: %s", resp.StatusCode, string(body))}, + }, + IsError: true, + }, nil + } return &mcp.CallToolResult{ Content: []mcp.Content{ &mcp.TextContent{Text: string(body)}, }, - IsError: err != nil, + IsError: false, }, nil }Based on PR objectives feedback from reviewer jyroscoped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cmd/mcp.go(1 hunks)internal/service/mcpserver/mcpserver.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/mcpserver/mcpserver.gointernal/cmd/mcp.go
🧬 Code graph analysis (2)
internal/service/mcpserver/mcpserver.go (4)
internal/common/config/config.go (1)
Global(52-80)internal/common/config/path.go (1)
Paths(13-30)internal/common/logger/tag/tag.go (3)
Name(271-273)Type(266-268)String(15-17)internal/common/logger/context.go (2)
Info(40-42)Errorf(75-77)
internal/cmd/mcp.go (2)
internal/common/config/path.go (1)
Paths(13-30)internal/cmd/context.go (1)
NewCommand(326-362)
🔇 Additional comments (1)
internal/cmd/mcp.go (1)
23-36: LGTM!The function properly initializes and starts the MCP server with appropriate error handling and contextual error messages.
| // Command line flags for the mcp command | ||
| var mcpFlags = []commandLineFlag{} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add configuration flags for MCP server settings.
The mcpFlags slice is currently empty, which means users cannot configure the MCP server via command-line arguments. The handlers in mcpserver.go use hardcoded values (host: "localhost", port: "8080", api_base_url: "api/v2").
Consider adding flags such as:
--hostor--mcp-hostfor the server host--portor--mcp-portfor the server port--api-base-urlfor the API base path
These flags would then be passed to the MCP server constructor or stored in the config.
Based on PR objectives feedback from reviewer jyroscoped.
🤖 Prompt for AI Agents
In internal/cmd/mcp.go around lines 20 to 21, mcpFlags is empty so the MCP
server cannot be configured via CLI; add command-line flags for host (e.g.
--mcp-host, default "localhost"), port (e.g. --mcp-port, default "8080"), and
api-base-url (e.g. --api-base-url, default "api/v2") to the mcpFlags slice with
appropriate types and help strings, wire those flag values into the MCP server
constructor or global config where the hardcoded values are used, and update any
usage/help text to reflect the new flags.
| type MCPServer struct { | ||
| server *mcp.Server | ||
| logDir string | ||
| running atomic.Bool | ||
| config *config.Config | ||
| dirLock dirlock.DirLock | ||
| location *time.Location | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused running field and initialize config field.
Two issues with the struct fields:
-
The
runningatomic.Bool field is declared but never used. Consider removing it unless there are plans to implement start/stop tracking. -
The
configfield is declared but never initialized in the constructor (see line 97-102), which will cause it to benil. Either initialize it inNew()or remove it if unused.
Based on PR objectives feedback from reviewer jyroscoped.
🤖 Prompt for AI Agents
In internal/service/mcpserver/mcpserver.go around lines 21 to 28 (and note
constructor at ~97-102), the struct declares an unused atomic.Bool field
`running` and a `config` pointer that is never initialized; remove the `running`
field entirely if start/stop tracking is not implemented, and either set
`m.config = cfg` (or the appropriate config variable passed into New) in the
New() constructor to avoid a nil pointer, or remove the `config` field if it is
not used elsewhere.
| /* | ||
| s.AddTool(&mcp.Tool{ | ||
| Name: "execute_DAG", | ||
| Description: "Execute a specific Workflow", | ||
| InputSchema: &jsonschema.Schema{ | ||
| Type: "object", | ||
| Properties: map[string]*jsonschema.Schema{ | ||
| "name": {Type: "string", MaxLength: jsonschema.Ptr(256)}, | ||
| }, | ||
| }, | ||
| }, executeDag) | ||
|
|
||
|
|
||
| s.AddTool(&mcp.Tool{ | ||
| Name: "create_DAG", | ||
| Description: "Create a new workflow", | ||
| InputSchema: &jsonschema.Schema{ | ||
| Type: "object", | ||
| Properties: map[string]*jsonschema.Schema{ | ||
| "name": {Type: "string", MaxLength: jsonschema.Ptr(10)}, | ||
| }, | ||
| }, | ||
| }, createDag) | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code before merging.
Large blocks of commented-out code for execute_DAG and create_DAG tools should be removed. Per coding guidelines, avoid leaving unused code and debug artifacts in the codebase.
If these features are planned for future implementation, track them in an issue and remove the commented code. Version control preserves the code history.
Based on coding guidelines and PR objectives feedback from reviewer jyroscoped.
Also applies to: 185-217
🤖 Prompt for AI Agents
In internal/service/mcpserver/mcpserver.go around lines 73-96 (and similarly
lines 185-217), remove the large commented-out blocks that define the
execute_DAG and create_DAG tools; delete these commented code sections entirely
before merging and, if needed for future work, create an issue or link to a
ticket that captures the intent so the code history preserves it in VCS.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1444 +/- ##
==========================================
- Coverage 59.95% 59.58% -0.38%
==========================================
Files 191 193 +2
Lines 21473 21615 +142
==========================================
+ Hits 12875 12880 +5
- Misses 7249 7386 +137
Partials 1349 1349
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
internal/service/mcpserver/mcpserver.go (4)
21-28: Remove unused field and initialize config field.Two struct field issues remain unaddressed:
- The
runningatomic.Bool field is declared but never used anywhere in the code.- The
configfield is declared but never initialized in theNew()constructor (lines 97-102), leaving itnil.Based on PR objectives feedback from reviewer jyroscoped.
Apply this diff:
type MCPServer struct { server *mcp.Server logDir string - running atomic.Bool config *config.Config dirLock dirlock.DirLock location *time.Location }And in the
New()function at line 97:return &MCPServer{ server: s, logDir: cfg.Paths.LogDir, location: timeLoc, dirLock: dirLock, + config: cfg, }, nil
73-96: Remove commented-out tool registrations.The commented-out
execute_DAGandcreate_DAGtool registrations should be removed before merging. Per coding guidelines, avoid leaving unused code and debug artifacts in the codebase.If these features are planned for future implementation, track them in an issue and remove the commented code. Version control preserves the history.
Based on coding guidelines and PR objectives feedback from reviewer jyroscoped.
105-127: Fix hardcoded values, error handling, and missing HTTP status checks.Multiple issues remain in this handler:
- Hardcoded connection parameters (lines 107, 109, 111): Host, port, and API base URL are hardcoded with environment variable lookups commented out, preventing configuration flexibility.
- Incomplete error handling (line 119): When
io.ReadAllfails, the function returns a result withIsError: trueinstead of propagating the error, which masks the failure.- Missing HTTP status validation: Non-2xx responses (404, 500, etc.) are not checked, treating errors as successful responses.
- Context not used: The HTTP request doesn't use the provided
ctx, preventing cancellation.Based on PR objectives feedback from reviewer jyroscoped and past review comments.
Apply this diff to address all issues:
func listDags(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // host := os.Getenv("DAGU_HOST") - host := "localhost" - // port := os.Getenv("DAGU_PORT") - port := "8080" - //api_base_url := os.Getenv("DAGU_API_BASE_URL") - api_base_url := "api/v2" + host := os.Getenv("DAGU_HOST") + if host == "" { + host = "localhost" + } + port := os.Getenv("DAGU_PORT") + if port == "" { + port = "8080" + } + apiBaseURL := os.Getenv("DAGU_API_BASE_URL") + if apiBaseURL == "" { + apiBaseURL = "api/v2" + } - resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags", host, port, api_base_url)) + url := fmt.Sprintf("http://%s:%s/%s/dags", host, port, apiBaseURL) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + + resp, err := http.DefaultClient.Do(req) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to call dags endpoint: %w", err) } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("request failed with status %d: %s", resp.StatusCode, string(body))}, + }, + IsError: true, + }, nil + } return &mcp.CallToolResult{ Content: []mcp.Content{ &mcp.TextContent{Text: string(body)}, }, - IsError: err != nil, + IsError: false, }, nil }
185-217: Remove commented-out handler implementations.The commented-out
executeDagandcreateDagfunctions should be removed before merging. Per coding guidelines, avoid leaving unused code and debug artifacts.If these features are planned for future implementation, track them in an issue. Version control preserves the code history.
Based on coding guidelines and PR objectives feedback from reviewer jyroscoped.
🧹 Nitpick comments (1)
internal/service/mcpserver/mcpserver.go (1)
105-183: Consider extracting repeated connection setup into a helper.All three handlers (
listDags,listDagRuns,getDag) repeat the same pattern for reading connection parameters from environment variables. Consider extracting this into a helper function or initializing these values once in theMCPServerstruct (from theconfigfield once it's initialized).Example helper:
func getAPIBaseURL() (host, port, apiBaseURL string) { host = os.Getenv("DAGU_HOST") if host == "" { host = "localhost" } port = os.Getenv("DAGU_PORT") if port == "" { port = "8080" } apiBaseURL = os.Getenv("DAGU_API_BASE_URL") if apiBaseURL == "" { apiBaseURL = "api/v2" } return }Or store these in the
MCPServerstruct during initialization.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/main.go(1 hunks)go.mod(2 hunks)internal/cmd/context.go(2 hunks)internal/service/mcpserver/mcpserver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/main.go
- go.mod
- internal/cmd/context.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/mcpserver/mcpserver.go
🧬 Code graph analysis (1)
internal/service/mcpserver/mcpserver.go (3)
internal/common/config/path.go (1)
Paths(13-30)internal/common/logger/tag/tag.go (3)
Name(271-273)Type(266-268)String(15-17)internal/common/logger/context.go (1)
Info(40-42)
🔇 Additional comments (1)
internal/service/mcpserver/mcpserver.go (1)
218-237: LGTM! Context is now properly propagated.The
Startmethod correctly:
- Creates a child context with cancellation
- Acquires the directory lock to prevent multiple server instances
- Passes the context to
s.server.Run()(line 231), allowing proper cancellation- Handles errors and cleanup appropriately
The previous issue with using
context.Background()has been addressed in commit 88c0568.
| func listDagRuns(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| // host := os.Getenv("DAGU_HOST") | ||
| host := "localhost" | ||
| // port := os.Getenv("DAGU_PORT") | ||
| port := "8080" | ||
| //api_base_url := os.Getenv("DAGU_API_BASE_URL") | ||
| api_base_url := "api/v2" | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dag-runs", host, port, api_base_url)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
|
|
||
| return &mcp.CallToolResult{ | ||
| Content: []mcp.Content{ | ||
| &mcp.TextContent{Text: string(body)}, | ||
| }, | ||
| IsError: err != nil, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Apply the same fixes as listDags.
This handler has identical issues to listDags: hardcoded connection parameters, incomplete error handling, missing HTTP status validation, and unused context.
Apply this diff:
func listDagRuns(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
- // host := os.Getenv("DAGU_HOST")
- host := "localhost"
- // port := os.Getenv("DAGU_PORT")
- port := "8080"
- //api_base_url := os.Getenv("DAGU_API_BASE_URL")
- api_base_url := "api/v2"
+ host := os.Getenv("DAGU_HOST")
+ if host == "" {
+ host = "localhost"
+ }
+ port := os.Getenv("DAGU_PORT")
+ if port == "" {
+ port = "8080"
+ }
+ apiBaseURL := os.Getenv("DAGU_API_BASE_URL")
+ if apiBaseURL == "" {
+ apiBaseURL = "api/v2"
+ }
- resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dag-runs", host, port, api_base_url))
+ url := fmt.Sprintf("http://%s:%s/%s/dag-runs", host, port, apiBaseURL)
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create request: %w", err)
+ }
+
+ resp, err := http.DefaultClient.Do(req)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to call dag-runs endpoint: %w", err)
}
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
+ if err != nil {
+ return nil, fmt.Errorf("failed to read response body: %w", err)
+ }
+
+ if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+ return &mcp.CallToolResult{
+ Content: []mcp.Content{
+ &mcp.TextContent{Text: fmt.Sprintf("request failed with status %d: %s", resp.StatusCode, string(body))},
+ },
+ IsError: true,
+ }, nil
+ }
return &mcp.CallToolResult{
Content: []mcp.Content{
&mcp.TextContent{Text: string(body)},
},
- IsError: err != nil,
+ IsError: false,
}, nil
}🤖 Prompt for AI Agents
internal/service/mcpserver/mcpserver.go around lines 129 to 151: this function
hardcodes host/port/api_base_url, ignores ctx, and fails to validate HTTP status
and handle read errors; change to read host/port/api_base_url from environment
with sensible defaults (like in listDags), create the GET request with
http.NewRequestWithContext(ctx, ...), use http.DefaultClient.Do(req) (or the
configured client), check resp.StatusCode and treat non-2xx as an error (include
status and response body in the error), handle and return any io.ReadAll error
instead of masking it, and ensure the returned CallToolResult sets IsError
appropriately when an error occurs and includes the actual error message rather
than returning nil error alongside an error flag.
| func getDag(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| var arg struct { | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| // host := os.Getenv("DAGU_HOST") | ||
| host := "localhost" | ||
| // port := os.Getenv("DAGU_PORT") | ||
| port := "8080" | ||
| //api_base_url := os.Getenv("DAGU_API_BASE_URL") | ||
| api_base_url := "api/v2" | ||
|
|
||
| if err := json.Unmarshal(ctr.Params.Arguments, &arg); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags/%s", host, port, api_base_url, arg.Name)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| defer resp.Body.Close() | ||
| body, err := io.ReadAll(resp.Body) | ||
|
|
||
| return &mcp.CallToolResult{ | ||
| Content: []mcp.Content{ | ||
| &mcp.TextContent{Text: string(body)}, | ||
| }, | ||
| IsError: err != nil, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Apply the same fixes as listDags and listDagRuns.
This handler has the same issues: hardcoded connection parameters, incomplete error handling, missing HTTP status validation, and unused context.
Apply this diff:
func getDag(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
var arg struct {
Name string `json:"name"`
}
- // host := os.Getenv("DAGU_HOST")
- host := "localhost"
- // port := os.Getenv("DAGU_PORT")
- port := "8080"
- //api_base_url := os.Getenv("DAGU_API_BASE_URL")
- api_base_url := "api/v2"
-
if err := json.Unmarshal(ctr.Params.Arguments, &arg); err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to unmarshal arguments: %w", err)
}
- resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags/%s", host, port, api_base_url, arg.Name))
+ host := os.Getenv("DAGU_HOST")
+ if host == "" {
+ host = "localhost"
+ }
+ port := os.Getenv("DAGU_PORT")
+ if port == "" {
+ port = "8080"
+ }
+ apiBaseURL := os.Getenv("DAGU_API_BASE_URL")
+ if apiBaseURL == "" {
+ apiBaseURL = "api/v2"
+ }
+
+ url := fmt.Sprintf("http://%s:%s/%s/dags/%s", host, port, apiBaseURL, arg.Name)
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create request: %w", err)
+ }
+
+ resp, err := http.DefaultClient.Do(req)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to call dag endpoint: %w", err)
}
-
defer resp.Body.Close()
+
body, err := io.ReadAll(resp.Body)
+ if err != nil {
+ return nil, fmt.Errorf("failed to read response body: %w", err)
+ }
+
+ if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+ return &mcp.CallToolResult{
+ Content: []mcp.Content{
+ &mcp.TextContent{Text: fmt.Sprintf("request failed with status %d: %s", resp.StatusCode, string(body))},
+ },
+ IsError: true,
+ }, nil
+ }
return &mcp.CallToolResult{
Content: []mcp.Content{
&mcp.TextContent{Text: string(body)},
},
- IsError: err != nil,
+ IsError: false,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getDag(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | |
| var arg struct { | |
| Name string `json:"name"` | |
| } | |
| // host := os.Getenv("DAGU_HOST") | |
| host := "localhost" | |
| // port := os.Getenv("DAGU_PORT") | |
| port := "8080" | |
| //api_base_url := os.Getenv("DAGU_API_BASE_URL") | |
| api_base_url := "api/v2" | |
| if err := json.Unmarshal(ctr.Params.Arguments, &arg); err != nil { | |
| return nil, err | |
| } | |
| resp, err := http.Get(fmt.Sprintf("http://%s:%s/%s/dags/%s", host, port, api_base_url, arg.Name)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| return &mcp.CallToolResult{ | |
| Content: []mcp.Content{ | |
| &mcp.TextContent{Text: string(body)}, | |
| }, | |
| IsError: err != nil, | |
| }, nil | |
| } | |
| func getDag(ctx context.Context, ctr *mcp.CallToolRequest) (*mcp.CallToolResult, error) { | |
| var arg struct { | |
| Name string `json:"name"` | |
| } | |
| if err := json.Unmarshal(ctr.Params.Arguments, &arg); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal arguments: %w", err) | |
| } | |
| host := os.Getenv("DAGU_HOST") | |
| if host == "" { | |
| host = "localhost" | |
| } | |
| port := os.Getenv("DAGU_PORT") | |
| if port == "" { | |
| port = "8080" | |
| } | |
| apiBaseURL := os.Getenv("DAGU_API_BASE_URL") | |
| if apiBaseURL == "" { | |
| apiBaseURL = "api/v2" | |
| } | |
| url := fmt.Sprintf("http://%s:%s/%s/dags/%s", host, port, apiBaseURL, arg.Name) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create request: %w", err) | |
| } | |
| resp, err := http.DefaultClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to call dag endpoint: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response body: %w", err) | |
| } | |
| if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { | |
| return &mcp.CallToolResult{ | |
| Content: []mcp.Content{ | |
| &mcp.TextContent{Text: fmt.Sprintf("request failed with status %d: %s", resp.StatusCode, string(body))}, | |
| }, | |
| IsError: true, | |
| }, nil | |
| } | |
| return &mcp.CallToolResult{ | |
| Content: []mcp.Content{ | |
| &mcp.TextContent{Text: string(body)}, | |
| }, | |
| IsError: false, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In internal/service/mcpserver/mcpserver.go around lines 153 to 183, this handler
repeats issues from listDags/listDagRuns: replace hardcoded
host/port/api_base_url with os.Getenv lookups and sensible defaults, build an
http.Request with http.NewRequestWithContext(ctx, "GET", url, nil) so the
provided ctx is used, perform the http.Client.Do call and check for network
error, validate the HTTP response status (treat non-2xx as an error and return
it), defer resp.Body.Close() after ensuring resp != nil, read the body and
return any read error instead of swallowing it, and propagate useful errors back
to the caller (don’t set IsError based only on the read error); return a non-nil
error on failures so callers can handle them.
yottahmd
force-pushed
the
main
branch
3 times, most recently
from
ac98dda to
887355f
Compare
2 participants
Implementation of an MCP server for Dagu #976
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.