From ea298614cd540a97a586aa43aaa58727787f9b7f Mon Sep 17 00:00:00 2001 From: sf-factory-bot Date: Thu, 8 Oct 2026 19:04:41 +0000 Subject: [PATCH 1/2] fix: escape proto comments correctly in descriptions and schemas (#100) Two bugs in how proto comments become Go string literals and JSON Schema descriptions: - processCommentToString escaped only ", so a comment containing a backslash (e.g. "matches \d+") produced an invalid Go string literal. Tool descriptions are now emitted with strconv.Quote instead of hand-wrapping the raw text in quotes, so any character valid in the source comment round-trips correctly. - processCommentToString also backslash-escaped " before the result was fed to schema.go's JSONSchema (later encoding/json-marshaled), so json.Marshal escaped it a second time and clients saw \\"quote\\" in schema descriptions. processCommentToString no longer escapes anything: callers are responsible for escaping the raw comment text for whatever they embed it in (strconv.Quote for a Go literal, encoding/json for a schema description). Added comments with a double quote, a backslash, a backtick and a newline to examples/protos/example.proto's SetVibeRequest.vibe field and the VibeService's SetVibe RPC, regenerated the example and golden file, and added a unit test (TestProcessCommentToString) covering each character directly. --- cmd/protoc-gen-go-mcp/mcp.go | 46 ++++++++++++--------- cmd/protoc-gen-go-mcp/mcp_test.go | 47 ++++++++++++++++++++++ examples/gen/example/v1/e2e_test.go | 4 +- examples/gen/example/v1/example.pb.go | 3 +- examples/gen/example/v1/example_grpc.pb.go | 4 +- examples/gen/example/v1/example_mcp.pb.go | 4 +- examples/protos/example.proto | 5 ++- 7 files changed, 86 insertions(+), 27 deletions(-) diff --git a/cmd/protoc-gen-go-mcp/mcp.go b/cmd/protoc-gen-go-mcp/mcp.go index 633a2e8..cd9db6b 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,46 @@ 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") have the + // "// " prefix already stripped from the first line (but not + // continuation lines) and a trailing newline; a block comment + // ("/* foo */") keeps its "/* " prefix and " */" suffix. Strip both + // styles uniformly below instead of relying on that asymmetry. 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 From 56b9960f38c11fa250e0e02f3011d0a4b4a405f4 Mon Sep 17 00:00:00 2001 From: sf-factory-bot Date: Thu, 8 Oct 2026 19:10:09 +0000 Subject: [PATCH 2/2] docs: clarify processCommentToString's comment-stripping comment (#100) --- cmd/protoc-gen-go-mcp/mcp.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/cmd/protoc-gen-go-mcp/mcp.go b/cmd/protoc-gen-go-mcp/mcp.go index cd9db6b..68a75b8 100644 --- a/cmd/protoc-gen-go-mcp/mcp.go +++ b/cmd/protoc-gen-go-mcp/mcp.go @@ -179,11 +179,15 @@ func generateHandler(g *protogen.GeneratedFile, method *protogen.Method, mcpServ // 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 { - // protogen.Comments strings for a line comment ("// foo") have the - // "// " prefix already stripped from the first line (but not - // continuation lines) and a trailing newline; a block comment - // ("/* foo */") keeps its "/* " prefix and " */" suffix. Strip both - // styles uniformly below instead of relying on that asymmetry. + // 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) commentText = strings.TrimPrefix(commentText, "// ")