Conversation
📝 WalkthroughWalkthroughAdds a new Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/internal/api/platform.go (1)
30-42: Fix copy/paste in error messages ("plugin" → "platform").Error messages in GetPlatform() reference “plugin details endpoint”.
- return nil, fmt.Errorf("received non 200 response from %s plugin details endpoint: %d", version.ProductName, response.StatusCode) + return nil, fmt.Errorf("received non 200 response from %s platform details endpoint: %d", version.ProductName, response.StatusCode) @@ - return nil, fmt.Errorf("failed to read response from %s plugin details endpoint: %v", version.ProductName, err) + return nil, fmt.Errorf("failed to read response from %s platform details endpoint: %v", version.ProductName, err) @@ - return nil, fmt.Errorf("unexpected response from %s plugin details endpoint: %v", version.ProductName, err) + return nil, fmt.Errorf("unexpected response from %s platform details endpoint: %v", version.ProductName, err)cli/internal/api/plugin.go (1)
72-72: Avoid empty endpoint label in errors.Passing "" yields double spaces in messages. Use a label (e.g., "team") to keep errors clean and distinguish from public path.
- return c.parsePluginManifest(body, "") + return c.parsePluginManifest(body, "team")
🧹 Nitpick comments (5)
cli/cmd/mcp.go (2)
41-47: Propagate Cobra context; stop signal on exit.Use cmd.Context() so cancellations cascade, and stop signal notifications to avoid leaks.
- ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(cmd.Context()) @@ - signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM) + signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM) + defer signal.Stop(sigChan)
20-27: Polish: silence usage on runtime errors.Avoid printing cobra usage when server returns an error.
mcpCmd := &cobra.Command{ Use: "mcp", @@ - Long: `Start the Suga MCP server that provides access to Suga platform APIs + Long: `Start the Suga MCP server that provides access to Suga platform APIs through the Model Context Protocol. This allows AI assistants to interact with your Suga templates, platforms, and build manifests. The server uses stdio transport and requires authentication via 'suga login'.`, + SilenceUsage: true,cli/internal/mcp/server.go (2)
214-218: Prefer structured JSON content over plain text.Returning JSON as TextContent forces clients to parse strings. If supported by the SDK, emit application/json content instead.
Example (pattern):
- Content: []mcp.Content{ - &mcp.TextContent{Text: string(result)}, - }, + Content: []mcp.Content{ + &mcp.Blob{ + MIMEType: "application/json", + Data: result, + }, + },Apply similarly to other handlers producing JSON.
Also applies to: 242-246, 278-282, 320-324, 356-360, 392-396, 428-432, 464-468
36-43: Version constant.Extract "1.0.0" to a const to track server version centrally.
- mcpServer := mcp.NewServer(&mcp.Implementation{ - Name: "suga-mcp", - Version: "1.0.0", - }, &mcp.ServerOptions{ + const serverVersion = "1.0.0" + mcpServer := mcp.NewServer(&mcp.Implementation{ + Name: "suga-mcp", + Version: serverVersion, + }, &mcp.ServerOptions{cli/go.mod (1)
102-102: Movegithub.com/modelcontextprotocol/go-sdkto direct dependencies incli/go.mod.The module is directly imported in
cli/internal/mcp/server.go:9but currently marked indirect at line 102. Move it to the direct require block and rungo mod tidyto clean up the indirect entry.Apply the suggested diff and run
go mod tidyfrom thecli/directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
cli/cmd/mcp.go(1 hunks)cli/cmd/root.go(1 hunks)cli/go.mod(1 hunks)cli/internal/api/platform.go(1 hunks)cli/internal/api/plugin.go(1 hunks)cli/internal/mcp/instructions.md(1 hunks)cli/internal/mcp/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cli/cmd/mcp.go (4)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(20-24)cli/internal/mcp/server.go (1)
NewServer(28-56)
cli/internal/api/platform.go (4)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/api/models.go (2)
PlatformResponse(48-56)PlatformsResponse(58-60)cli/internal/api/errors.go (2)
ErrNotFound(7-7)ErrUnauthenticated(5-5)cli/internal/version/version.go (1)
ProductName(10-10)
cli/cmd/root.go (1)
cli/cmd/mcp.go (1)
NewMcpCmd(19-70)
cli/internal/api/plugin.go (4)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/api/models.go (4)
PluginLibraryWithVersions(171-174)ListPluginLibrariesResponse(182-184)PluginLibraryVersion(176-180)GetPluginLibraryVersionResponse(190-192)cli/internal/api/errors.go (2)
ErrNotFound(7-7)ErrUnauthenticated(5-5)cli/internal/version/version.go (2)
ProductName(10-10)Version(33-33)
cli/internal/mcp/server.go (5)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(20-24)cli/internal/api/models.go (4)
Platform(70-79)PlatformResponse(48-56)PluginLibraryWithVersions(171-174)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
🪛 LanguageTool
cli/internal/mcp/instructions.md
[grammar] ~168-~168: Ensure spelling is correct
Context: ...rm inputs and variables are NEVER valid configration in a suga.yaml file 5. Generate config...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (2)
cli/cmd/root.go (1)
30-45: The review comment is incorrect.Both
SugaApiClientandBuilderServiceare already registered in main.go viado.Provideas factory functions. Config is provided inPersistentPreRunEbefore any command'sRunEexecutes, which is exactly when the MCP command needs it. The factories will invoke successfully: NewSugaApiClient requires Config (now available), and NewBuilderService requires Fs (already provided in main.go) and SugaApiClient (factory chain resolved).Likely an incorrect or invalid review comment.
cli/go.mod (1)
3-3: Toolchain version confirmed—no action required.Verification confirms cli/go.mod specifies Go 1.24.3, which matches all CI workflows (.github/workflows/publish.yaml and test-cli.yaml) that also specify go-version: 1.24.3. The configuration is correct.
tjholm
force-pushed
the
suga-mcp
branch
2 times, most recently
from
970e911 to
dd46551
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/internal/mcp/instructions-app-development.md(1 hunks)cli/internal/mcp/instructions-main.md(1 hunks)cli/internal/mcp/instructions-platform-development.md(1 hunks)cli/internal/mcp/server.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cli/internal/mcp/instructions-app-development.md
🧰 Additional context used
🧬 Code graph analysis (1)
cli/internal/mcp/server.go (6)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(20-24)cli/internal/api/teams.go (1)
Team(8-16)cli/internal/api/models.go (4)
Platform(70-79)PlatformResponse(48-56)PluginLibraryWithVersions(171-174)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
🪛 markdownlint-cli2 (0.18.1)
cli/internal/mcp/instructions-platform-development.md
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
74-74: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Security Scan
- GitHub Check: Test
🔇 Additional comments (9)
cli/internal/mcp/server.go (8)
1-24: LGTM!Clean package structure with appropriate imports and embedded documentation.
25-62: LGTM!Well-structured constructor with proper error handling and clear initialization flow.
64-87: LGTM!Team resolution logic is clear and handles the default case appropriately.
89-143: LGTM!Input types are well-defined with clear jsonschema annotations for MCP tool binding.
145-202: LGTM!Tool registration is comprehensive and well-organized with clear descriptions.
204-231: LGTM!Resource registration follows MCP patterns with consistent URI scheme and appropriate MIME types.
233-236: LGTM!Clean server execution with stdio transport.
641-679: LGTM!Resource handlers correctly return embedded documentation with appropriate content types.
cli/internal/mcp/instructions-main.md (1)
1-54: LGTM!Clear, well-organized user documentation with helpful scenario-based guidance.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/internal/mcp/server.go (2)
251-601: Consider extracting common handler pattern.All eight handlers follow identical structure: resolve team, call API, marshal JSON, return result. While this works correctly, consolidating into a helper would reduce duplication and improve maintainability. Since the pattern is consistent, a single
handleWithTeamhelper accepting closures for team extraction and API calls would streamline these ~350 lines.
603-650: CRITICAL: Path traversal vulnerability remains unaddressed.Lines 614-619 accept user-provided
project_filepaths without validation. An attacker could exploit this with paths like../../etc/passwdto read arbitrary files. The previous review's fix is correct and must be applied before release.Apply path validation:
+import ( + "os" + "path/filepath" + "strings" +) + func (s *Server) handleBuild(ctx context.Context, req *mcp.CallToolRequest, args BuildArgs) (*mcp.CallToolResult, any, error) { team, err := s.getTeamOrDefault(args.Team) if err != nil { return &mcp.CallToolResult{ IsError: true, Content: []mcp.Content{ &mcp.TextContent{Text: fmt.Sprintf("Failed to get team: %v", err)}, }, }, nil, nil } projectFile := args.ProjectFile if projectFile == "" { projectFile = "./suga.yaml" } + + // Prevent path traversal + clean := filepath.Clean(projectFile) + absProject, err := filepath.Abs(clean) + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Invalid project file path: %v", err)}}}, nil, nil + } + wd, err := os.Getwd() + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Cannot determine working directory: %v", err)}}}, nil, nil + } + if !strings.HasPrefix(absProject, wd+string(os.PathSeparator)) && absProject != filepath.Join(wd, "suga.yaml") { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: "project_file must be within the current workspace"}}}, nil, nil + } - stackPath, err := s.builder.BuildProjectFromFile(projectFile, team) + stackPath, err := s.builder.BuildProjectFromFile(absProject, team)
🧹 Nitpick comments (1)
cli/internal/mcp/instructions-plugin-library-development.md (1)
977-980: Optional: Format URLs as markdown links.Lines 977 and 979 use bare URLs. While functional, wrapping them in angle brackets or markdown link syntax improves rendering in some markdown viewers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/internal/mcp/instructions-main.md(1 hunks)cli/internal/mcp/instructions-plugin-library-development.md(1 hunks)cli/internal/mcp/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/internal/mcp/server.go (6)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(20-24)cli/internal/api/teams.go (1)
Team(8-16)cli/internal/api/models.go (4)
Platform(70-79)PlatformResponse(48-56)PluginLibraryWithVersions(171-174)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
🪛 markdownlint-cli2 (0.18.1)
cli/internal/mcp/instructions-plugin-library-development.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
977-977: Bare URL used
(MD034, no-bare-urls)
979-979: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
cli/internal/mcp/instructions-main.md (1)
1-70: LGTM! Clear and well-structured usage guide.The three-scenario breakdown (Application, Platform, Plugin Library Development) provides clear entry points for different user workflows. The authentication requirements and team parameter defaults are well-documented, and the Critical Reminder about querying MCP tools is valuable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/internal/mcp/instructions-platform-development.md (1)
73-81: Add language specifiers to all fenced code blocks.Four code blocks lack language identifiers, affecting syntax highlighting. All three response examples should be
json, and the command output should betextorshell.- ### Step 1: Discover Available Plugins - - Use MCP tools to find plugins to compose: - - 1. **Call `list_plugin_libraries()`** to see available libraries - ``` - → Returns: [ + ```json + → Returns: [- 2. **Call `get_plugin_library_version(library: "aws", library_version: "v1.0.0")`** - ``` + ```json → Returns: {- 3. **Call `get_plugin_manifest(library: "aws", library_version: "v1.0.0", plugin_name: "fargate")`** - ``` + ```json → Returns: {- 3. **Test with `get_build_manifest`**: - ``` + ```text After publishing your platform: get_build_manifest(platform: "your-platform", revision: 1)Also applies to: 84-94, 97-114, 523-526
🧹 Nitpick comments (2)
cli/internal/mcp/instructions-plugin-library-development.md (2)
31-52: Add language specifier to directory structure block.Line 31 opens a fenced code block for the repository layout but lacks a language identifier. Use
textfor file tree structures.- ### Repository Layout - - ``` + ```text plugins-aws/ # Plugin library repository
988-990: Wrap bare URLs in markdown link syntax.Lines 988 and 990 contain bare URLs that should be wrapped in markdown link format for better rendering.
## Resources - - - **Terraform Documentation** - https://www.terraform.io/docs - - **Provider Documentation** - Refer to specific provider docs (AWS, GCP, etc.) - - **Go Documentation** - https://go.dev/doc/ + - **Terraform Documentation** - [https://www.terraform.io/docs](https://www.terraform.io/docs) + - **Provider Documentation** - Refer to specific provider docs (AWS, GCP, etc.) + - **Go Documentation** - [https://go.dev/doc/](https://go.dev/doc/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/internal/mcp/instructions-app-development.md(1 hunks)cli/internal/mcp/instructions-main.md(1 hunks)cli/internal/mcp/instructions-platform-development.md(1 hunks)cli/internal/mcp/instructions-plugin-library-development.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/internal/mcp/instructions-main.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
cli/internal/mcp/instructions-platform-development.md
73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
523-523: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
cli/internal/mcp/instructions-plugin-library-development.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
988-988: Bare URL used
(MD034, no-bare-urls)
990-990: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Security Scan
- GitHub Check: Build (windows-latest, amd64)
- GitHub Check: Test
🔇 Additional comments (1)
cli/internal/mcp/instructions-app-development.md (1)
1-1: No issues identified.The guide is well-structured and provides clear workflow guidance. Documentation formatting and content are sound.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/internal/mcp/server.go (2)
267-319: Nice extraction of the common handler.This addresses earlier feedback to DRY the team/JSON pattern. Looks good.
473-520: Blocker: path traversal in build tool; restrict project_file to workspace.An attacker can read/build outside the repo via "../../". Normalize, resolve symlinks, and enforce workspace containment before calling BuildProjectFromFile.
Apply imports (top block):
import ( "context" _ "embed" "encoding/json" "fmt" "log" + "os" + "path/filepath" + "strings" )Harden handleBuild:
- projectFile := args.ProjectFile - if projectFile == "" - projectFile = "./suga.yaml" - } - - stackPath, err := s.builder.BuildProjectFromFile(projectFile, team) + projectFile := args.ProjectFile + if projectFile == "" { + projectFile = "./suga.yaml" + } + clean := filepath.Clean(projectFile) + absProject, err := filepath.Abs(clean) + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Invalid project file path: %v", err)}}}, nil, nil + } + wd, err := os.Getwd() + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Cannot determine working directory: %v", err)}}}, nil, nil + } + // Resolve symlinks and ensure path is within workspace + resolved, err := filepath.EvalSymlinks(absProject) + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Invalid project file path: %v", err)}}}, nil, nil + } + rel, err := filepath.Rel(wd, resolved) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: "project_file must be within the current workspace"}}}, nil, nil + } + + stackPath, err := s.builder.BuildProjectFromFile(resolved, team)I can add unit tests for traversal attempts and symlink escapes.
🧹 Nitpick comments (7)
cli/internal/mcp/server.go (7)
156-164: Add a short timeout when connecting to the docs proxy.Prevent hangs on startup with a bounded connect.
- ctx := context.Background() - if err := s.docsProxy.Connect(ctx); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + if err := s.docsProxy.Connect(ctx); err != nil { log.Printf("Warning: failed to connect to docs server, documentation features will be unavailable: %v", err) } else {And import:
+ "time"
349-367: Public calls without a team require auth today; add fallback to configured public team.If args.Public is true and team is empty, prefer the first configured public team (e.g., config.PublicTemplatesTeams[0]) instead of requiring auth.
Example helper:
func (s *Server) getTeamFor(public bool, supplied string) (string, error) { if supplied != "" { return supplied, nil } if public && len(s.config.PublicTemplatesTeams) > 0 { return s.config.PublicTemplatesTeams[0], nil } return s.getCurrentTeam() }Then either:
- Extend handleWithTeamAndJSON to accept a publicExtractor func() bool and resolve via getTeamFor, or
- Precompute team in each handler and pass a closure that returns it.
Please confirm intended UX for unauthenticated public reads.
Also applies to: 369-391, 393-411, 413-431, 433-451, 453-471
349-351: Fail fast on invalid args (name empty, revision <= 0).Add minimal checks before API calls.
func (s *Server) handleGetPlatform(ctx context.Context, req *mcp.CallToolRequest, args GetPlatformArgs) (*mcp.CallToolResult, any, error) { + if args.Name == "" { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: "name is required"}}}, nil, nil + } + if args.Revision <= 0 { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: "revision must be > 0"}}}, nil, nil + } return s.handleWithTeamAndJSON(Repeat similar guards in handleGetBuildManifest (platform/revision), handleGetPluginManifest (library/library_version/plugin_name), and handleGetPluginLibraryVersion (library/library_version).
47-52: Avoid hard-coding the MCP implementation version.Wire this to your CLI version via ldflags or a version package to prevent drift.
Minimal option:
- Version: "1.0.0", + Version: mcpVersion,Add near the top of this file:
var mcpVersion = "dev" // override with: -ldflags "-X github.com/nitrictech/suga/cli/internal/mcp.mcpVersion=$(VERSION)"
581-593: Harden docs proxy tool handler with panic recovery.Protect the server from panics in proxied tools.
- mcp.AddTool(s.mcpServer, tool, func(ctx context.Context, req *mcp.CallToolRequest, args map[string]interface{}) (*mcp.CallToolResult, any, error) { - result, err := s.docsProxy.CallTool(ctx, toolName, args) + mcp.AddTool(s.mcpServer, tool, func(ctx context.Context, req *mcp.CallToolRequest, args map[string]interface{}) (res *mcp.CallToolResult, meta any, err error) { + defer func() { + if r := recover(); r != nil { + res = &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Docs tool panic: %v", r)}}, + } + meta, err = nil, nil + } + }() + res, err = s.docsProxy.CallTool(ctx, toolName, args) - if err != nil { - return &mcp.CallToolResult{ + if err != nil { + return &mcp.CallToolResult{ IsError: true, Content: []mcp.Content{ &mcp.TextContent{Text: fmt.Sprintf("Failed to call docs tool: %v", err)}, }, - }, nil, nil - } - return result, nil, nil + }, nil, nil + } + return res, nil, nil })
103-107: Unify team arg naming; keep back-compat.Prefer "team" across tools but accept "team_slug" for compatibility.
type GetTemplateArgs struct { - TeamSlug string `json:"team_slug,omitempty" jsonschema:"Team slug that owns the template (defaults to current team if not specified)"` + Team string `json:"team,omitempty" jsonschema:"Team slug that owns the template (defaults to current team if not specified)"` + TeamSlug string `json:"team_slug,omitempty" jsonschema:"Deprecated: use 'team'"` TemplateName string `json:"template_name" jsonschema:"Name of the template"` Version string `json:"version,omitempty" jsonschema:"Version of the template (optional defaults to latest)"` }- return s.handleWithTeamAndJSON( - func() string { return args.TeamSlug }, + return s.handleWithTeamAndJSON( + func() string { + if args.Team != "" { + return args.Team + } + return args.TeamSlug + },Also applies to: 335-347
524-536: Refactor ApplicationJsonSchemaString to return (string, error) and handle errors at call sites.The concern is valid:
log.Fatalon line 91 inApplicationJsonSchemaStringwill crash the server ifjson.Marshalfails during a resource read. Both callers—handleApplicationSchemaandApplicationFromJson—already return error types and can handle this change without issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/internal/mcp/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/internal/mcp/server.go (6)
cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(20-24)cli/internal/mcp/docs_proxy.go (2)
DocsProxy(17-24)NewDocsProxy(27-29)cli/internal/api/teams.go (1)
Team(8-16)cli/internal/api/models.go (4)
Platform(70-79)PlatformResponse(48-56)PluginLibraryWithVersions(171-174)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows-latest, amd64)
- GitHub Check: Test
- GitHub Check: Security Scan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/internal/mcp/server.go (1)
522-583: Use validated absolute path for consistency.The path traversal protection correctly validates
absProject(line 540-550), but then passescleanto the builder (line 552) instead ofabsProject. For consistency with the validation logic and to eliminate ambiguity, use the validated absolute path.Apply this diff:
- stackPath, err := s.builder.BuildProjectFromFile(clean, team) + stackPath, err := s.builder.BuildProjectFromFile(absProject, team)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/internal/api/platform.go(1 hunks)cli/internal/api/plugin.go(1 hunks)cli/internal/mcp/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cli/internal/api/plugin.go (4)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/api/models.go (4)
PluginLibraryWithVersions(171-174)ListPluginLibrariesResponse(182-184)PluginLibraryVersion(176-180)GetPluginLibraryVersionResponse(190-192)cli/internal/api/errors.go (2)
ErrNotFound(7-7)ErrUnauthenticated(5-5)cli/internal/version/version.go (2)
ProductName(10-10)Version(33-33)
cli/internal/api/platform.go (4)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/api/models.go (2)
PlatformResponse(48-56)PlatformsResponse(58-60)cli/internal/api/errors.go (2)
ErrNotFound(7-7)ErrUnauthenticated(5-5)cli/internal/version/version.go (1)
ProductName(10-10)
cli/internal/mcp/server.go (7)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(20-24)cli/internal/mcp/docs_proxy.go (2)
DocsProxy(17-24)NewDocsProxy(27-29)cli/internal/api/teams.go (1)
Team(8-16)cli/internal/api/models.go (2)
Platform(70-79)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows-latest, amd64)
- GitHub Check: Security Scan
- GitHub Check: Test
🔇 Additional comments (4)
cli/internal/api/plugin.go (1)
98-216: LGTM! Consistent API client implementation.All four methods follow the established patterns in this codebase:
- Proper URL escaping for path parameters
- Consistent error handling (404, 401 for authenticated endpoints, non-200 fallback)
- Response body read and unmarshaled into appropriate types
- Public endpoints correctly omit 401 checks
cli/internal/api/platform.go (1)
74-132: LGTM! Consistent with existing patterns.Both
ListPlatformsandListPublicPlatformsfollow the same well-established patterns as the existingGetPlatformmethods in this file and the new plugin library methods.cli/internal/mcp/server.go (2)
270-322: Excellent refactoring of common handler pattern.The
handleWithTeamAndJSONhelper cleanly extracts the repeated logic (team resolution → API call → optional transform → JSON marshaling) with proper error handling at each step. This addresses the past review feedback effectively.
42-73: Well-structured server initialization and registration.The constructor pattern with separate tool and resource registration, plus graceful degradation for docs proxy connection failures, provides good separation of concerns and resilience.
Also applies to: 157-224, 226-261
|
Ran some more tests and ran into the following.
Also it can be convinced to run suga dev in the terminal but cause its a blocking command it gets stuck waiting for a response. I think if its done via a tool call it can continue knowing its running in the background, but would love to see this work, so it can run tests |
Have added an example to the app dev example. For running suga dev as a tool, I'd like to do this as a separate PR as its not as simple as just adding a single tool for running suga dev we need to add a workflow and process management so the LLM has tools it can use to stop dev as well. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/internal/mcp/instructions-platform-development.md (1)
73-114: Add language specifiers to fenced code blocks.Four code blocks lack language hints, affecting syntax highlighting. Lines 73, 84, 97 show JSON API responses; line 523 shows command output.
Apply this diff to add language specifiers:
2. **Call `list_plugin_libraries()`** to see available libraries
→ Returns: [```diff 2. **Call `get_plugin_library_version(library: "aws", library_version: "v1.0.0")`**
→ Returns: {```diff 3. **Call `get_plugin_manifest(library: "aws", library_version: "v1.0.0", plugin_name: "fargate")`**
→ Returns: {```diff 1. **Verify all plugin inputs are satisfied**: - Required inputs must be in `properties:` - Check the plugin manifest for what's required 2. **Ensure dependencies exist**: - Resources in `depends_on:` must be defined - Outputs referenced must exist in the plugin manifest 3. **Test with `get_build_manifest`**: ``` + ```shell After publishing your platform: get_build_manifest(platform: "your-platform", revision: 1) ```Also applies to: 523-526
🧹 Nitpick comments (4)
cli/internal/mcp/instructions-plugin-library-development.md (2)
31-52: Add language tag to fenced code block.The directory structure should specify a language tag (e.g.,
textorbash) per markdown linting standards.-``` +```text plugins-aws/ # Plugin library repository
986-991: Format bare URLs as markdown links.Wrap bare URLs in markdown link syntax to follow markdown linting conventions.
## Resources -- **Terraform Documentation** - https://www.terraform.io/docs -- **Provider Documentation** - Refer to specific provider docs (AWS, GCP, etc.) -- **Go Documentation** - https://go.dev/doc/ +- **[Terraform Documentation](https://www.terraform.io/docs)** +- **Provider Documentation** - Refer to specific provider docs (AWS, GCP, etc.) +- **[Go Documentation](https://go.dev/doc/)**cli/internal/mcp/docs_proxy.go (1)
42-45: Add timeout to HTTP client.The HTTP client has no timeout configured, which could cause indefinite hangs if the docs server is unresponsive. Consider adding reasonable timeouts.
transport := &mcp.StreamableClientTransport{ Endpoint: docsServerURL, - HTTPClient: &http.Client{}, + HTTPClient: &http.Client{ + Timeout: 30 * time.Second, + }, }cli/internal/mcp/server.go (1)
538-552: Path validation uses absProject but passes clean to builder.After validating
absProject, line 552 passescleanto the builder. This is inconsistent - the validation checksabsProjectis within the workspace, but then uses a different path. UseabsProjectfor consistency with the validation logic.- stackPath, err := s.builder.BuildProjectFromFile(clean, team) + stackPath, err := s.builder.BuildProjectFromFile(absProject, team)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cli/cmd/mcp.go(1 hunks)cli/cmd/root.go(1 hunks)cli/go.mod(3 hunks)cli/internal/api/platform.go(1 hunks)cli/internal/api/plugin.go(1 hunks)cli/internal/mcp/docs_proxy.go(1 hunks)cli/internal/mcp/instructions-app-development.md(1 hunks)cli/internal/mcp/instructions-main.md(1 hunks)cli/internal/mcp/instructions-platform-development.md(1 hunks)cli/internal/mcp/instructions-plugin-library-development.md(1 hunks)cli/internal/mcp/server.go(1 hunks)cli/pkg/app/suga.go(3 hunks)docs/.vale/styles/config/vocabularies/Suga/accept.txt(1 hunks)docs/guides/mcp-integration.mdx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- cli/internal/api/platform.go
- cli/internal/api/plugin.go
- cli/cmd/mcp.go
- cli/internal/mcp/instructions-main.md
- docs/.vale/styles/config/vocabularies/Suga/accept.txt
- cli/cmd/root.go
- cli/go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
cli/pkg/app/suga.go (1)
cli/internal/mcp/server.go (1)
NewServer(42-73)
cli/internal/mcp/server.go (7)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (1)
BuilderService(21-25)cli/internal/mcp/docs_proxy.go (2)
DocsProxy(17-24)NewDocsProxy(27-29)cli/internal/api/teams.go (1)
Team(8-16)cli/internal/api/models.go (2)
Platform(70-79)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
🪛 GitHub Check: Lint
cli/pkg/app/suga.go
[failure] 24-24:
could not import github.com/nitrictech/suga/cli/internal/mcp (-: # github.com/nitrictech/suga/cli/internal/mcp
🪛 markdownlint-cli2 (0.18.1)
cli/internal/mcp/instructions-platform-development.md
73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
523-523: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
cli/internal/mcp/instructions-plugin-library-development.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
988-988: Bare URL used
(MD034, no-bare-urls)
990-990: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
cli/internal/mcp/instructions-platform-development.md (1)
1-705: Comprehensive platform development guide.Well-structured documentation with clear workflow, concrete examples, validation checklist, and emphasis on using MCP tools for discovery. Good emphasis on avoiding training data assumptions and consulting plugin manifests as source of truth.
cli/internal/mcp/instructions-plugin-library-development.md (1)
1-991: Comprehensive and well-structured guide.The guide effectively covers plugin development from architecture through best practices. Technical examples are accurate, realistic, and demonstrate proper patterns for both Terraform and Go runtime code. Clear distinction between plugins requiring runtime code (services, buckets) and those that don't (infrastructure, identity) is helpful.
cli/pkg/app/suga.go (1)
665-699: MCP implementation looks solid.The signal handling and graceful shutdown logic is well-implemented. Context cancellation properly coordinates shutdown between the signal handler and server goroutine.
cli/internal/mcp/instructions-app-development.md (1)
16-16: SearchSugaDocs is properly registered via the docs proxy integration.The tool isn't directly registered in
server.gobut is dynamically proxied from the external docs server athttps://docs.addsuga.com/mcpviaregisterDocsProxyTools()(line 638), which fetches and registers tools at runtime. The documentation is accurate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/internal/mcp/server.go (1)
538-552: Use validated real path and pass it to the builder; also resolve symlinks.Currently you validate
absProjectbut passcleanto the builder; symlinks can also bypass the prefix check.Apply:
// Prevent path traversal clean := filepath.Clean(projectFile) absProject, err := filepath.Abs(clean) if err != nil { return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Invalid project file path: %v", err)}}}, nil, nil } - wd, err := os.Getwd() + // Resolve symlinks to prevent escaping the workspace via symlink + realProject, err := filepath.EvalSymlinks(absProject) + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Invalid project file path (symlink): %v", err)}}}, nil, nil + } + wd, err := os.Getwd() if err != nil { return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Cannot determine working directory: %v", err)}}}, nil, nil } - if !strings.HasPrefix(absProject, wd+string(os.PathSeparator)) && absProject != filepath.Join(wd, "suga.yaml") { + wdReal, err := filepath.EvalSymlinks(wd) + if err != nil { + return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Cannot resolve working directory: %v", err)}}}, nil, nil + } + if !strings.HasPrefix(realProject, wdReal+string(os.PathSeparator)) { return &mcp.CallToolResult{IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: "project_file must be within the current workspace"}}}, nil, nil } - stackPath, err := s.builder.BuildProjectFromFile(clean, team, build.BuildOptions{}) + stackPath, err := s.builder.BuildProjectFromFile(realProject, team, build.BuildOptions{})
🧹 Nitpick comments (7)
cli/internal/mcp/server.go (7)
49-55: Avoid hardcoding server version; use a single source of truth.Prefer a package-level const or module version to keep CLI and MCP consistent.
159-167: Wrap docs-proxy connect with a short timeout.Prevents blocking startup if the docs server is slow/unreachable.
- // Connect to docs proxy and register docs tools first - ctx := context.Background() - if err := s.docsProxy.Connect(ctx); err != nil { + // Connect to docs proxy and register docs tools first (with timeout) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + if err := s.docsProxy.Connect(ctx); err != nil { log.Printf("Warning: failed to connect to docs server, documentation features will be unavailable: %v", err) } else {Add import:
+ "time"
75-99: Cache current team to reduce API calls.
getCurrentTeam()hits the API every time; cache the slug for the session and invalidate on auth/team changes.Can you confirm whether
SugaApiClientexposes “current team” already? If so, wire that in here rather than re-listing teams for each call.
106-111: Add minimal input validation and align arg naming.
- Validate required fields (non-empty name/library/version and revision > 0) before making API calls to return clearer MCP errors.
- Consider standardizing on
teamfor consistency; if changing is breaking, deprecateteam_slugwith clear docs.Example diffs:
func (s *Server) handleGetPlatform(ctx context.Context, req *mcp.CallToolRequest, args GetPlatformArgs) (*mcp.CallToolResult, any, error) { + if strings.TrimSpace(args.Name) == "" || args.Revision <= 0 { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: "name is required and revision must be > 0"}}, + }, nil, nil + }func (s *Server) handleGetBuildManifest(ctx context.Context, req *mcp.CallToolRequest, args GetBuildManifestArgs) (*mcp.CallToolResult, any, error) { + if strings.TrimSpace(args.Platform) == "" || args.Revision <= 0 { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: "platform is required and revision must be > 0"}}, + }, nil, nil + }func (s *Server) handleGetPluginManifest(ctx context.Context, req *mcp.CallToolRequest, args GetPluginManifestArgs) (*mcp.CallToolResult, any, error) { + if strings.TrimSpace(args.Library) == "" || strings.TrimSpace(args.LibraryVersion) == "" || strings.TrimSpace(args.PluginName) == "" { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: "library, library_version, and plugin_name are required"}}, + }, nil, nil + }func (s *Server) handleGetPluginLibraryVersion(ctx context.Context, req *mcp.CallToolRequest, args GetPluginLibraryVersionArgs) (*mcp.CallToolResult, any, error) { + if strings.TrimSpace(args.Library) == "" || strings.TrimSpace(args.LibraryVersion) == "" { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: "library and library_version are required"}}, + }, nil, nil + }Also applies to: 112-117, 119-125, 144-150
270-322: Let handlers propagate ctx into API calls via the helper.Plumb
context.ContextthroughhandleWithTeamAndJSONso API calls can observe cancellation/timeouts.-func (s *Server) handleWithTeamAndJSON( - teamExtractor func() string, - apiCall func(team string) (interface{}, error), - resultTransform func(interface{}) (interface{}, error), -) (*mcp.CallToolResult, any, error) { +func (s *Server) handleWithTeamAndJSON( + ctx context.Context, + teamExtractor func() string, + apiCall func(context.Context, string) (interface{}, error), + resultTransform func(interface{}) (interface{}, error), +) (*mcp.CallToolResult, any, error) { @@ - result, err := apiCall(team) + result, err := apiCall(ctx, team)Call sites would pass
ctxand adapt closures to accept it.
416-457: Deduplicate JSON marshalling for public list handlers.Both handlers inline identical marshal/error logic. Extract a tiny helper to reduce duplication.
+func marshalResult(v any) (*mcp.CallToolResult, any, error) { + b, err := json.MarshalIndent(v, "", " ") + if err != nil { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("Failed to marshal result: %v", err)}}, + }, nil, nil + } + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: string(b)}}}, nil, nil +}Then in both branches:
- resultJSON, err := json.MarshalIndent(platforms, "", " ") - if err != nil { ... } - return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: string(resultJSON)}}}, nil, nil + return marshalResult(platforms)Also applies to: 459-500
639-657: Prevent tool name collisions with docs-proxy tools.If a docs tool shares a name with a built-in tool, registration may collide or shadow. Prefix proxied tools.
for _, tool := range tools { // Create a closure to capture the tool name for this iteration toolName := tool.Name // Register a proxy handler for this tool - mcp.AddTool(s.mcpServer, tool, func(ctx context.Context, req *mcp.CallToolRequest, args map[string]interface{}) (*mcp.CallToolResult, any, error) { + t := *tool + t.Name = "docs/" + toolName + mcp.AddTool(s.mcpServer, &t, func(ctx context.Context, req *mcp.CallToolRequest, args map[string]interface{}) (*mcp.CallToolResult, any, error) { result, err := s.docsProxy.CallTool(ctx, toolName, args)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/internal/mcp/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/internal/mcp/server.go (7)
cli/internal/api/api.go (1)
SugaApiClient(24-29)cli/internal/config/config.go (1)
Config(16-20)cli/internal/build/build.go (2)
BuilderService(21-25)BuildOptions(34-36)cli/internal/mcp/docs_proxy.go (2)
DocsProxy(17-24)NewDocsProxy(27-29)cli/internal/api/teams.go (1)
Team(8-16)cli/internal/api/models.go (2)
Platform(70-79)PluginLibraryVersion(176-180)cli/pkg/schema/schema.go (1)
ApplicationJsonSchemaString(86-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (windows-latest, amd64)
- GitHub Check: Build (macos-latest, arm64)
|
🎉 This PR is included in version 0.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
5 participants
Needs much more instruction and prompt refinement, but here is PR for review and testing.
Closes NIT-412