diff --git a/cmd/protoc-gen-go-mcp/mcp.go b/cmd/protoc-gen-go-mcp/mcp.go index 633a2e8..68a75b8 100644 --- a/cmd/protoc-gen-go-mcp/mcp.go +++ b/cmd/protoc-gen-go-mcp/mcp.go @@ -129,7 +129,7 @@ func generateMCPTool(g *protogen.GeneratedFile, method *protogen.Method, mcpServ g.P("func (s *", unexport(mcpServerName), ") ", method.GoName, "Tool() *", mcpPackage.Ident("Tool"), " {") g.P("return &", mcpPackage.Ident("Tool"), "{") g.P("Name: \"", method.GoName, "\",") - g.P("Description: \"", methodDescription, "\",") + g.P("Description: ", strconv.Quote(methodDescription), ",") g.P("InputSchema: ", jsonPackage.Ident("RawMessage"), "(", quoteBacktickString(string(schemaJSON)), "),") g.P("}") g.P("}") @@ -172,36 +172,50 @@ func generateHandler(g *protogen.GeneratedFile, method *protogen.Method, mcpServ } // processCommentToString turns a (possibly multi-line, possibly -// block-style) leading proto comment into a single-line, double-quote-safe -// string suitable for embedding in a Go string literal. +// block-style) leading proto comment into a single-line string with its +// raw text otherwise untouched: callers are responsible for escaping it +// for whatever they embed it in (a Go string literal via strconv.Quote, a +// JSON Schema "description" via encoding/json), so this never escapes +// quotes or backslashes itself - doing so here would corrupt the text when +// a caller's own encoding escapes it a second time. func processCommentToString(comments protogen.Comments) string { - // Remove comment markers and clean up the text + // protogen.Comments strings for a line comment ("// foo") already + // have the "// " prefix stripped from the first line and a trailing + // newline, but each continuation line still has its own "// " (the + // source text between consecutive "//" markers, including that + // prefix, becomes part of the comment string); a block comment + // ("/* foo */") instead keeps its "/* " prefix and " */" suffix on + // the string as a whole, with no per-line marker to strip. Strip the + // block markers first, then strip any remaining per-line "// " below + // (a no-op for an already-prefix-stripped block comment). commentText := string(comments) - // Remove leading comment markers and spaces commentText = strings.TrimPrefix(commentText, "// ") commentText = strings.TrimPrefix(commentText, "/* ") - - // Replace newlines with spaces - commentText = strings.ReplaceAll(commentText, "\n// ", " ") - commentText = strings.ReplaceAll(commentText, "\n", " ") - - // Remove trailing comment markers commentText = strings.TrimSuffix(commentText, " */") - // Clean up extra whitespace - commentText = strings.TrimSpace(commentText) + // Join every line (dropping any remaining "// " line-comment prefix) + // into a single space-separated line. + lines := strings.Split(commentText, "\n") + for i, line := range lines { + lines[i] = strings.TrimPrefix(line, "// ") + } + commentText = strings.Join(lines, " ") - // Replace multiple spaces with a single space - spaceRegex := regexp.MustCompile(`\s+`) - commentText = spaceRegex.ReplaceAllString(commentText, " ") + commentText = strings.TrimSpace(commentText) - // Escape quotes to prevent JSON issues - commentText = strings.ReplaceAll(commentText, "\"", "\\\"") + // Collapse runs of whitespace (including the spaces just introduced + // for blank comment lines) into a single space. + commentText = whitespaceRunRegexp.ReplaceAllString(commentText, " ") return commentText } +// whitespaceRunRegexp matches one or more consecutive whitespace +// characters, used by processCommentToString to collapse them to a single +// space. +var whitespaceRunRegexp = regexp.MustCompile(`\s+`) + func generateMcpServerStruct(g *protogen.GeneratedFile, mcpServerName string, clientName string) { g.P("type ", unexport(mcpServerName), " struct {") g.P(clientName) diff --git a/cmd/protoc-gen-go-mcp/mcp_test.go b/cmd/protoc-gen-go-mcp/mcp_test.go index 73076aa..b7c33f0 100644 --- a/cmd/protoc-gen-go-mcp/mcp_test.go +++ b/cmd/protoc-gen-go-mcp/mcp_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "google.golang.org/protobuf/compiler/protogen" ) func TestUnexport(t *testing.T) { @@ -15,3 +16,49 @@ func TestQuoteBacktickString(t *testing.T) { assert.Equal(t, "`abc`", quoteBacktickString("abc")) assert.Equal(t, `"a`+"`"+`b"`, quoteBacktickString("a`b")) } + +// TestProcessCommentToString covers the two bugs in issue #100: a +// backslash or backtick in a proto comment must come through completely +// untouched (callers, not this function, are responsible for escaping the +// text for whatever they embed it in), and a double quote must likewise +// come through unescaped rather than backslash-escaped here (which used to +// make json.Marshal double-escape it when schema.go embedded the result in +// a JSON Schema "description"). +func TestProcessCommentToString(t *testing.T) { + tests := []struct { + name string + in protogen.Comments + want string + }{ + { + name: "quote", + in: protogen.Comments(` Must match "chill" exactly.` + "\n"), + want: `Must match "chill" exactly.`, + }, + { + name: "backslash", + in: protogen.Comments(` Must match \d+.` + "\n"), + want: `Must match \d+.`, + }, + { + name: "backtick", + in: protogen.Comments(" Use the `vibe` field.\n"), + want: "Use the `vibe` field.", + }, + { + name: "multiline joins with a single space", + in: protogen.Comments(" First line.\n Second line.\n"), + want: "First line. Second line.", + }, + { + name: "block comment", + in: protogen.Comments("/* Block \"comment\" with \\backslash\\. */"), + want: `Block "comment" with \backslash\.`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, processCommentToString(tt.in)) + }) + } +} diff --git a/examples/gen/example/v1/e2e_test.go b/examples/gen/example/v1/e2e_test.go index a689b7d..9a74bf0 100644 --- a/examples/gen/example/v1/e2e_test.go +++ b/examples/gen/example/v1/e2e_test.go @@ -250,12 +250,12 @@ func TestToolsList(t *testing.T) { // fields, named with their protojson (lowerCamel) JSON name, not // nested under a property named after the request message. setVibe := byName["SetVibe"] - require.Equal(t, `This is a block comment with multiple lines to test block handling "Hello World"`, setVibe.Description) + require.Equal(t, "This is a block comment with multiple lines to test block handling \"Hello World\", a `backtick`, and a path like C:\\vibes\\new", setVibe.Description) require.Equal(t, "object", setVibe.InputSchema["type"]) require.Equal(t, false, setVibe.InputSchema["additionalProperties"]) require.Equal(t, map[string]any{ "vibe": map[string]any{ - "description": "The vibe of the server to be set", + "description": "The vibe of the server to be set. Must match \\d+ or a `code` like \"chill\", and must not contain a literal newline.", "type": "string", }, }, setVibe.InputSchema["properties"]) diff --git a/examples/gen/example/v1/example.pb.go b/examples/gen/example/v1/example.pb.go index f07cdd8..40a9dee 100644 --- a/examples/gen/example/v1/example.pb.go +++ b/examples/gen/example/v1/example.pb.go @@ -71,7 +71,8 @@ func (VibeScalar_VibeEnum) EnumDescriptor() ([]byte, []int) { // The request to set the vibe of the server type SetVibeRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - //The vibe of the server to be set + // The vibe of the server to be set. Must match \d+ or a `code` like + // "chill", and must not contain a literal newline. Vibe string `protobuf:"bytes,1,opt,name=vibe,proto3" json:"vibe,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/examples/gen/example/v1/example_grpc.pb.go b/examples/gen/example/v1/example_grpc.pb.go index 2dbfe21..ddab85c 100644 --- a/examples/gen/example/v1/example_grpc.pb.go +++ b/examples/gen/example/v1/example_grpc.pb.go @@ -44,7 +44,7 @@ type VibeServiceClient interface { // This is a block comment // with multiple lines // to test block handling - // "Hello World" + // "Hello World", a `backtick`, and a path like C:\vibes\new SetVibe(ctx context.Context, in *SetVibeRequest, opts ...grpc.CallOption) (*SetVibeResponse, error) // Get Vibe // of the server @@ -134,7 +134,7 @@ type VibeServiceServer interface { // This is a block comment // with multiple lines // to test block handling - // "Hello World" + // "Hello World", a `backtick`, and a path like C:\vibes\new SetVibe(context.Context, *SetVibeRequest) (*SetVibeResponse, error) // Get Vibe // of the server diff --git a/examples/gen/example/v1/example_mcp.pb.go b/examples/gen/example/v1/example_mcp.pb.go index ba52e25..9f731b9 100644 --- a/examples/gen/example/v1/example_mcp.pb.go +++ b/examples/gen/example/v1/example_mcp.pb.go @@ -31,8 +31,8 @@ func NewVibeServiceMCPServer( func (s *vibeServiceMCPServer) SetVibeTool() *mcp.Tool { return &mcp.Tool{ Name: "SetVibe", - Description: "This is a block comment with multiple lines to test block handling \"Hello World\"", - InputSchema: json.RawMessage(`{"additionalProperties":false,"description":"The request to set the vibe of the server","properties":{"vibe":{"description":"The vibe of the server to be set","type":"string"}},"type":"object"}`), + Description: "This is a block comment with multiple lines to test block handling \"Hello World\", a `backtick`, and a path like C:\\vibes\\new", + InputSchema: json.RawMessage("{\"additionalProperties\":false,\"description\":\"The request to set the vibe of the server\",\"properties\":{\"vibe\":{\"description\":\"The vibe of the server to be set. Must match \\\\d+ or a `code` like \\\"chill\\\", and must not contain a literal newline.\",\"type\":\"string\"}},\"type\":\"object\"}"), } } diff --git a/examples/protos/example.proto b/examples/protos/example.proto index bf9f311..00a7a64 100644 --- a/examples/protos/example.proto +++ b/examples/protos/example.proto @@ -6,7 +6,8 @@ option go_package = "examples/gen/example/v1;examplev1"; // The request to set the vibe of the server message SetVibeRequest { - //The vibe of the server to be set + // The vibe of the server to be set. Must match \d+ or a `code` like + // "chill", and must not contain a literal newline. string vibe = 1; } @@ -150,7 +151,7 @@ service VibeService { * This is a block comment * with multiple lines * to test block handling - * "Hello World" + * "Hello World", a `backtick`, and a path like C:\vibes\new */ rpc SetVibe(SetVibeRequest) returns (SetVibeResponse) {} // Get Vibe