add mcp sub cmd#502
Open
skeeey wants to merge 1 commit intoopenshift-online:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new "mcp" Cobra subcommand and a stdio-based Maestro MCP server exposing 12 tools (7 resource-bundle, 5 consumer). Wires REST and gRPC clients, implements tool handlers and registration, includes extensive unit tests, adds CLI docs, and updates module dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as Maestro CLI
participant MCP as Maestro MCP Server
participant Tools as Tool Handlers
participant REST as REST API
participant GRPC as gRPC API
User->>CLI: run `maestro mcp`
CLI->>MCP: Start(ctx)
MCP->>Tools: registerTools(GetMaestroTools)
MCP->>MCP: Serve on stdio
Note over User,MCP: runtime tool calls over stdio
User->>MCP: CallTool request
MCP->>Tools: Dispatch handler (by name)
Tools->>REST: Read operations (list/get/search)
Tools->>GRPC: Mutations (apply/delete)
REST-->>Tools: REST response
GRPC-->>Tools: gRPC response
Tools-->>MCP: CallToolResult
MCP-->>User: Tool result
User->>CLI: Interrupt (SIGINT)
CLI->>MCP: Stop()
MCP->>GRPC: Close client
MCP->>REST: Close client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (12)
docs/cli/mcp.md (2)
421-443: Add language specifiers to conversational workflow code blocks.These example conversation blocks should have a language specifier (e.g.,
textorplaintext) for consistent rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/mcp.md` around lines 421 - 443, The example conversation code blocks (showing the User/Claude exchanges that call maestro_create_consumer, maestro_apply_resource_bundle, and maestro_get_resource_bundle_status) lack a language specifier; update each triple-backtick fence around those conversation examples to include a language tag such as "text" or "plaintext" (e.g., ```text) so the blocks render consistently and plainly across documentation viewers.
62-71: Add language specifier to ASCII diagram code block.The architecture diagram code block lacks a language specifier. Consider using
textorplaintextfor ASCII art diagrams to satisfy markdown linters.📝 Proposed fix
-``` +```text ┌─────────────────┐ ┌──────────────────┐ ┌─────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/mcp.md` around lines 62 - 71, The code block containing the ASCII architecture diagram is missing a language specifier; update the opening fence for the diagram from ``` to a plaintext specifier (e.g., ```text or ```plaintext) so markdown linters treat it as literal text; locate the ASCII block shown with the MCP Server / Maestro API diagram and replace its opening triple backticks with ```text (or ```plaintext) while leaving the diagram content and closing fence unchanged.cmd/maestro/mcp/tools/resource_bundles_test.go (1)
15-425: Consider adding tests for Apply and Delete handlers.The test file covers List, Get, Status, Search, and ListByConsumer handlers, but is missing tests for
HandleApplyResourceBundleandHandleDeleteResourceBundle. These are important mutation operations that should have test coverage.Would you like me to generate test cases for
HandleApplyResourceBundleandHandleDeleteResourceBundle?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles_test.go` around lines 15 - 425, Add unit tests for the missing mutation handlers: create TestResourceBundleTools_HandleApplyResourceBundle and TestResourceBundleTools_HandleDeleteResourceBundle using the same setup pattern (mock.NewMaestroServer, clients.NewRESTClient, NewResourceBundleTools) and invoking tools.HandleApplyResourceBundle and tools.HandleDeleteResourceBundle via mcp.CallToolRequest; for Apply include at minimum: successful apply (valid bundle payload), validation error (missing/invalid args), and server error response; for Delete include: successful delete (valid "id"), delete non-existent id (expect error containing "not found"), and missing/empty "id" parameter (expect error mentioning "id parameter is required"); assert results by checking result.IsError and using getResultText to verify error messages consistent with other tests.cmd/maestro/mcp/cmd.go (1)
91-104: Consider returning server errors instead of nil.When a server error occurs (line 96-97), the function logs the error but returns
nil, making it appear successful to the caller. Additionally, server errors are logged twice (line 81 and line 97).♻️ Proposed fix
+ var serverErr error select { case <-sigCh: klog.Info("Received interrupt signal, shutting down gracefully...") case <-ctx.Done(): klog.Info("Context cancelled, shutting down...") case err := <-errCh: - klog.Errorf("Server error: %v", err) + serverErr = err } // Stop the server mcpServer.Stop() klog.Info("MCP server stopped successfully") - return nil + return serverErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/cmd.go` around lines 91 - 104, The select currently logs server errors from errCh and then returns nil, and also logs the same error elsewhere; change the err handling to return the error (or a wrapped error) instead of returning nil and avoid duplicate logging: in the select case for "case err := <-errCh" call mcpServer.Stop() (or ensure mcpServer.Stop() is deferred earlier) and then return err (or fmt.Errorf("server error: %w", err)); reference the errCh receive case, the mcpServer.Stop() call, and the sigCh / ctx.Done() branches so the function consistently stops the server and returns the actual error to the caller.go.mod (1)
26-26: Update mcp-go to the latest version v0.45.0.The
mcp-golibrary is currently pinned to v0.44.1, but v0.45.0 is the latest stable release (released March 6, 2026). Consider updating to the latest version to benefit from recent improvements and fixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 26, Update the mcp-go dependency from v0.44.1 to v0.45.0 by changing the version string for "github.com/mark3labs/mcp-go v0.44.1" to v0.45.0 in go.mod (or run: go get github.com/mark3labs/mcp-go@v0.45.0), then run go mod tidy to refresh go.sum and ensure the module graph is consistent; verify builds and tests pass referencing the updated dependency.cmd/maestro/mcp/server/server.go (3)
24-60: Consider nil-check for the config parameter.The function dereferences
cfg.RESTConfigand passescfgdirectly without validating thatcfgis non-nil, which could cause a panic.🛡️ Proposed fix to add nil check
// NewMaestroMCPServer creates a new Maestro MCP server instance func NewMaestroMCPServer(cfg *clients.Config) (*MaestroMCPServer, error) { + if cfg == nil { + return nil, fmt.Errorf("config is required") + } + // Create REST client (required) restClient, err := clients.NewRESTClient(&cfg.RESTConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 24 - 60, Add a nil-check at the start of NewMaestroMCPServer to validate cfg before dereferencing it: if cfg == nil return nil, fmt.Errorf("nil config"), then proceed to call clients.NewRESTClient(&cfg.RESTConfig) and clients.NewGRPCClient(cfg) as before; this prevents a potential panic when accessing cfg.RESTConfig and ensures callers get a clear error when cfg is missing.
62-99: DRY violation: tool-to-handler mapping is duplicated.The tool name → handler mapping is defined twice: once in
registerTools()(lines 68-84) and again inHandleToolCall()(lines 130-157). If a tool is added or renamed, both places must be updated, risking inconsistency.Since
registerTools()already registers handlers with the MCP server viaAddTool, and the MCP server likely dispatches calls internally, consider whetherHandleToolCallis needed at all. If it serves as a fallback entry point, consolidate the mapping into a shared data structure.♻️ Proposed consolidation approach
+// toolHandlers returns the mapping of tool names to their handlers +func (s *MaestroMCPServer) toolHandlers() map[string]server.ToolHandlerFunc { + return map[string]server.ToolHandlerFunc{ + // Resource Bundle Tools + "maestro_list_resource_bundles": s.resourceBundleTools.HandleListResourceBundles, + "maestro_get_resource_bundle": s.resourceBundleTools.HandleGetResourceBundle, + // ... other tools + } +} func (s *MaestroMCPServer) registerTools() error { toolDefs := tools.GetMaestroTools() - handlers := map[string]server.ToolHandlerFunc{ - // ... duplicated mapping - } + handlers := s.toolHandlers() // ... rest of registration } func (s *MaestroMCPServer) HandleToolCall(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - switch request.Params.Name { - // ... duplicated cases - } + handlers := s.toolHandlers() + if handler, ok := handlers[request.Params.Name]; ok { + return handler(ctx, request) + } + return mcp.NewToolResultError(fmt.Sprintf("unknown tool: %s", request.Params.Name)), nil }Also applies to: 127-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 62 - 99, The registerTools() and HandleToolCall() functions duplicate the tool→handler mapping; consolidate by centralizing the mapping into a single source (e.g., a package-level variable or a helper function like getToolHandlers()) that returns the map[string]server.ToolHandlerFunc built from s.resourceBundleTools and s.consumerTools and/or derived from tools.GetMaestroTools(); then have registerTools() iterate toolDefs and call s.mcpServer.AddTool(tool, handler) using that shared map and have HandleToolCall() look up handlers from the same shared map (or remove HandleToolCall entirely if s.mcpServer already dispatches calls), ensuring all references use the single canonical mapping to avoid desynchronization.
101-111: Remove the unusedctxparameter or document it for future use with server shutdown.The
ctxparameter is unused.mark3labs/mcp-go'sServeStdiodoes not accept a context argument—it only takes the server instance. Either remove the parameter or document it if you plan to useserver.Shutdown(ctx)for graceful shutdown in a future update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 101 - 111, The Start method on MaestroMCPServer currently declares an unused ctx parameter; remove the unused ctx from the Start signature (func (s *MaestroMCPServer) Start() error) and update all callers to invoke Start without a context, and remove any now-unused imports; alternatively, if you intend to support graceful shutdown later, document the parameter intent in a comment and implement shutdown logic using server.Shutdown(ctx) (or similar) inside Start, but do not leave ctx unused—update the MaestroMCPServer.Start method and all call sites accordingly.cmd/maestro/mcp/tools/consumers.go (2)
69-77: Non-string label values are silently dropped.When converting labels from
map[string]anytomap[string]string, non-string values are silently ignored. This could lead to user confusion if they provide numeric or boolean labels expecting them to be converted or stored. Consider either warning about skipped values or converting them viafmt.Sprintf.💡 Alternative: convert non-string values
if labels, ok := args["labels"].(map[string]any); ok { labelMap := make(map[string]string) for k, v := range labels { - if strVal, ok := v.(string); ok { - labelMap[k] = strVal - } + switch val := v.(type) { + case string: + labelMap[k] = val + default: + labelMap[k] = fmt.Sprintf("%v", val) + } } consumer.SetLabels(labelMap) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/consumers.go` around lines 69 - 77, The code that builds labelMap from args["labels"] (variable labels) silently drops non-string values before calling consumer.SetLabels, which can lose user-provided data; update the conversion logic in the same block (the labels -> labelMap construction used before consumer.SetLabels) to convert non-string values with fmt.Sprintf("%v", v) (or otherwise serialize them) instead of skipping, or emit a warning/log for each skipped key so callers know a label was dropped; ensure you import fmt if you choose conversion and keep the resulting map[string]string passed to consumer.SetLabels.
100-106: Same silent drop issue for label conversion in UpdateConsumerLabels.Apply the same consideration here as in
HandleCreateConsumerfor consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/consumers.go` around lines 100 - 106, UpdateConsumerLabels currently silently drops non-string label values when building labelMap; make it consistent with HandleCreateConsumer by applying the same conversion/validation logic used there: instead of skipping non-string values, convert them to strings (e.g. via fmt.Sprint or the same helper used in HandleCreateConsumer) and/or emit a warning log for unexpected types, then assign the converted string to labelMap[k]; reference UpdateConsumerLabels and HandleCreateConsumer to copy the exact conversion/validation behavior.cmd/maestro/mcp/server/server_test.go (2)
60-95: Consider adding negative test cases for server initialization.The current test validates the happy path. Consider adding tests for error scenarios such as invalid REST config (missing BaseURL) or invalid gRPC config (missing ServerAddress/SourceID) to ensure proper error handling in
NewMaestroMCPServer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server_test.go` around lines 60 - 95, Add negative test cases to server_test.go that call NewMaestroMCPServer with invalid configs and assert it returns an error and no server: e.g., a subtest that passes a clients.Config with RESTConfig.BaseURL empty to validate REST validation, and separate subtests that pass GRPCConfig with empty ServerAddress and empty SourceID to validate gRPC validation; for each subtest assert err != nil and that the returned *MaestroMCPServer (from NewMaestroMCPServer) is nil or its mcpServer field is nil to ensure initialization failed as expected.
152-252: Good parameterized test structure for input schema validation.The table-driven test approach is idiomatic Go. Consider adding
maestro_list_resource_bundles,maestro_list_consumers, andmaestro_apply_resource_bundleto the test cases for complete coverage of all tools' schema definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server_test.go` around lines 152 - 252, Add test cases for the missing tools inside the tests slice in TestToolInputSchemas: include entries for "maestro_list_resource_bundles", "maestro_list_consumers", and "maestro_apply_resource_bundle" (using the same struct fields toolName, requiredParams, optionalParams), populating requiredParams and optionalParams to match each tool's InputSchema as defined in tools.GetMaestroTools(); this ensures the loop that finds toolDefs and the subsequent checks against tool.InputSchema.Required and tool.InputSchema.Properties will validate those tools as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/maestro/mcp/tools/resource_bundles.go`:
- Around line 74-78: The code dereferences bundle.ConsumerName and
bundle.Version before calling t.grpcClient.Delete which can panic if either is
nil; update the logic in the function containing t.grpcClient.Delete to validate
that bundle.ConsumerName and bundle.Version are non-nil and return an
appropriate mcp.NewToolResultError (e.g., "missing bundle consumer name" or
"missing bundle version") if either is nil, only calling
t.grpcClient.Delete(ctx, id, *bundle.ConsumerName, *bundle.Version) when both
are present.
- Around line 124-143: In HandleListResourceBundlesByConsumer, consumerName is
directly interpolated into the search string which allows injection; sanitize or
escape consumerName before building search (e.g., replace single quote ' with
two single quotes '' or otherwise validate/whitelist allowed characters) and
then use the escaped value when creating search and calling
t.restClient.ListResourceBundles so the constructed search string is safe.
In `@docs/cli/mcp.md`:
- Line 16: The anchor link "[Troubleshooting](`#Troubleshooting`)" is incorrectly
capitalized and doesn't match the generated heading anchor; update the fragment
to lowercase by replacing "#Troubleshooting" with "#troubleshooting" wherever it
appears (locate the link text "[Troubleshooting](`#Troubleshooting`)" in
docs/cli/mcp.md) so the link correctly targets the "Troubleshooting" heading.
---
Nitpick comments:
In `@cmd/maestro/mcp/cmd.go`:
- Around line 91-104: The select currently logs server errors from errCh and
then returns nil, and also logs the same error elsewhere; change the err
handling to return the error (or a wrapped error) instead of returning nil and
avoid duplicate logging: in the select case for "case err := <-errCh" call
mcpServer.Stop() (or ensure mcpServer.Stop() is deferred earlier) and then
return err (or fmt.Errorf("server error: %w", err)); reference the errCh receive
case, the mcpServer.Stop() call, and the sigCh / ctx.Done() branches so the
function consistently stops the server and returns the actual error to the
caller.
In `@cmd/maestro/mcp/server/server_test.go`:
- Around line 60-95: Add negative test cases to server_test.go that call
NewMaestroMCPServer with invalid configs and assert it returns an error and no
server: e.g., a subtest that passes a clients.Config with RESTConfig.BaseURL
empty to validate REST validation, and separate subtests that pass GRPCConfig
with empty ServerAddress and empty SourceID to validate gRPC validation; for
each subtest assert err != nil and that the returned *MaestroMCPServer (from
NewMaestroMCPServer) is nil or its mcpServer field is nil to ensure
initialization failed as expected.
- Around line 152-252: Add test cases for the missing tools inside the tests
slice in TestToolInputSchemas: include entries for
"maestro_list_resource_bundles", "maestro_list_consumers", and
"maestro_apply_resource_bundle" (using the same struct fields toolName,
requiredParams, optionalParams), populating requiredParams and optionalParams to
match each tool's InputSchema as defined in tools.GetMaestroTools(); this
ensures the loop that finds toolDefs and the subsequent checks against
tool.InputSchema.Required and tool.InputSchema.Properties will validate those
tools as well.
In `@cmd/maestro/mcp/server/server.go`:
- Around line 24-60: Add a nil-check at the start of NewMaestroMCPServer to
validate cfg before dereferencing it: if cfg == nil return nil, fmt.Errorf("nil
config"), then proceed to call clients.NewRESTClient(&cfg.RESTConfig) and
clients.NewGRPCClient(cfg) as before; this prevents a potential panic when
accessing cfg.RESTConfig and ensures callers get a clear error when cfg is
missing.
- Around line 62-99: The registerTools() and HandleToolCall() functions
duplicate the tool→handler mapping; consolidate by centralizing the mapping into
a single source (e.g., a package-level variable or a helper function like
getToolHandlers()) that returns the map[string]server.ToolHandlerFunc built from
s.resourceBundleTools and s.consumerTools and/or derived from
tools.GetMaestroTools(); then have registerTools() iterate toolDefs and call
s.mcpServer.AddTool(tool, handler) using that shared map and have
HandleToolCall() look up handlers from the same shared map (or remove
HandleToolCall entirely if s.mcpServer already dispatches calls), ensuring all
references use the single canonical mapping to avoid desynchronization.
- Around line 101-111: The Start method on MaestroMCPServer currently declares
an unused ctx parameter; remove the unused ctx from the Start signature (func (s
*MaestroMCPServer) Start() error) and update all callers to invoke Start without
a context, and remove any now-unused imports; alternatively, if you intend to
support graceful shutdown later, document the parameter intent in a comment and
implement shutdown logic using server.Shutdown(ctx) (or similar) inside Start,
but do not leave ctx unused—update the MaestroMCPServer.Start method and all
call sites accordingly.
In `@cmd/maestro/mcp/tools/consumers.go`:
- Around line 69-77: The code that builds labelMap from args["labels"] (variable
labels) silently drops non-string values before calling consumer.SetLabels,
which can lose user-provided data; update the conversion logic in the same block
(the labels -> labelMap construction used before consumer.SetLabels) to convert
non-string values with fmt.Sprintf("%v", v) (or otherwise serialize them)
instead of skipping, or emit a warning/log for each skipped key so callers know
a label was dropped; ensure you import fmt if you choose conversion and keep the
resulting map[string]string passed to consumer.SetLabels.
- Around line 100-106: UpdateConsumerLabels currently silently drops non-string
label values when building labelMap; make it consistent with
HandleCreateConsumer by applying the same conversion/validation logic used
there: instead of skipping non-string values, convert them to strings (e.g. via
fmt.Sprint or the same helper used in HandleCreateConsumer) and/or emit a
warning log for unexpected types, then assign the converted string to
labelMap[k]; reference UpdateConsumerLabels and HandleCreateConsumer to copy the
exact conversion/validation behavior.
In `@cmd/maestro/mcp/tools/resource_bundles_test.go`:
- Around line 15-425: Add unit tests for the missing mutation handlers: create
TestResourceBundleTools_HandleApplyResourceBundle and
TestResourceBundleTools_HandleDeleteResourceBundle using the same setup pattern
(mock.NewMaestroServer, clients.NewRESTClient, NewResourceBundleTools) and
invoking tools.HandleApplyResourceBundle and tools.HandleDeleteResourceBundle
via mcp.CallToolRequest; for Apply include at minimum: successful apply (valid
bundle payload), validation error (missing/invalid args), and server error
response; for Delete include: successful delete (valid "id"), delete
non-existent id (expect error containing "not found"), and missing/empty "id"
parameter (expect error mentioning "id parameter is required"); assert results
by checking result.IsError and using getResultText to verify error messages
consistent with other tests.
In `@docs/cli/mcp.md`:
- Around line 421-443: The example conversation code blocks (showing the
User/Claude exchanges that call maestro_create_consumer,
maestro_apply_resource_bundle, and maestro_get_resource_bundle_status) lack a
language specifier; update each triple-backtick fence around those conversation
examples to include a language tag such as "text" or "plaintext" (e.g., ```text)
so the blocks render consistently and plainly across documentation viewers.
- Around line 62-71: The code block containing the ASCII architecture diagram is
missing a language specifier; update the opening fence for the diagram from ```
to a plaintext specifier (e.g., ```text or ```plaintext) so markdown linters
treat it as literal text; locate the ASCII block shown with the MCP Server /
Maestro API diagram and replace its opening triple backticks with ```text (or
```plaintext) while leaving the diagram content and closing fence unchanged.
In `@go.mod`:
- Line 26: Update the mcp-go dependency from v0.44.1 to v0.45.0 by changing the
version string for "github.com/mark3labs/mcp-go v0.44.1" to v0.45.0 in go.mod
(or run: go get github.com/mark3labs/mcp-go@v0.45.0), then run go mod tidy to
refresh go.sum and ensure the module graph is consistent; verify builds and
tests pass referencing the updated dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bb64d63-8da6-4f9e-9c78-b0b935c72458
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/maestro/main.gocmd/maestro/mcp/cmd.gocmd/maestro/mcp/server/server.gocmd/maestro/mcp/server/server_test.gocmd/maestro/mcp/tools/consumers.gocmd/maestro/mcp/tools/consumers_test.gocmd/maestro/mcp/tools/resource_bundles.gocmd/maestro/mcp/tools/resource_bundles_test.gocmd/maestro/mcp/tools/types.godocs/cli/README.mddocs/cli/mcp.mdgo.mod
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docs/cli/mcp.md (1)
16-16:⚠️ Potential issue | 🟡 MinorFix broken anchor link.
The link fragment
#Troubleshootingshould be lowercase#troubleshootingto match standard markdown anchor generation.-- [Troubleshooting](`#Troubleshooting`) +- [Troubleshooting](`#troubleshooting`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/mcp.md` at line 16, Update the markdown link fragment so it matches the generated anchor casing: change the link "[Troubleshooting](`#Troubleshooting`)" to use a lowercase fragment "[Troubleshooting](`#troubleshooting`)"; locate the occurrence of that link text in docs/cli/mcp.md and replace the fragment portion only.cmd/maestro/mcp/tools/resource_bundles.go (2)
74-78:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference when accessing bundle fields.
If
bundle.ConsumerNameorbundle.Versionis nil (as they are pointer types permodel_resource_bundle.go), dereferencing them on line 76 will cause a panic.🛡️ Proposed fix to add nil checks
+ // Validate required fields are present + if bundle.ConsumerName == nil { + return mcp.NewToolResultError("resource bundle has no consumer name"), nil + } + if bundle.Version == nil { + return mcp.NewToolResultError("resource bundle has no version"), nil + } + // Delete via gRPC if err := t.grpcClient.Delete(ctx, id, *bundle.ConsumerName, *bundle.Version); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles.go` around lines 74 - 78, The deletion call dereferences pointer fields bundle.ConsumerName and bundle.Version which may be nil and cause a panic; update the code around the t.grpcClient.Delete invocation to validate bundle.ConsumerName and bundle.Version are non-nil before dereferencing (returning an mcp.NewToolResultError with a clear message if either is nil), and then pass the dereferenced values to t.grpcClient.Delete (or use safe local variables) so the function name t.grpcClient.Delete is only called with validated concrete strings.
124-143:⚠️ Potential issue | 🔴 CriticalSQL injection vulnerability: user input directly interpolated into search query.
The
consumerNameparameter is directly interpolated into the search string without escaping. A malicious input likefoo' OR '1'='1could manipulate the WHERE clause.🔒 Proposed fix: escape single quotes
+import "strings" func (t *ResourceBundleTools) HandleListResourceBundlesByConsumer(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { // ... parameter extraction ... - search := fmt.Sprintf("consumer_name = '%s'", consumerName) + escapedName := strings.ReplaceAll(consumerName, "'", "''") + search := fmt.Sprintf("consumer_name = '%s'", escapedName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles.go` around lines 124 - 143, The code builds a SQL-style search string in HandleListResourceBundlesByConsumer by directly interpolating consumerName into search, allowing injection; sanitize consumerName first (e.g., replace single quote ' with doubled quotes '' or use a proper escaping helper) before constructing search, then call t.restClient.ListResourceBundles with the sanitized value; reference HandleListResourceBundlesByConsumer, the consumerName variable, the search string, and restClient.ListResourceBundles when making the change.
🧹 Nitpick comments (3)
docs/cli/mcp.md (1)
62-71: Add language specifier to ASCII diagram code block.For consistency and to satisfy linters, consider using a language hint like
textorplaintext.-``` +```text ┌─────────────────┐ ┌──────────────────┐ ┌─────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/mcp.md` around lines 62 - 71, The ASCII diagram code block in docs/cli/mcp.md lacks a language specifier for the fenced code block; update the opening fence for the diagram (the block that begins with ```) to include a language hint such as "text" or "plaintext" (e.g., change ``` to ```text) so linters recognize it as plain text while leaving the diagram content unchanged.cmd/maestro/mcp/tools/resource_bundles_test.go (1)
1-425: Missing test coverage forHandleDeleteResourceBundleandHandleApplyResourceBundle.These handlers involve gRPC operations and more complex logic (version validation, create vs update). Consider adding tests with a mock gRPC client to ensure correctness.
Would you like me to generate test scaffolding for these handlers?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles_test.go` around lines 1 - 425, Add unit tests for HandleDeleteResourceBundle and HandleApplyResourceBundle in resource_bundles_test.go that use a mock gRPC client (or extend mock.NewMaestroServer) to simulate success, not-found, and error responses; for HandleDeleteResourceBundle include tests for deleting an existing bundle, deleting a non-existent bundle (expect error result with "not found"), and missing/empty id parameter; for HandleApplyResourceBundle include tests for creating a new bundle, updating an existing bundle, and a version-mismatch/validation failure (expect appropriate error messages), invoking tools.HandleDeleteResourceBundle and tools.HandleApplyResourceBundle with mcp.CallToolRequest objects and asserting result.IsError and getResultText(result) contains expected text; ensure the mock client implements the same gRPC methods the handlers call so the tests exercise the create-vs-update logic and version validation paths.cmd/maestro/mcp/server/server.go (1)
101-111: Consider:Startignores the passed context.The
ctxparameter is unused sinceserver.ServeStdiois a blocking call that doesn't accept a context. This means graceful shutdown via context cancellation won't interrupt the server. The current signal-based shutdown incmd.goworks, but the API contract is misleading.💡 Option: Document the limitation or remove unused parameter
-// Start starts the MCP server on stdio -func (s *MaestroMCPServer) Start(ctx context.Context) error { +// Start starts the MCP server on stdio. +// Note: The server runs until stdio is closed; ctx is reserved for future use. +func (s *MaestroMCPServer) Start(_ context.Context) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 101 - 111, Start currently ignores the ctx parameter because server.ServeStdio is blocking; update Start (func (s *MaestroMCPServer) Start(ctx context.Context)) to run server.ServeStdio(s.mcpServer) in a goroutine and use a select to wait for either ctx.Done() or an error from ServeStdio sent on an error channel; on ctx cancellation call the appropriate shutdown method on s.mcpServer (e.g., Stop or Close) to terminate ServeStdio, and return either the ServeStdio error or a context.Canceled/timeout error—alternatively, if graceful shutdown is not supported, remove the ctx parameter from Start or add a comment documenting that ctx is unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/maestro/mcp/cmd.go`:
- Around line 91-104: The server error from errCh is logged but not propagated
from runMCP; update the select handling so that when err := <-errCh is received
you still stop the server (call mcpServer.Stop()) and then return that error
instead of nil. Locate the select block that reads from sigCh, ctx.Done(), and
errCh in runMCP and change the errCh case to ensure mcpServer.Stop() is invoked
and the function returns err; keep the existing klog.Errorf call but propagate
the error to the caller.
In `@cmd/maestro/mcp/tools/resource_bundles.go`:
- Around line 184-244: HandleApplyResourceBundle does not validate required
openapi.ResourceBundle fields before submitting to grpcClient.Apply; add checks
that bundle.ConsumerName is non-nil/non-empty and bundle.Manifests is non-nil
and has length>0 (or other manifest-required condition) after unmarshaling and
before deciding create/update or calling t.grpcClient.Apply, and return
mcp.NewToolResultError with a clear message (e.g., "consumer_name is required"
or "manifests are required") when those validations fail; keep existing flow for
ID/version handling and only call t.grpcClient.Apply when validations pass.
---
Duplicate comments:
In `@cmd/maestro/mcp/tools/resource_bundles.go`:
- Around line 74-78: The deletion call dereferences pointer fields
bundle.ConsumerName and bundle.Version which may be nil and cause a panic;
update the code around the t.grpcClient.Delete invocation to validate
bundle.ConsumerName and bundle.Version are non-nil before dereferencing
(returning an mcp.NewToolResultError with a clear message if either is nil), and
then pass the dereferenced values to t.grpcClient.Delete (or use safe local
variables) so the function name t.grpcClient.Delete is only called with
validated concrete strings.
- Around line 124-143: The code builds a SQL-style search string in
HandleListResourceBundlesByConsumer by directly interpolating consumerName into
search, allowing injection; sanitize consumerName first (e.g., replace single
quote ' with doubled quotes '' or use a proper escaping helper) before
constructing search, then call t.restClient.ListResourceBundles with the
sanitized value; reference HandleListResourceBundlesByConsumer, the consumerName
variable, the search string, and restClient.ListResourceBundles when making the
change.
In `@docs/cli/mcp.md`:
- Line 16: Update the markdown link fragment so it matches the generated anchor
casing: change the link "[Troubleshooting](`#Troubleshooting`)" to use a lowercase
fragment "[Troubleshooting](`#troubleshooting`)"; locate the occurrence of that
link text in docs/cli/mcp.md and replace the fragment portion only.
---
Nitpick comments:
In `@cmd/maestro/mcp/server/server.go`:
- Around line 101-111: Start currently ignores the ctx parameter because
server.ServeStdio is blocking; update Start (func (s *MaestroMCPServer)
Start(ctx context.Context)) to run server.ServeStdio(s.mcpServer) in a goroutine
and use a select to wait for either ctx.Done() or an error from ServeStdio sent
on an error channel; on ctx cancellation call the appropriate shutdown method on
s.mcpServer (e.g., Stop or Close) to terminate ServeStdio, and return either the
ServeStdio error or a context.Canceled/timeout error—alternatively, if graceful
shutdown is not supported, remove the ctx parameter from Start or add a comment
documenting that ctx is unused.
In `@cmd/maestro/mcp/tools/resource_bundles_test.go`:
- Around line 1-425: Add unit tests for HandleDeleteResourceBundle and
HandleApplyResourceBundle in resource_bundles_test.go that use a mock gRPC
client (or extend mock.NewMaestroServer) to simulate success, not-found, and
error responses; for HandleDeleteResourceBundle include tests for deleting an
existing bundle, deleting a non-existent bundle (expect error result with "not
found"), and missing/empty id parameter; for HandleApplyResourceBundle include
tests for creating a new bundle, updating an existing bundle, and a
version-mismatch/validation failure (expect appropriate error messages),
invoking tools.HandleDeleteResourceBundle and tools.HandleApplyResourceBundle
with mcp.CallToolRequest objects and asserting result.IsError and
getResultText(result) contains expected text; ensure the mock client implements
the same gRPC methods the handlers call so the tests exercise the
create-vs-update logic and version validation paths.
In `@docs/cli/mcp.md`:
- Around line 62-71: The ASCII diagram code block in docs/cli/mcp.md lacks a
language specifier for the fenced code block; update the opening fence for the
diagram (the block that begins with ```) to include a language hint such as
"text" or "plaintext" (e.g., change ``` to ```text) so linters recognize it as
plain text while leaving the diagram content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4ca5635-0d70-41f4-b993-f83d16519a48
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/maestro/main.gocmd/maestro/mcp/cmd.gocmd/maestro/mcp/server/server.gocmd/maestro/mcp/server/server_test.gocmd/maestro/mcp/tools/consumers.gocmd/maestro/mcp/tools/consumers_test.gocmd/maestro/mcp/tools/resource_bundles.gocmd/maestro/mcp/tools/resource_bundles_test.gocmd/maestro/mcp/tools/types.godocs/cli/README.mddocs/cli/mcp.mdgo.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/maestro/main.go
- go.mod
- cmd/maestro/mcp/tools/consumers_test.go
- cmd/maestro/mcp/tools/consumers.go
- cmd/maestro/mcp/server/server_test.go
Comment on lines
+91
to
+104
| select { | ||
| case <-sigCh: | ||
| klog.Info("Received interrupt signal, shutting down gracefully...") | ||
| case <-ctx.Done(): | ||
| klog.Info("Context cancelled, shutting down...") | ||
| case err := <-errCh: | ||
| klog.Errorf("Server error: %v", err) | ||
| } | ||
|
|
||
| // Stop the server | ||
| mcpServer.Stop() | ||
| klog.Info("MCP server stopped successfully") | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Server errors are logged but not returned.
When the server encounters an error (line 96-97), it's logged but runMCP still returns nil. This means the CLI will exit with status 0 even on server failure, which could mask issues in automated deployments.
🔧 Proposed fix to propagate errors
+ var serverErr error
select {
case <-sigCh:
klog.Info("Received interrupt signal, shutting down gracefully...")
case <-ctx.Done():
klog.Info("Context cancelled, shutting down...")
case err := <-errCh:
klog.Errorf("Server error: %v", err)
+ serverErr = err
}
// Stop the server
mcpServer.Stop()
klog.Info("MCP server stopped successfully")
- return nil
+ return serverErr📝 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.
Suggested change
| select { | |
| case <-sigCh: | |
| klog.Info("Received interrupt signal, shutting down gracefully...") | |
| case <-ctx.Done(): | |
| klog.Info("Context cancelled, shutting down...") | |
| case err := <-errCh: | |
| klog.Errorf("Server error: %v", err) | |
| } | |
| // Stop the server | |
| mcpServer.Stop() | |
| klog.Info("MCP server stopped successfully") | |
| return nil | |
| var serverErr error | |
| select { | |
| case <-sigCh: | |
| klog.Info("Received interrupt signal, shutting down gracefully...") | |
| case <-ctx.Done(): | |
| klog.Info("Context cancelled, shutting down...") | |
| case err := <-errCh: | |
| klog.Errorf("Server error: %v", err) | |
| serverErr = err | |
| } | |
| // Stop the server | |
| mcpServer.Stop() | |
| klog.Info("MCP server stopped successfully") | |
| return serverErr |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/maestro/mcp/cmd.go` around lines 91 - 104, The server error from errCh is
logged but not propagated from runMCP; update the select handling so that when
err := <-errCh is received you still stop the server (call mcpServer.Stop()) and
then return that error instead of nil. Locate the select block that reads from
sigCh, ctx.Done(), and errCh in runMCP and change the errCh case to ensure
mcpServer.Stop() is invoked and the function returns err; keep the existing
klog.Errorf call but propagate the error to the caller.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
cmd/maestro/mcp/tools/resource_bundles.go (1)
210-245:⚠️ Potential issue | 🟡 MinorValidate
consumer_nameandmanifestsbeforegrpcClient.Apply.
HandleApplyResourceBundlestill accepts any deserializable object and forwards it downstream, even thoughconsumer_nameandmanifestsare documented as required. That turns malformed MCP input into a backend-side failure instead of a clear tool error.🛡️ Suggested guard
var bundle openapi.ResourceBundle if err := json.Unmarshal(bundleJSON, &bundle); err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to parse bundle: %v", err)), nil } + + if bundle.ConsumerName == nil || *bundle.ConsumerName == "" { + return mcp.NewToolResultError("consumer_name is required"), nil + } + if len(bundle.Manifests) == 0 { + return mcp.NewToolResultError("manifests are required"), nil + } // Determine action based on whether ID was provided var action cetypes.EventAction🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles.go` around lines 210 - 245, In HandleApplyResourceBundle, validate required fields before calling t.grpcClient.Apply: ensure bundle.ConsumerName is non-nil and non-empty (return mcp.NewToolResultError with a clear message if missing) and ensure bundle.Manifests is non-nil and has at least one entry (return mcp.NewToolResultError if empty); perform these checks after setting bundle.Id/Version and action but before the Apply call, referencing bundle.ConsumerName and bundle.Manifests to produce explicit tool errors instead of forwarding malformed input downstream.
🧹 Nitpick comments (2)
cmd/maestro/mcp/tools/resource_bundles_test.go (1)
15-425: The mutation handlers still have no regression coverage.This suite only exercises read/search flows.
HandleApplyResourceBundleandHandleDeleteResourceBundleare where the tricky logic lives—required field validation, ID/version handling, and gRPC error mapping—so they should have dedicated tests before this lands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles_test.go` around lines 15 - 425, Tests currently only cover read/search flows; add focused unit tests for the mutation handlers HandleApplyResourceBundle and HandleDeleteResourceBundle that validate required-field checking, id/version handling, and gRPC error-to-result mapping. Create table-driven tests calling tools.HandleApplyResourceBundle and tools.HandleDeleteResourceBundle (using NewResourceBundleTools, mock.NewMaestroServer and clients.NewRESTClient as in existing tests) with cases for missing required fields, empty/invalid id or version, successful create/update/delete, and simulated gRPC errors (map to expected result.IsError and specific error text); assert the returned mcp.CallToolResult IsError and that getResultText(result) contains the expected messages for each scenario.cmd/maestro/mcp/server/server.go (1)
67-84: Use one source of truth for tool dispatch.The tool-name list is duplicated in
registerTools()andHandleToolCall(). That makes it easy to add a tool in one place and silently miss the other, leaving the generic path to returnunknown tool.Also applies to: 129-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 67 - 84, The handlers map in registerTools() is duplicated in HandleToolCall(), causing drift; consolidate to a single source of truth by extracting the map into one shared location (e.g., a server field like s.toolHandlers or a package-level var) and have both registerTools() and HandleToolCall() reference that single map (keep the existing server.ToolHandlerFunc signatures and entries such as s.resourceBundleTools.HandleListResourceBundles, s.consumerTools.HandleListConsumers, etc.); update initialization so registerTools() populates that single map (or assigns it) and modify HandleToolCall() to look up tools only from s.toolHandlers to remove the duplicated list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/maestro/mcp/server/server_test.go`:
- Around line 81-94: TestRegisterTools only checks GetMaestroTools() and
mcpServer.mcpServer non-nil, so it won't fail if registerTools() is a no-op;
update the test to assert observable behavior from registration by invoking a
known tool via the server (use HandleToolCall or the server's exposed call API)
or by inspecting the server's registered tool list if available. Specifically,
in TestRegisterTools call a deterministic tool from tools.GetMaestroTools()
(pick a tool name/ID you know exists) through mcpServer.HandleToolCall (or
equivalent) and assert the expected response or error, or use the server method
that returns registered tool descriptors to assert the known tool is present;
this ensures registerTools() actually registered handlers tied to
mcpServer.mcpServer.
In `@cmd/maestro/mcp/server/server.go`:
- Around line 102-125: Start currently blocks in server.ServeStdio and ignores
ctx, and Stop only closes the gRPC client, so the stdio server never shuts down;
change Start (MaestroMCPServer.Start) to launch ServeStdio in a goroutine and
record a cancellation mechanism (either store a cancel func from a
context.WithCancel or a serverDone channel), then have Start select on ctx.Done
to initiate shutdown and wait for the ServeStdio goroutine to finish (capture
ServeStdio's error and return it if not due to cancellation); update Stop
(MaestroMCPServer.Stop) to trigger that same cancellation/shutdown (call the
stored cancel func or close the shutdown channel) and perform any
server-specific shutdown API (e.g., call s.mcpServer.Stop/Close/GracefulStop if
available) and wait for the serverDone to ensure the stdio server actually stops
before returning, while still closing s.grpcClient as before.
- Around line 25-29: The constructor NewMaestroMCPServer dereferences cfg
immediately causing a panic for a nil config; add a nil guard at the start of
NewMaestroMCPServer to return a clear error when cfg == nil (e.g.,
fmt.Errorf("nil config: cfg is required")) before calling
clients.NewRESTClient(&cfg.RESTConfig), so references to cfg.RESTConfig only
occur after the nil check; update any unit tests or callers if needed to expect
the returned error when passing nil.
- Around line 33-59: The constructor opens a gRPC client via
clients.NewGRPCClient but if s.registerTools() fails the function returns
without closing that client; ensure the client is closed on error by either
deferring its shutdown immediately after successful creation (e.g., defer
func(){ if err!=nil { grpcClient.Stop() } }()) or explicitly calling
grpcClient.Stop()/Close() before returning the error from the registerTools
failure branch; locate NewGRPCClient, grpcClient, registerTools and use the MCP
client's Stop/Close method (e.g., grpcClient.Stop()) to release resources on the
error path.
In `@cmd/maestro/mcp/tools/consumers.go`:
- Around line 69-76: The handlers currently silently drop non-string entries
when building labelMap from args["labels"], which hides caller errors; change
this by extracting the parsing logic into a shared function (e.g.,
parseLabels(args map[string]any) (map[string]string, error)) that validates
every value is a string and returns a tool error if any label value is not a
string, then call that function from both code paths and pass its result to
consumer.SetLabels; ensure the new parser is used in the other block noted
(lines ~100-110) so both handlers behave identically.
In `@docs/cli/mcp.md`:
- Around line 61-70: Several unlabeled fenced code blocks (notably the ASCII
diagram containing "AI Assistant", "MCP Server", "Maestro API" and other blocks
like the ones showing "Resource Bundle Tools (7)" / "Consumer Tools (5)" and
CLI/JSON examples) trigger markdownlint MD040; fix by adding an explicit fence
language to each triple-backtick block (use text or console for ASCII diagrams,
bash/console for CLI examples, json for JSON snippets) so each block starts with
```text, ```console, ```bash, or ```json respectively, updating the unlabeled
fences around the diagram and the other noted blocks.
---
Duplicate comments:
In `@cmd/maestro/mcp/tools/resource_bundles.go`:
- Around line 210-245: In HandleApplyResourceBundle, validate required fields
before calling t.grpcClient.Apply: ensure bundle.ConsumerName is non-nil and
non-empty (return mcp.NewToolResultError with a clear message if missing) and
ensure bundle.Manifests is non-nil and has at least one entry (return
mcp.NewToolResultError if empty); perform these checks after setting
bundle.Id/Version and action but before the Apply call, referencing
bundle.ConsumerName and bundle.Manifests to produce explicit tool errors instead
of forwarding malformed input downstream.
---
Nitpick comments:
In `@cmd/maestro/mcp/server/server.go`:
- Around line 67-84: The handlers map in registerTools() is duplicated in
HandleToolCall(), causing drift; consolidate to a single source of truth by
extracting the map into one shared location (e.g., a server field like
s.toolHandlers or a package-level var) and have both registerTools() and
HandleToolCall() reference that single map (keep the existing
server.ToolHandlerFunc signatures and entries such as
s.resourceBundleTools.HandleListResourceBundles,
s.consumerTools.HandleListConsumers, etc.); update initialization so
registerTools() populates that single map (or assigns it) and modify
HandleToolCall() to look up tools only from s.toolHandlers to remove the
duplicated list.
In `@cmd/maestro/mcp/tools/resource_bundles_test.go`:
- Around line 15-425: Tests currently only cover read/search flows; add focused
unit tests for the mutation handlers HandleApplyResourceBundle and
HandleDeleteResourceBundle that validate required-field checking, id/version
handling, and gRPC error-to-result mapping. Create table-driven tests calling
tools.HandleApplyResourceBundle and tools.HandleDeleteResourceBundle (using
NewResourceBundleTools, mock.NewMaestroServer and clients.NewRESTClient as in
existing tests) with cases for missing required fields, empty/invalid id or
version, successful create/update/delete, and simulated gRPC errors (map to
expected result.IsError and specific error text); assert the returned
mcp.CallToolResult IsError and that getResultText(result) contains the expected
messages for each scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b544668-ca77-462d-a199-f59a1c108638
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/maestro/main.gocmd/maestro/mcp/cmd.gocmd/maestro/mcp/server/server.gocmd/maestro/mcp/server/server_test.gocmd/maestro/mcp/tools/consumers.gocmd/maestro/mcp/tools/consumers_test.gocmd/maestro/mcp/tools/resource_bundles.gocmd/maestro/mcp/tools/resource_bundles_test.gocmd/maestro/mcp/tools/types.godocs/cli/README.mddocs/cli/mcp.mdgo.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/maestro/main.go
- cmd/maestro/mcp/cmd.go
- cmd/maestro/mcp/tools/consumers_test.go
Comment on lines
+81
to
+94
| // Verify all tools are registered | ||
| toolDefs := tools.GetMaestroTools() | ||
| expectedToolCount := len(toolDefs) | ||
|
|
||
| // We can't directly check the registered tools without exposing internal state, | ||
| // but we can verify that registration completed without error | ||
| if expectedToolCount == 0 { | ||
| t.Error("Expected tools to be defined, but got 0") | ||
| } | ||
|
|
||
| // Verify the server was initialized | ||
| if mcpServer.mcpServer == nil { | ||
| t.Fatal("MCP server not initialized after registration") | ||
| } |
There was a problem hiding this comment.
TestRegisterTools never observes registration.
Right now this only proves that GetMaestroTools() is non-empty and the server constructor returned a non-nil MCP server. It would still pass if registerTools() became a no-op. Please assert behavior that depends on registration, e.g. a known tool call through HandleToolCall or the server's registered tool list if that API is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/maestro/mcp/server/server_test.go` around lines 81 - 94,
TestRegisterTools only checks GetMaestroTools() and mcpServer.mcpServer non-nil,
so it won't fail if registerTools() is a no-op; update the test to assert
observable behavior from registration by invoking a known tool via the server
(use HandleToolCall or the server's exposed call API) or by inspecting the
server's registered tool list if available. Specifically, in TestRegisterTools
call a deterministic tool from tools.GetMaestroTools() (pick a tool name/ID you
know exists) through mcpServer.HandleToolCall (or equivalent) and assert the
expected response or error, or use the server method that returns registered
tool descriptors to assert the known tool is present; this ensures
registerTools() actually registered handlers tied to mcpServer.mcpServer.
Comment on lines
+69
to
+76
| if labels, ok := args["labels"].(map[string]any); ok { | ||
| labelMap := make(map[string]string) | ||
| for k, v := range labels { | ||
| if strVal, ok := v.(string); ok { | ||
| labelMap[k] = strVal | ||
| } | ||
| } | ||
| consumer.SetLabels(labelMap) |
There was a problem hiding this comment.
Reject non-string label values instead of silently dropping them.
Both handlers currently ignore invalid entries while building map[string]string. A payload like {"labels":{"tier":1}} will succeed with that key silently omitted, which is hard for the caller to spot. Return a tool error when any label value is not a string, and reuse the same parser in both paths.
Also applies to: 100-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/maestro/mcp/tools/consumers.go` around lines 69 - 76, The handlers
currently silently drop non-string entries when building labelMap from
args["labels"], which hides caller errors; change this by extracting the parsing
logic into a shared function (e.g., parseLabels(args map[string]any)
(map[string]string, error)) that validates every value is a string and returns a
tool error if any label value is not a string, then call that function from both
code paths and pass its result to consumer.SetLabels; ensure the new parser is
used in the other block noted (lines ~100-110) so both handlers behave
identically.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cmd/maestro/mcp/server/server_test.go (1)
81-94:⚠️ Potential issue | 🟡 MinorMake
TestRegisterToolsassert observable registration.This still passes if
registerTools()does nothing, because it only checksGetMaestroTools()andmcpServer.mcpServer != nil. Exercise a known tool throughHandleToolCallor inspect the registered descriptors so the test fails when registration breaks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server_test.go` around lines 81 - 94, Update TestRegisterTools to assert observable registration by exercising a known registered tool instead of just checking GetMaestroTools() and mcpServer.mcpServer; call registerTools() (or ensure it runs) then pick a concrete tool descriptor from tools.GetMaestroTools() (e.g., a tool with a known name used in tests) and invoke mcpServer.HandleToolCall (or the equivalent public method) with a minimal, valid request for that tool and assert the expected non-error response or behavior; alternatively, retrieve the registered descriptors from the registry returned by tools.GetMaestroTools() and assert the presence and expected fields (name/identifier/inputs) for a specific known tool to fail the test if registration is a no-op.docs/cli/mcp.md (1)
61-70:⚠️ Potential issue | 🟡 MinorAdd fence languages to the remaining code blocks.
These fences still trigger MD040. Use
textfor the diagram/dialog blocks andbashorconsolefor command examples so the docs lint cleanly.Also applies to: 420-425, 428-434, 437-442, 462-470, 483-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/mcp.md` around lines 61 - 70, The remaining fenced code blocks are missing language tags and trigger MD040; update each diagram/dialog ASCII block (e.g., the block containing the "AI Assistant / MCP Server / Maestro API" diagram) to use ```text and update command/example blocks to use ```bash or ```console as appropriate so the markdown linter stops flagging MD040; search for other similar blocks (the dialog/diagram blocks and the command examples mentioned in the comment) and add the correct fence language consistently.cmd/maestro/mcp/tools/consumers.go (1)
69-77:⚠️ Potential issue | 🟡 MinorReject non-string label values instead of silently dropping them.
A payload like
{"labels":{"tier":1}}currently succeeds withtieromitted, which is hard for callers to spot. Parse labels once and fail fast if any value is not a string.🔧 Suggested refactor
+func parseLabels(raw map[string]any) (map[string]string, error) { + labelMap := make(map[string]string, len(raw)) + for k, v := range raw { + strVal, ok := v.(string) + if !ok { + return nil, fmt.Errorf("label %q must be a string", k) + } + labelMap[k] = strVal + } + return labelMap, nil +} + // HandleCreateConsumer creates a new consumer func (t *ConsumerTools) HandleCreateConsumer(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { @@ // Add labels if provided if labels, ok := args["labels"].(map[string]any); ok { - labelMap := make(map[string]string) - for k, v := range labels { - if strVal, ok := v.(string); ok { - labelMap[k] = strVal - } - } + labelMap, err := parseLabels(labels) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } consumer.SetLabels(labelMap) } @@ - labelMap := make(map[string]string) - for k, v := range labels { - if strVal, ok := v.(string); ok { - labelMap[k] = strVal - } - } + labelMap, err := parseLabels(labels) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + }Also applies to: 95-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/consumers.go` around lines 69 - 77, The code currently silently drops non-string label values when building labelMap from args["labels"]; change the parsing in the block that reads labels (and the duplicate block around lines 95-110) to validate all entries and return an error if any value is not a string: when args["labels"] is a map[string]any iterate its entries, if any v is not a string produce a clear error (don’t skip the key) and abort the operation, otherwise build labelMap and call consumer.SetLabels(labelMap); ensure the same strict validation logic is applied to the other labels parsing block as well.cmd/maestro/mcp/cmd.go (1)
57-104:⚠️ Potential issue | 🟠 MajorPropagate
Startfailures instead of exiting 0.
runMCPstill returnsnilafter a server failure, and the extracancel()in the goroutine means the select can hitctx.Done()and drop the real error altogether. Derive the lifecycle context fromcmd.Context(), leterrChcarry server failures, stop the server, and return that error.🔧 Suggested shape
-func runMCP(cmd *cobra.Command, args []string) error { - // Create cancellable context - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() +func runMCP(cmd *cobra.Command, args []string) error { + ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + defer stop() @@ errCh := make(chan error, 1) go func() { if err := mcpServer.Start(ctx); err != nil { klog.Errorf("Server error: %v", err) errCh <- err - cancel() } }() - - // Wait for interrupt signal or error - sigCh := make(chan os.Signal, 1) - signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) + var serverErr error select { - case <-sigCh: - klog.Info("Received interrupt signal, shutting down gracefully...") case <-ctx.Done(): klog.Info("Context cancelled, shutting down...") case err := <-errCh: klog.Errorf("Server error: %v", err) + serverErr = err } @@ - return nil + return serverErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/cmd.go` around lines 57 - 104, The runMCP function must propagate Start failures instead of returning nil: derive the lifecycle context from cmd.Context() (use ctx := cmd.Context() or context.WithCancel(cmd.Context()) as appropriate), remove the extra cancel() call inside the goroutine, send any error from mcpServer.Start into errCh, and after the select ensure mcpServer.Stop() is called then return the server error if errCh delivered one (or the ctx error), rather than always returning nil; reference runMCP, mcpServer.Start, errCh, cmd.Context(), and mcpServer.Stop to locate and update the logic.
🧹 Nitpick comments (2)
cmd/maestro/mcp/tools/types.go (2)
15-22: Useintegerfor pagination fields.These arguments are consumed as whole numbers, but the schema currently advertises them as
number. That allows values like1.5, whichgetIntParam()will silently truncate instead of rejecting.🔧 Representative schema fix
- "type": "number", + "type": "integer",Also applies to: 96-103, 118-125, 138-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/types.go` around lines 15 - 22, The JSON schema for pagination fields currently uses "type": "number", which permits non-integer values that getIntParam() will truncate; update the schema entries for the pagination fields (the "page" and "size" map[string]interface{} objects shown in types.go and the other occurrences at the same groups) to use "type": "integer" and keep the existing descriptions so the schema enforces whole numbers; ensure you update every occurrence noted (including the other blocks around the same file referenced in the comment) so all pagination params are declared as integers.
47-55: Advertise the bundle shape instead of a bareobject.
maestro_apply_resource_bundleis the hardest tool for an assistant to call, but the schema currently exposes no nested fields. Defining properties likeid,consumer_name,version, andmanifestshere would give MCP clients enough structure to validate and synthesize requests reliably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/types.go` around lines 47 - 55, Update the InputSchema for the "bundle" property to advertise its nested shape instead of a bare "object": replace the current bundle entry under InputSchema (mcp.ToolInputSchema -> Properties -> "bundle") with a nested schema that defines properties id (string), consumer_name (string), version (string), manifests (array of objects or array of JSON with an appropriate item schema), and an optional metadata (object) field; also add a "Required" list inside that bundle schema listing id, consumer_name, version, and manifests so callers can validate requests properly while leaving metadata optional. Ensure you modify the same mcp.ToolInputSchema structure that defines InputSchema and keep the top-level Required still including "bundle".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/maestro/mcp/tools/resource_bundles.go`:
- Around line 219-225: The current logic in the block that calls
t.restClient.GetResourceBundle collapses any error into a "not found" result;
change it so that if GetResourceBundle returns a non-nil error you return that
error (wrapped via mcp.NewToolResultError or returned directly) rather than the
not-found message, and only return "resource bundle '<id>' not found" when err
== nil but existingBundle == nil; update the code around resourceID,
existingBundle, and the GetResourceBundle call accordingly so auth/timeouts/500s
propagate instead of being misreported as not-found.
---
Duplicate comments:
In `@cmd/maestro/mcp/cmd.go`:
- Around line 57-104: The runMCP function must propagate Start failures instead
of returning nil: derive the lifecycle context from cmd.Context() (use ctx :=
cmd.Context() or context.WithCancel(cmd.Context()) as appropriate), remove the
extra cancel() call inside the goroutine, send any error from mcpServer.Start
into errCh, and after the select ensure mcpServer.Stop() is called then return
the server error if errCh delivered one (or the ctx error), rather than always
returning nil; reference runMCP, mcpServer.Start, errCh, cmd.Context(), and
mcpServer.Stop to locate and update the logic.
In `@cmd/maestro/mcp/server/server_test.go`:
- Around line 81-94: Update TestRegisterTools to assert observable registration
by exercising a known registered tool instead of just checking GetMaestroTools()
and mcpServer.mcpServer; call registerTools() (or ensure it runs) then pick a
concrete tool descriptor from tools.GetMaestroTools() (e.g., a tool with a known
name used in tests) and invoke mcpServer.HandleToolCall (or the equivalent
public method) with a minimal, valid request for that tool and assert the
expected non-error response or behavior; alternatively, retrieve the registered
descriptors from the registry returned by tools.GetMaestroTools() and assert the
presence and expected fields (name/identifier/inputs) for a specific known tool
to fail the test if registration is a no-op.
In `@cmd/maestro/mcp/tools/consumers.go`:
- Around line 69-77: The code currently silently drops non-string label values
when building labelMap from args["labels"]; change the parsing in the block that
reads labels (and the duplicate block around lines 95-110) to validate all
entries and return an error if any value is not a string: when args["labels"] is
a map[string]any iterate its entries, if any v is not a string produce a clear
error (don’t skip the key) and abort the operation, otherwise build labelMap and
call consumer.SetLabels(labelMap); ensure the same strict validation logic is
applied to the other labels parsing block as well.
In `@docs/cli/mcp.md`:
- Around line 61-70: The remaining fenced code blocks are missing language tags
and trigger MD040; update each diagram/dialog ASCII block (e.g., the block
containing the "AI Assistant / MCP Server / Maestro API" diagram) to use ```text
and update command/example blocks to use ```bash or ```console as appropriate so
the markdown linter stops flagging MD040; search for other similar blocks (the
dialog/diagram blocks and the command examples mentioned in the comment) and add
the correct fence language consistently.
---
Nitpick comments:
In `@cmd/maestro/mcp/tools/types.go`:
- Around line 15-22: The JSON schema for pagination fields currently uses
"type": "number", which permits non-integer values that getIntParam() will
truncate; update the schema entries for the pagination fields (the "page" and
"size" map[string]interface{} objects shown in types.go and the other
occurrences at the same groups) to use "type": "integer" and keep the existing
descriptions so the schema enforces whole numbers; ensure you update every
occurrence noted (including the other blocks around the same file referenced in
the comment) so all pagination params are declared as integers.
- Around line 47-55: Update the InputSchema for the "bundle" property to
advertise its nested shape instead of a bare "object": replace the current
bundle entry under InputSchema (mcp.ToolInputSchema -> Properties -> "bundle")
with a nested schema that defines properties id (string), consumer_name
(string), version (string), manifests (array of objects or array of JSON with an
appropriate item schema), and an optional metadata (object) field; also add a
"Required" list inside that bundle schema listing id, consumer_name, version,
and manifests so callers can validate requests properly while leaving metadata
optional. Ensure you modify the same mcp.ToolInputSchema structure that defines
InputSchema and keep the top-level Required still including "bundle".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 303164ef-ff19-4917-90f6-99d103739bf5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/maestro/main.gocmd/maestro/mcp/cmd.gocmd/maestro/mcp/server/server.gocmd/maestro/mcp/server/server_test.gocmd/maestro/mcp/tools/consumers.gocmd/maestro/mcp/tools/consumers_test.gocmd/maestro/mcp/tools/resource_bundles.gocmd/maestro/mcp/tools/resource_bundles_test.gocmd/maestro/mcp/tools/types.godocs/cli/README.mddocs/cli/mcp.mdgo.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/maestro/main.go
- cmd/maestro/mcp/tools/consumers_test.go
- cmd/maestro/mcp/tools/resource_bundles_test.go
- go.mod
- cmd/maestro/mcp/server/server.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/cli/mcp.md (1)
61-70:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code blocks.
The ASCII architecture diagram code block lacks a language specifier, triggering markdownlint MD040. Add
textto the fence.📝 Proposed fix
-``` +```text ┌─────────────────┐ ┌──────────────────┐ ┌─────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/mcp.md` around lines 61 - 70, Update the fenced code block that contains the ASCII architecture diagram (the triple-backtick block that begins with the diagram showing "AI Assistant", "MCP Server", "Maestro API") to include a language specifier by changing the opening fence from ``` to ```text so markdownlint MD040 is satisfied.cmd/maestro/mcp/tools/consumers.go (2)
100-107:⚠️ Potential issue | 🟡 MinorNon-string label values are silently dropped in update handler.
Same issue as in
HandleCreateConsumer- non-string label values are silently ignored.🛡️ Proposed fix
// Convert labels to map[string]string labelMap := make(map[string]string) for k, v := range labels { - if strVal, ok := v.(string); ok { - labelMap[k] = strVal + strVal, ok := v.(string) + if !ok { + return mcp.NewToolResultError(fmt.Sprintf("label '%s' has non-string value; all label values must be strings", k)), nil } + labelMap[k] = strVal }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/consumers.go` around lines 100 - 107, The update handler currently drops non-string label values when building labelMap (loop over labels -> labelMap), mirroring the same bug from HandleCreateConsumer; change the conversion in the labels loop inside the update handler to coerce any non-string value to a string (e.g. use fmt.Sprint(v) or similar) instead of skipping, so every key in labels is preserved in labelMap; update imports if needed and ensure this same approach is consistent with HandleCreateConsumer to avoid silent data loss.
69-77:⚠️ Potential issue | 🟡 MinorNon-string label values are silently dropped.
When labels contain non-string values (e.g.,
{"tier": 1}), they are silently ignored. The caller has no indication that their label was dropped, which could lead to confusion. Consider returning an error for invalid label types.🛡️ Proposed fix
// Add labels if provided if labels, ok := args["labels"].(map[string]any); ok { labelMap := make(map[string]string) for k, v := range labels { - if strVal, ok := v.(string); ok { - labelMap[k] = strVal + strVal, ok := v.(string) + if !ok { + return mcp.NewToolResultError(fmt.Sprintf("label '%s' has non-string value; all label values must be strings", k)), nil } + labelMap[k] = strVal } consumer.SetLabels(labelMap) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/consumers.go` around lines 69 - 77, The current block that reads args["labels"] into labelMap silently drops non-string values; update the code around the labels handling (the args["labels"] cast and the loop that builds labelMap and calls consumer.SetLabels) to validate every label value and return an error (or propagate one) when any value is not a string instead of ignoring it; ensure the function signals failure to the caller with a clear message indicating which key had a non-string value so callers are aware of invalid label types before calling consumer.SetLabels.
🧹 Nitpick comments (4)
cmd/maestro/mcp/tools/consumers_test.go (1)
16-25: Consider extractinggetResultTextto a shared test helper.This helper function is used in both
consumers_test.goandresource_bundles_test.go. Moving it to a shared test utilities file would reduce duplication and ensure consistent behavior across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/consumers_test.go` around lines 16 - 25, Extract the getResultText helper into a shared test utilities package/file and update callers to use it: create a test helper (e.g., TestHelpers or testutil) that exposes a function GetResultText taking *mcp.CallToolResult and reimplement the logic using mcp.AsTextContent; then replace the local getResultText definitions in consumers_test.go and resource_bundles_test.go to call the shared GetResultText to remove duplication and ensure consistent behavior.cmd/maestro/mcp/tools/resource_bundles_test.go (1)
255-425: Consider adding tests forHandleApplyResourceBundleandHandleDeleteResourceBundle.The test file covers list, get, status, search, and list-by-consumer handlers, but omits tests for apply and delete operations. These are write operations with more complex logic (optimistic concurrency, gRPC calls) that would benefit from test coverage.
Would you like me to help generate test cases for these handlers?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/tools/resource_bundles_test.go` around lines 255 - 425, Add unit tests for HandleApplyResourceBundle and HandleDeleteResourceBundle similar to existing handlers: create a mock Maestro server and REST client, instantiate NewResourceBundleTools(restClient, nil), and craft mcp.CallToolRequest inputs to exercise success, validation errors (missing/empty required params like "consumer_name", "bundle", "etag" where applicable), optimistic concurrency failure (simulate etag mismatch), and gRPC or backend error paths; verify the returned mcp.CallToolResult IsError flag and that result text contains expected messages. Specifically target functions HandleApplyResourceBundle and HandleDeleteResourceBundle, include pagination/flags and both create/update flows for apply (new bundle vs update with etag), and assert correct behavior on delete when bundle missing or etag mismatch.cmd/maestro/mcp/server/server.go (2)
112-136:Startignores context andStopdoesn't halt the stdio server.
Startblocks inServeStdioignoring the passedctx, andStoponly closes the gRPC client without stopping the stdio server. For stdio-based MCP servers this may be acceptable since the process typically terminates when stdin closes, but callers cannot use context cancellation to initiate graceful shutdown.Consider documenting this behavior or, if graceful shutdown is needed, wrapping
ServeStdioin a goroutine with context-based termination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 112 - 136, Start currently calls server.ServeStdio synchronously and ignores the provided ctx, and Stop only closes grpcClient without stopping the stdio server; change Start to launch server.ServeStdio(s.mcpServer) in a goroutine and capture its error on a channel, then monitor ctx.Done() to initiate shutdown (call a shutdown/stop method on s.mcpServer or equivalent) and wait for the ServeStdio goroutine to exit; update Stop to also trigger that same shutdown path (not just close s.grpcClient) so the stdio server is halted on cancellation or Stop(), ensuring ServeStdio errors are propagated via the error channel and returned from Start when appropriate.
138-173:HandleToolCallduplicates the routing logic fromregisterTools.The switch statement mirrors the
handlersmap inregisterTools. While this provides a convenient single entry point, changes to tool names require updates in two places. Consider either:
- Removing
HandleToolCallif unused (MCP server routes via registered handlers)- Using the registered handlers map for dispatch if this method is needed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/maestro/mcp/server/server.go` around lines 138 - 173, HandleToolCall duplicates routing in registerTools by hardcoding the same switch of tool names; either remove HandleToolCall if it's unused, or change it to reuse the registered handlers map to avoid double maintenance. If keeping it, replace the switch in HandleToolCall with a lookup into the same handlers map built in registerTools (locate the handlers map variable or method that registers handlers), validate the found handler exists, invoke it (passing ctx and request) and return its result, and return NewToolResultError if not found; if removing it, ensure no callers reference HandleToolCall and delete the function and any tests or usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/maestro/mcp/tools/consumers.go`:
- Around line 100-107: The update handler currently drops non-string label
values when building labelMap (loop over labels -> labelMap), mirroring the same
bug from HandleCreateConsumer; change the conversion in the labels loop inside
the update handler to coerce any non-string value to a string (e.g. use
fmt.Sprint(v) or similar) instead of skipping, so every key in labels is
preserved in labelMap; update imports if needed and ensure this same approach is
consistent with HandleCreateConsumer to avoid silent data loss.
- Around line 69-77: The current block that reads args["labels"] into labelMap
silently drops non-string values; update the code around the labels handling
(the args["labels"] cast and the loop that builds labelMap and calls
consumer.SetLabels) to validate every label value and return an error (or
propagate one) when any value is not a string instead of ignoring it; ensure the
function signals failure to the caller with a clear message indicating which key
had a non-string value so callers are aware of invalid label types before
calling consumer.SetLabels.
In `@docs/cli/mcp.md`:
- Around line 61-70: Update the fenced code block that contains the ASCII
architecture diagram (the triple-backtick block that begins with the diagram
showing "AI Assistant", "MCP Server", "Maestro API") to include a language
specifier by changing the opening fence from ``` to ```text so markdownlint
MD040 is satisfied.
---
Nitpick comments:
In `@cmd/maestro/mcp/server/server.go`:
- Around line 112-136: Start currently calls server.ServeStdio synchronously and
ignores the provided ctx, and Stop only closes grpcClient without stopping the
stdio server; change Start to launch server.ServeStdio(s.mcpServer) in a
goroutine and capture its error on a channel, then monitor ctx.Done() to
initiate shutdown (call a shutdown/stop method on s.mcpServer or equivalent) and
wait for the ServeStdio goroutine to exit; update Stop to also trigger that same
shutdown path (not just close s.grpcClient) so the stdio server is halted on
cancellation or Stop(), ensuring ServeStdio errors are propagated via the error
channel and returned from Start when appropriate.
- Around line 138-173: HandleToolCall duplicates routing in registerTools by
hardcoding the same switch of tool names; either remove HandleToolCall if it's
unused, or change it to reuse the registered handlers map to avoid double
maintenance. If keeping it, replace the switch in HandleToolCall with a lookup
into the same handlers map built in registerTools (locate the handlers map
variable or method that registers handlers), validate the found handler exists,
invoke it (passing ctx and request) and return its result, and return
NewToolResultError if not found; if removing it, ensure no callers reference
HandleToolCall and delete the function and any tests or usages.
In `@cmd/maestro/mcp/tools/consumers_test.go`:
- Around line 16-25: Extract the getResultText helper into a shared test
utilities package/file and update callers to use it: create a test helper (e.g.,
TestHelpers or testutil) that exposes a function GetResultText taking
*mcp.CallToolResult and reimplement the logic using mcp.AsTextContent; then
replace the local getResultText definitions in consumers_test.go and
resource_bundles_test.go to call the shared GetResultText to remove duplication
and ensure consistent behavior.
In `@cmd/maestro/mcp/tools/resource_bundles_test.go`:
- Around line 255-425: Add unit tests for HandleApplyResourceBundle and
HandleDeleteResourceBundle similar to existing handlers: create a mock Maestro
server and REST client, instantiate NewResourceBundleTools(restClient, nil), and
craft mcp.CallToolRequest inputs to exercise success, validation errors
(missing/empty required params like "consumer_name", "bundle", "etag" where
applicable), optimistic concurrency failure (simulate etag mismatch), and gRPC
or backend error paths; verify the returned mcp.CallToolResult IsError flag and
that result text contains expected messages. Specifically target functions
HandleApplyResourceBundle and HandleDeleteResourceBundle, include
pagination/flags and both create/update flows for apply (new bundle vs update
with etag), and assert correct behavior on delete when bundle missing or etag
mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 139655de-9600-4ee9-9e3c-06f7ada404ab
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/maestro/main.gocmd/maestro/mcp/cmd.gocmd/maestro/mcp/server/server.gocmd/maestro/mcp/server/server_test.gocmd/maestro/mcp/tools/consumers.gocmd/maestro/mcp/tools/consumers_test.gocmd/maestro/mcp/tools/resource_bundles.gocmd/maestro/mcp/tools/resource_bundles_test.gocmd/maestro/mcp/tools/types.godocs/cli/README.mddocs/cli/mcp.mdgo.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/maestro/mcp/cmd.go
- cmd/maestro/main.go
- cmd/maestro/mcp/server/server_test.go
- go.mod
skeeey
force-pushed
the
cli-mcp
branch
4 times, most recently
from
3d91edb to
5607914
Compare
Signed-off-by: Wei Liu <liuweixa@redhat.com>
Contributor
Author
|
/cc @clyang82 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.