Add Google Stitch integration#105
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
acmacalister
left a comment
acmacalister
left a comment
There was a problem hiding this comment.
Solid integration — clean architecture, good test coverage, nice use of field compaction and RetryableError. A few things worth fixing before merge, mostly around URL path escaping and some empty-string serialization that could corrupt API calls.
| if name == "" { | ||
| return &mcp.ToolResult{Data: "name parameter is required", IsError: true}, nil | ||
| } | ||
| data, err := s.get(ctx, "/%s", name) |
There was a problem hiding this comment.
name here is user-supplied (e.g. projects/4044680601076201931) and goes straight into the URL path without escaping. A value containing ?, #, or % would corrupt the request URL — or worse, inject query parameters into the upstream call. The other handlers in this file call url.PathEscape(projectID) correctly; this one should too.
data, err := s.get(ctx, "/%s", url.PathEscape(name))Same issue in screens.go:36 (getScreen) and design_systems.go:144 (updateDesignSystem).
| if name == "" { | ||
| return &mcp.ToolResult{Data: "name parameter is required", IsError: true}, nil | ||
| } | ||
| data, err := s.get(ctx, "/%s", name) |
There was a problem hiding this comment.
Same unescaped name issue as getProject in projects.go:49 — should use url.PathEscape(name) before path interpolation.
| }, | ||
| } | ||
|
|
||
| path := fmt.Sprintf("/%s", name) |
There was a problem hiding this comment.
Same unescaped name issue — name like assets/15996705518239280238 is safe today, but url.PathEscape is still the right call here for consistency and to guard against unexpected inputs.
path := fmt.Sprintf("/%s", url.PathEscape(name))|
|
||
| body := map[string]any{ | ||
| "name": name, | ||
| "projectId": projectID, |
There was a problem hiding this comment.
projectId is always set in the PATCH body, even as "" when the caller doesn't supply it. The sibling function createDesignSystem guards this correctly:
// createDesignSystem (line 82)
if projectID != "" {
body["projectId"] = projectID
}updateDesignSystem drops that guard, so every PATCH call sends "projectId": "" to the API when the field is absent — which could disassociate the design system from its project.
| return &mcp.ToolResult{Data: "display_name parameter is required", IsError: true}, nil | ||
| } | ||
|
|
||
| theme := map[string]any{ |
There was a problem hiding this comment.
All five theme fields are unconditionally included in the map even when empty, so a call that omits color_mode will send "colorMode": "" to the API. On a PATCH endpoint (updateDesignSystem has the same shape at line 111), that would overwrite existing values with empty strings.
Compare: colorVariant and designMd are correctly guarded with if != "" before being set. The five required-looking fields should either be validated as non-empty upfront, or guarded the same way:
theme := map[string]any{}
if colorMode != "" { theme["colorMode"] = colorMode }
if headlineFont != "" { theme["headlineFont"] = headlineFont }
// etc.| return &mcp.ToolResult{Data: "name parameter is required", IsError: true}, nil | ||
| } | ||
|
|
||
| theme := map[string]any{ |
There was a problem hiding this comment.
Same empty-string serialization issue as createDesignSystem:47 — the theme map in updateDesignSystem will send "colorMode": "", "headlineFont": "", etc. for any fields the caller omits. On a PATCH this will clobber the existing design system's theme fields with empty strings.
|
|
||
| func New() mcp.Integration { | ||
| return &stitch{ | ||
| client: &http.Client{Timeout: 120 * time.Second}, |
There was a problem hiding this comment.
The tool descriptions for stitch_generate_screen_from_text, stitch_edit_screens, and stitch_generate_variants all say "Takes a few minutes to complete" — but the shared client timeout is 120s (2 min). These AI generation calls will likely hit the deadline before completing. Worth bumping to at least 300 * time.Second or using a per-request context with a longer deadline for the generation tools.
| return json.RawMessage(data), nil | ||
| } | ||
|
|
||
| func (s *stitch) get(ctx context.Context, pathFmt string, args ...any) (json.RawMessage, error) { |
There was a problem hiding this comment.
The get() helper accepts a format string, but post() and patch() take a plain path string. The format-string API creates a footgun: if a future caller passes user input as the format string itself (rather than as an argument), it opens up format string misinterpretation. Since all existing callers either pass a fixed literal format string or build the path externally, we could simplify this to match post/patch:
func (s *stitch) get(ctx context.Context, path string) (json.RawMessage, error) {
return s.doRequest(ctx, "GET", path, nil)
}Callers that currently use the format form would just build the path string before calling — which they mostly do anyway.
2 participants
Summary
cmd/server/main.goandconfig/config.goTest plan
make cipassessearchand executable viaexecutewith a validSTITCH_ACCESS_TOKEN