From 979ea6bb3c12a771910767dc7a5b97d8e7caf917 Mon Sep 17 00:00:00 2001 From: Hermes Bot Date: Tue, 16 Jun 2026 14:46:34 +0000 Subject: [PATCH 1/5] Inline text file reads in GitHub MCP --- .../__toolsnaps__/get_file_contents.snap | 12 + pkg/github/repositories.go | 324 +++++++++++++++--- pkg/github/repositories_test.go | 122 +++++-- 3 files changed, 375 insertions(+), 83 deletions(-) diff --git a/pkg/github/__toolsnaps__/get_file_contents.snap b/pkg/github/__toolsnaps__/get_file_contents.snap index 94b7aeedac..8a1eca96c7 100644 --- a/pkg/github/__toolsnaps__/get_file_contents.snap +++ b/pkg/github/__toolsnaps__/get_file_contents.snap @@ -6,6 +6,14 @@ "description": "Get the contents of a file or directory from a GitHub repository", "inputSchema": { "properties": { + "end_line": { + "description": "Optional end line for text file reads", + "type": "integer" + }, + "max_bytes": { + "description": "Optional maximum UTF-8 bytes to return for text file reads", + "type": "integer" + }, "owner": { "description": "Repository owner (username or organization)", "type": "string" @@ -26,6 +34,10 @@ "sha": { "description": "Accepts optional commit SHA. If specified, it will be used instead of ref", "type": "string" + }, + "start_line": { + "description": "Optional start line for text file reads", + "type": "integer" } }, "required": [ diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 21cbf7e643..2cc5e03a9e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -6,7 +6,9 @@ import ( "encoding/json" "fmt" "io" + "mime" "net/http" + "path/filepath" "slices" "strconv" "strings" @@ -693,6 +695,194 @@ func FetchRepoIsPrivate(ctx context.Context, client *github.Client, owner, repo return r.GetPrivate(), nil } +func isSecretLikeRepoPath(path string) bool { + lower := strings.ToLower(strings.TrimSpace(path)) + base := filepath.Base(lower) + switch { + case lower == ".env", + strings.HasPrefix(base, ".env."), + base == "id_rsa", + base == "id_ed25519", + strings.HasSuffix(base, ".pem"), + strings.HasSuffix(base, ".key"), + base == "credentials.json", + strings.Contains(base, "service-account") && strings.HasSuffix(base, ".json"), + strings.HasSuffix(base, ".sql"), + strings.HasSuffix(base, ".dump"), + strings.HasSuffix(base, ".bak"), + strings.HasSuffix(base, ".sqlite"), + strings.HasSuffix(base, ".sqlite3"): + return true + default: + return false + } +} + +func detectFileMimeType(path, fallback string) string { + if strings.EqualFold(filepath.Base(path), "Dockerfile") { + return "text/x-dockerfile" + } + if ext := filepath.Ext(path); ext != "" { + if byExt := mime.TypeByExtension(ext); byExt != "" { + return byExt + } + } + if fallback != "" { + return fallback + } + return "text/plain; charset=utf-8" +} + +func detectFenceLanguage(path string) string { + if strings.EqualFold(filepath.Base(path), "Dockerfile") { + return "dockerfile" + } + switch strings.ToLower(filepath.Ext(path)) { + case ".py": + return "py" + case ".ts": + return "ts" + case ".tsx": + return "tsx" + case ".js": + return "js" + case ".jsx": + return "jsx" + case ".json": + return "json" + case ".md": + return "md" + case ".yml", ".yaml": + return "yaml" + case ".toml": + return "toml" + case ".css": + return "css" + case ".html": + return "html" + case ".sh": + return "sh" + default: + return "text" + } +} + +func trimTextByBytes(content string, maxBytes int) (string, bool) { + if maxBytes <= 0 || len(content) <= maxBytes { + return content, false + } + return content[:maxBytes], true +} + +func optionalIntArg(args map[string]any, key string) int { + value, ok := args[key] + if !ok || value == nil { + return 0 + } + switch v := value.(type) { + case int: + return v + case int32: + return int(v) + case int64: + return int(v) + case float64: + return int(v) + default: + return 0 + } +} + +func sliceTextByLines(content string, startLine, endLine int) (string, int, int, bool) { + lines := strings.Split(content, "\n") + if startLine < 1 { + startLine = 1 + } + if endLine < startLine || endLine == 0 { + endLine = len(lines) + } + if startLine > len(lines) { + return "", startLine, startLine, true + } + if endLine > len(lines) { + endLine = len(lines) + } + return strings.Join(lines[startLine-1:endLine], "\n"), startLine, endLine, endLine < len(lines) +} + +func newInlineFileToolResult(owner, repo, filePath, ref, sha, mimeType, content string, startLine, endLine int, truncated bool) *mcp.CallToolResult { + structured := map[string]any{ + "owner": owner, + "repo": repo, + "path": filePath, + "ref": ref, + "sha": sha, + "mime_type": mimeType, + "encoding": "utf-8", + "truncated": truncated, + "start_line": startLine, + "end_line": endLine, + "content": content, + } + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{ + Text: fmt.Sprintf("File: %s\nRepo: %s/%s\nRef: %s\nSHA: %s\n\n```%s\n%s\n```", + filePath, + owner, + repo, + ref, + sha, + detectFenceLanguage(filePath), + content, + ), + }, + }, + StructuredContent: structured, + } +} + +func newBlockedFileToolResult(path, reason string) *mcp.CallToolResult { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf(`{"blocked":true,"reason":"%s","path":"%s"}`, reason, path)}, + }, + StructuredContent: map[string]any{ + "blocked": true, + "reason": reason, + "path": path, + }, + IsError: true, + } +} + +func newBinaryMetadataToolResult(owner, repo, filePath, ref, sha, mimeType string, size int) *mcp.CallToolResult { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{ + Text: fmt.Sprintf("Binary file not inlined.\nPath: %s\nRepo: %s/%s\nRef: %s\nSHA: %s\nMIME: %s\nSize: %d", + filePath, owner, repo, ref, sha, mimeType, size), + }, + }, + StructuredContent: map[string]any{ + "owner": owner, + "repo": repo, + "path": filePath, + "ref": ref, + "sha": sha, + "mime_type": mimeType, + "encoding": "binary", + "size": size, + "truncated": true, + "start_line": 0, + "end_line": 0, + "content": "", + "blocked": true, + "reason": "binary_file", + }, + } +} + // GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository. func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( @@ -728,6 +918,18 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool Type: "string", Description: "Accepts optional commit SHA. If specified, it will be used instead of ref", }, + "start_line": { + Type: "integer", + Description: "Optional start line for text file reads", + }, + "end_line": { + Type: "integer", + Description: "Optional end line for text file reads", + }, + "max_bytes": { + Type: "integer", + Description: "Optional maximum UTF-8 bytes to return for text file reads", + }, }, Required: []string{"owner", "repo"}, }, @@ -748,6 +950,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool return utils.NewToolResultError(err.Error()), nil, nil } path = strings.TrimPrefix(path, "/") + if isSecretLikeRepoPath(path) { + return newBlockedFileToolResult(path, "secret_like_file"), nil, nil + } ref, err := OptionalParam[string](args, "ref") if err != nil { @@ -778,6 +983,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to resolve git reference: %s", err)), nil, nil } + startLine := optionalIntArg(args, "start_line") + endLine := optionalIntArg(args, "end_line") + maxBytes := optionalIntArg(args, "max_bytes") if rawOpts.SHA != "" { ref = rawOpts.SHA @@ -802,85 +1010,87 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if fileContent != nil && fileContent.SHA != nil { fileSHA = *fileContent.SHA fileSize := fileContent.GetSize() - // Build resource URI for the file using URI templates - pathParts := strings.Split(path, "/") - resourceURI, err := expandRepoResourceURI(owner, repo, sha, ref, pathParts) - if err != nil { - return utils.NewToolResultError("failed to build resource URI"), nil, nil - } // main branch ref passed in ref parameter but it doesn't exist - default branch was used var successNote string if fallbackUsed { successNote = fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.", originalRef, rawOpts.Ref) } - - // Empty files (0 bytes) have no content to decode; return - // them directly as empty text to avoid errors from - // GetContent when the API returns null content with a - // base64 encoding field, and to avoid DetectContentType - // misclassifying them as binary. if fileSize == 0 { - result := &mcp.ResourceContents{ - URI: resourceURI, - Text: "", - MIMEType: "text/plain", + result := newInlineFileToolResult(owner, repo, path, rawOpts.Ref, fileSHA, "text/plain", "", 1, 1, false) + if successNote != "" { + text := result.Content[0].(*mcp.TextContent) + text.Text = strings.Replace(text.Text, "\n\n```", fmt.Sprintf("%s\n\n```", successNote), 1) } - return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil + return attachIFC(result), nil, nil } - // For files >= 1MB, return a ResourceLink instead of content + var contentBytes []byte + var contentType string const maxContentSize = 1024 * 1024 // 1MB - if fileSize >= maxContentSize { - size := int64(fileSize) - resourceLink := &mcp.ResourceLink{ - URI: resourceURI, - Name: fileContent.GetName(), - Title: fmt.Sprintf("File: %s", path), - Size: &size, + useRaw := fileSize >= maxContentSize + if !useRaw { + content, decodeErr := fileContent.GetContent() + if decodeErr != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to decode file content: %s", decodeErr)), nil, nil } - return attachIFC(utils.NewToolResultResourceLink( - fmt.Sprintf("File %s is too large to display (%d bytes). Use the download URL to fetch the content: %s (SHA: %s)%s", - path, fileSize, fileContent.GetDownloadURL(), fileSHA, successNote), - resourceLink)), nil, nil - } - - // For files < 1MB, get content directly from Contents API - content, err := fileContent.GetContent() - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to decode file content: %s", err)), nil, nil + contentBytes = []byte(content) + contentType = detectFileMimeType(path, http.DetectContentType(contentBytes)) + } else { + rawClient, rawErr := deps.GetRawClient(ctx) + if rawErr != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub raw content client: %s", rawErr)), nil, nil + } + rawResp, rawErr := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts) + if rawErr != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to fetch raw file content: %s", rawErr)), nil, nil + } + defer func() { _ = rawResp.Body.Close() }() + if rawResp.StatusCode != http.StatusOK { + body, readErr := io.ReadAll(rawResp.Body) + if readErr == nil { + _ = body + } + return attachIFC(newBinaryMetadataToolResult(owner, repo, path, rawOpts.Ref, fileSHA, detectFileMimeType(path, "application/octet-stream"), fileSize)), nil, nil + } + contentBytes, err = io.ReadAll(rawResp.Body) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to read file content: %s", err)), nil, nil + } + contentType = detectFileMimeType(path, rawResp.Header.Get("Content-Type")) } - - // Detect content type from the actual content bytes, - // mirroring the original approach of using the Content-Type header - // from the raw API response. - contentBytes := []byte(content) - contentType := http.DetectContentType(contentBytes) - - // Determine if content is text or binary based on detected content type isTextContent := strings.HasPrefix(contentType, "text/") || contentType == "application/json" || contentType == "application/xml" || strings.HasSuffix(contentType, "+json") || strings.HasSuffix(contentType, "+xml") - - if isTextContent { - result := &mcp.ResourceContents{ - URI: resourceURI, - Text: content, - MIMEType: contentType, + if !isTextContent { + msg := fmt.Sprintf("Binary file not inlined (SHA: %s)%s", fileSHA, successNote) + return attachIFC(newBinaryMetadataToolResult(owner, repo, path, rawOpts.Ref, fileSHA, contentType, fileSize)), map[string]any{"message": msg}, nil + } + + content := string(contentBytes) + if startLine > 0 || endLine > 0 { + var truncated bool + content, startLine, endLine, truncated = sliceTextByLines(content, startLine, endLine) + result := newInlineFileToolResult(owner, repo, path, rawOpts.Ref, fileSHA, contentType, content, startLine, endLine, truncated) + if successNote != "" { + text := result.Content[0].(*mcp.TextContent) + text.Text = strings.Replace(text.Text, "\n\n```", fmt.Sprintf("%s\n\n```", successNote), 1) } - return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil + return attachIFC(result), nil, nil } - // Binary content - encode as base64 blob - blobContent := base64.StdEncoding.EncodeToString(contentBytes) - result := &mcp.ResourceContents{ - URI: resourceURI, - Blob: []byte(blobContent), - MIMEType: contentType, + if maxBytes == 0 { + maxBytes = 128 * 1024 + } + content, truncated := trimTextByBytes(content, maxBytes) + result := newInlineFileToolResult(owner, repo, path, rawOpts.Ref, fileSHA, contentType, content, 1, strings.Count(content, "\n")+1, truncated) + if successNote != "" { + text := result.Content[0].(*mcp.TextContent) + text.Text = strings.Replace(text.Text, "\n\n```", fmt.Sprintf("%s\n\n```", successNote), 1) } - return attachIFC(utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)%s", fileSHA, successNote), result)), nil, nil + return attachIFC(result), nil, nil } else if dirContent != nil { // file content or file SHA is nil which means it's a directory r, err := json.Marshal(dirContent) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index e5531cc55b..052778ffb0 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -39,6 +39,9 @@ func Test_GetFileContents(t *testing.T) { assert.Contains(t, schema.Properties, "path") assert.Contains(t, schema.Properties, "ref") assert.Contains(t, schema.Properties, "sha") + assert.Contains(t, schema.Properties, "start_line") + assert.Contains(t, schema.Properties, "end_line") + assert.Contains(t, schema.Properties, "max_bytes") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo"}) // Mock response for raw content @@ -105,7 +108,7 @@ func Test_GetFileContents(t *testing.T) { expectedResult: mcp.ResourceContents{ URI: "repo://owner/repo/refs/heads/main/contents/README.md", Text: "# Test Repository\n\nThis is a test repository.", - MIMEType: "text/plain; charset=utf-8", + MIMEType: "text/markdown; charset=utf-8", }, }, { @@ -232,7 +235,7 @@ func Test_GetFileContents(t *testing.T) { expectedResult: mcp.ResourceContents{ URI: "repo://owner/repo/refs/heads/main/contents/README.md", Text: "# Test Repository\n\nThis is a test repository.", - MIMEType: "text/plain; charset=utf-8", + MIMEType: "text/markdown; charset=utf-8", }, }, { @@ -316,7 +319,7 @@ func Test_GetFileContents(t *testing.T) { expectedResult: mcp.ResourceContents{ URI: "repo://owner/repo/sha/abc123def456abc123def456abc123def456abc1/contents/README.md", Text: "# Test Repository\n\nThis is a test repository.", - MIMEType: "text/plain; charset=utf-8", + MIMEType: "text/markdown; charset=utf-8", }, expectedMsg: " Note: the provided ref 'main' does not exist, default branch 'refs/heads/develop' was used instead.", }, @@ -339,6 +342,11 @@ func Test_GetFileContents(t *testing.T) { contentBytes, _ := json.Marshal(fileContent) _, _ = w.Write(contentBytes) }, + GetRawReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte{0x01, 0x02, 0x03}) + }, }), requestArgs: map[string]any{ "owner": "owner", @@ -347,10 +355,9 @@ func Test_GetFileContents(t *testing.T) { "ref": "refs/heads/main", }, expectError: false, - expectedResult: &mcp.ResourceLink{ - URI: "repo://owner/repo/refs/heads/main/contents/large-file.bin", - Name: "large-file.bin", - Title: "File: large-file.bin", + expectedResult: mcp.ResourceContents{ + MIMEType: "application/octet-stream", + Blob: []byte("non-empty"), }, }, { @@ -385,7 +392,6 @@ func Test_GetFileContents(t *testing.T) { Text: "", MIMEType: "text/plain", }, - expectedMsg: "successfully downloaded empty file", }, { name: "content fetch fails", @@ -441,16 +447,23 @@ func Test_GetFileContents(t *testing.T) { // Use the correct result helper based on the expected type switch expected := tc.expectedResult.(type) { case mcp.ResourceContents: - // Handle both text and blob resources - resource := getResourceResult(t, result) - assert.Equal(t, expected, *resource) - - // If expectedMsg is set, verify the message text - if tc.expectedMsg != "" { - require.Len(t, result.Content, 2) - textContent, ok := result.Content[0].(*mcp.TextContent) - require.True(t, ok, "expected Content[0] to be TextContent") - assert.Contains(t, textContent.Text, tc.expectedMsg) + if expected.Blob == nil { + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, expected.Text) + sc, ok := result.StructuredContent.(map[string]any) + require.True(t, ok, "expected structured content for text file") + assert.Equal(t, expected.Text, sc["content"]) + assert.Equal(t, expected.MIMEType, sc["mime_type"]) + if tc.expectedMsg != "" { + assert.Contains(t, textContent.Text, tc.expectedMsg) + } + } else { + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "Binary file not inlined") + sc, ok := result.StructuredContent.(map[string]any) + require.True(t, ok, "expected structured content for binary file") + assert.Equal(t, expected.MIMEType, sc["mime_type"]) + assert.Equal(t, true, sc["blocked"]) } case []*github.RepositoryContent: // Directory content fetch returns a text result (JSON array) @@ -464,14 +477,6 @@ func Test_GetFileContents(t *testing.T) { assert.Equal(t, *expected[i].Path, *content.Path) assert.Equal(t, *expected[i].Type, *content.Type) } - case *mcp.ResourceLink: - // Large file returns a ResourceLink - require.Len(t, result.Content, 2) - resourceLink, ok := result.Content[1].(*mcp.ResourceLink) - require.True(t, ok, "expected Content[1] to be ResourceLink") - assert.Equal(t, expected.URI, resourceLink.URI) - assert.Equal(t, expected.Name, resourceLink.Name) - assert.Equal(t, expected.Title, resourceLink.Title) case mcp.TextContent: textContent := getErrorResult(t, result) require.Equal(t, textContent, expected) @@ -622,6 +627,71 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { }) } +func Test_GetFileContents_BlocksSecretLikePaths(t *testing.T) { + serverTool := GetFileContents(translations.NullTranslationHelper) + client := mustNewGHClient(t, NewMockedHTTPClient()) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "path": ".env.production", + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + textContent := getErrorResult(t, result) + assert.Contains(t, textContent.Text, `"blocked":true`) + assert.Contains(t, textContent.Text, `"reason":"secret_like_file"`) +} + +func Test_GetFileContents_LineRange(t *testing.T) { + serverTool := GetFileContents(translations.NullTranslationHelper) + content := []byte("one\ntwo\nthree\nfour\n") + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + encodedContent := base64.StdEncoding.EncodeToString(content) + fileContent := &github.RepositoryContent{ + Name: github.Ptr("README.md"), + Path: github.Ptr("README.md"), + SHA: github.Ptr("abc123"), + Type: github.Ptr("file"), + Content: github.Ptr(encodedContent), + Size: github.Ptr(len(content)), + Encoding: github.Ptr("base64"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }, + })) + mockRawClient, err := raw.NewClient(client, &url.URL{Scheme: "https", Host: "raw.example.com", Path: "/"}) + require.NoError(t, err) + deps := BaseDeps{Client: client, RawClient: mockRawClient} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "README.md", + "ref": "refs/heads/main", + "start_line": 2, + "end_line": 3, + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "two\nthree") + sc, ok := result.StructuredContent.(map[string]any) + require.True(t, ok) + assert.Equal(t, 2, sc["start_line"]) + assert.Equal(t, 3, sc["end_line"]) + assert.Equal(t, "two\nthree", sc["content"]) +} + // Test_GetCommit_IFC_FeatureFlag verifies that the IFC security label is only // attached to get_commit results when the ifc_labels feature flag is enabled, // and that the label content matches the commit-contents rule (untrusted on From 7b3016bc82e97fc69763b881dad9b5929a0b6bef Mon Sep 17 00:00:00 2001 From: Hermes Bot Date: Wed, 17 Jun 2026 04:13:58 +0000 Subject: [PATCH 2/5] Add shared-path GitHub file updates --- ...reate_or_update_file_from_shared_path.snap | 48 +++ pkg/github/repositories.go | 353 +++++++++++++----- pkg/github/repositories_test.go | 159 ++++++++ pkg/github/tools.go | 1 + 4 files changed, 470 insertions(+), 91 deletions(-) create mode 100644 pkg/github/__toolsnaps__/create_or_update_file_from_shared_path.snap diff --git a/pkg/github/__toolsnaps__/create_or_update_file_from_shared_path.snap b/pkg/github/__toolsnaps__/create_or_update_file_from_shared_path.snap new file mode 100644 index 0000000000..e944c686aa --- /dev/null +++ b/pkg/github/__toolsnaps__/create_or_update_file_from_shared_path.snap @@ -0,0 +1,48 @@ +{ + "annotations": { + "title": "Create or update file from shared path" + }, + "description": "Create or update a single file in a GitHub repository using a UTF-8 text file that already exists on the MCP host under the shared directory.\n\nUse this when the file content is already available on the server and is too large or too awkward to send inline through the MCP tool call.\n\nOnly files inside the configured shared directory are allowed. SHA MUST be provided for existing file updates.", + "inputSchema": { + "properties": { + "branch": { + "description": "Branch to create/update the file in", + "type": "string" + }, + "message": { + "description": "Commit message", + "type": "string" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "path": { + "description": "Repository path where to create/update the file", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "sha": { + "description": "The blob SHA of the file being replaced. Required if the file already exists.", + "type": "string" + }, + "shared_path": { + "description": "Relative path under the shared directory mounted into the MCP container", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "path", + "shared_path", + "message", + "branch" + ], + "type": "object" + }, + "name": "create_or_update_file_from_shared_path" +} \ No newline at end of file diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 2cc5e03a9e..5bc93ca028 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -8,10 +8,12 @@ import ( "io" "mime" "net/http" + "os" "path/filepath" "slices" "strconv" "strings" + "unicode/utf8" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/ifc" @@ -457,119 +459,288 @@ SHA MUST be provided for existing file updates. if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + sha, err := OptionalParam[string](args, "sha") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + return createOrUpdateFileResult(ctx, deps, client, owner, repo, path, content, message, branch, sha) + }, + ) +} - // json.Marshal encodes byte arrays with base64, which is required for the API. - contentBytes := []byte(content) +func githubSharedRoot() string { + if root := strings.TrimSpace(os.Getenv("GITHUB_MCP_SHARED_ROOT")); root != "" { + return root + } + return "/shared" +} - // Create the file options - opts := &github.RepositoryContentFileOptions{ - Message: github.Ptr(message), - Content: contentBytes, - Branch: github.Ptr(branch), +func resolveSharedPath(sharedPath string) (string, error) { + if strings.TrimSpace(sharedPath) == "" { + return "", fmt.Errorf("shared_path is required") + } + cleaned := filepath.Clean(strings.TrimPrefix(sharedPath, "/")) + if cleaned == "." || cleaned == "" { + return "", fmt.Errorf("shared_path must point to a file inside the shared directory") + } + if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(os.PathSeparator)) { + return "", fmt.Errorf("shared_path must stay within the shared directory") + } + joined := filepath.Join(githubSharedRoot(), cleaned) + rel, err := filepath.Rel(githubSharedRoot(), joined) + if err != nil { + return "", fmt.Errorf("failed to resolve shared_path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", fmt.Errorf("shared_path must stay within the shared directory") + } + return joined, nil +} + +func loadSharedTextFile(sharedPath string, maxBytes int) (string, int, error) { + resolved, err := resolveSharedPath(sharedPath) + if err != nil { + return "", 0, err + } + info, err := os.Stat(resolved) + if err != nil { + return "", 0, fmt.Errorf("failed to stat shared file: %w", err) + } + if info.IsDir() { + return "", 0, fmt.Errorf("shared_path points to a directory, not a file") + } + if maxBytes > 0 && info.Size() > int64(maxBytes) { + return "", 0, fmt.Errorf("shared file is too large: %d bytes exceeds limit of %d bytes", info.Size(), maxBytes) + } + contentBytes, err := os.ReadFile(resolved) + if err != nil { + return "", 0, fmt.Errorf("failed to read shared file: %w", err) + } + if !utf8.Valid(contentBytes) { + return "", 0, fmt.Errorf("shared file is not valid UTF-8 text") + } + return string(contentBytes), len(contentBytes), nil +} + +func firstTextResult(result *mcp.CallToolResult) *mcp.TextContent { + if result == nil { + return nil + } + for _, content := range result.Content { + if text, ok := content.(*mcp.TextContent); ok { + return text + } + } + return nil +} + +func createOrUpdateFileResult(ctx context.Context, deps ToolDependencies, client *github.Client, owner, repo, path, content, message, branch, sha string) (*mcp.CallToolResult, any, error) { + // json.Marshal encodes byte arrays with base64, which is required for the API. + contentBytes := []byte(content) + + // Create the file options + opts := &github.RepositoryContentFileOptions{ + Message: github.Ptr(message), + Content: contentBytes, + Branch: github.Ptr(branch), + } + if sha != "" { + opts.SHA = github.Ptr(sha) + } + + path = strings.TrimPrefix(path, "/") + getOpts := &github.RepositoryContentGetOptions{Ref: branch} + + if sha != "" { + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + _ = respCheck.Body.Close() + } + switch { + case getErr != nil: + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to verify file SHA", + respCheck, + getErr, + ), nil, nil + } + case dirContent != nil: + return utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path)), nil, nil + case existingFile != nil: + currentSHA := existingFile.GetSHA() + if currentSHA != sha { + return utils.NewToolResultError(fmt.Sprintf( + "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ + "Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", + sha, currentSHA, branch, path)), nil, nil } + } + } else { + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + _ = respCheck.Body.Close() + } + switch { + case getErr != nil: + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to check if file exists", + respCheck, + getErr, + ), nil, nil + } + case dirContent != nil: + return utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path)), nil, nil + case existingFile != nil: + return utils.NewToolResultError(fmt.Sprintf( + "File already exists at %s. You must provide the current file's SHA when updating. "+ + "Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.", + path, branch, path)), nil, nil + } + } - // If SHA is provided, set it (for updates) + fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to create/update file", + resp, + err, + ), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != 200 && resp.StatusCode != 201 { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create/update file", resp, body), nil, nil + } + + minimalResponse := convertToMinimalFileContentResponse(fileContent) + return MarshalledTextResult(minimalResponse), nil, nil +} + +func CreateOrUpdateFileFromSharedPath(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataRepos, + mcp.Tool{ + Name: "create_or_update_file_from_shared_path", + Description: t("TOOL_CREATE_OR_UPDATE_FILE_FROM_SHARED_PATH_DESCRIPTION", `Create or update a single file in a GitHub repository using a UTF-8 text file that already exists on the MCP host under the shared directory. + +Use this when the file content is already available on the server and is too large or too awkward to send inline through the MCP tool call. + +Only files inside the configured shared directory are allowed. SHA MUST be provided for existing file updates.`), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_CREATE_OR_UPDATE_FILE_FROM_SHARED_PATH_USER_TITLE", "Create or update file from shared path"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "path": { + Type: "string", + Description: "Repository path where to create/update the file", + }, + "shared_path": { + Type: "string", + Description: "Relative path under the shared directory mounted into the MCP container", + }, + "message": { + Type: "string", + Description: "Commit message", + }, + "branch": { + Type: "string", + Description: "Branch to create/update the file in", + }, + "sha": { + Type: "string", + Description: "The blob SHA of the file being replaced. Required if the file already exists.", + }, + }, + Required: []string{"owner", "repo", "path", "shared_path", "message", "branch"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + path, err := RequiredParam[string](args, "path") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + sharedPath, err := RequiredParam[string](args, "shared_path") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + message, err := RequiredParam[string](args, "message") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + branch, err := RequiredParam[string](args, "branch") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } sha, err := OptionalParam[string](args, "sha") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - if sha != "" { - opts.SHA = github.Ptr(sha) + + content, sizeBytes, err := loadSharedTextFile(sharedPath, 512*1024) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } - // Create or update the file client, err := deps.GetClient(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - path = strings.TrimPrefix(path, "/") - - // SHA validation using Contents API to fetch current file metadata (blob SHA) - getOpts := &github.RepositoryContentGetOptions{Ref: branch} - - if sha != "" { - // User provided SHA - validate it's still current - existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) - if respCheck != nil { - _ = respCheck.Body.Close() - } - switch { - case getErr != nil: - // 404 means file doesn't exist - proceed (new file creation) - // Any other error (403, 500, network) should be surfaced - if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to verify file SHA", - respCheck, - getErr, - ), nil, nil - } - case dirContent != nil: - return utils.NewToolResultError(fmt.Sprintf( - "Path %s is a directory, not a file. This tool only works with files.", - path)), nil, nil - case existingFile != nil: - currentSHA := existingFile.GetSHA() - if currentSHA != sha { - return utils.NewToolResultError(fmt.Sprintf( - "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ - "Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", - sha, currentSHA, branch, path)), nil, nil - } - } - } else { - // No SHA provided - check if file already exists - existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) - if respCheck != nil { - _ = respCheck.Body.Close() - } - switch { - case getErr != nil: - // 404 means file doesn't exist - proceed with creation - // Any other error (403, 500, network) should be surfaced - if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to check if file exists", - respCheck, - getErr, - ), nil, nil - } - case dirContent != nil: - return utils.NewToolResultError(fmt.Sprintf( - "Path %s is a directory, not a file. This tool only works with files.", - path)), nil, nil - case existingFile != nil: - // File exists but no SHA was provided - reject to prevent blind overwrites - return utils.NewToolResultError(fmt.Sprintf( - "File already exists at %s. You must provide the current file's SHA when updating. "+ - "Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.", - path, branch, path)), nil, nil - } - // If file not found, no previous SHA needed (new file creation) + result, _, err := createOrUpdateFileResult(ctx, deps, client, owner, repo, path, content, message, branch, sha) + if err != nil || result == nil || result.IsError { + return result, nil, err } - fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to create/update file", - resp, - err, - ), nil, nil + textResult := firstTextResult(result) + if textResult == nil { + return result, nil, nil } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != 200 && resp.StatusCode != 201 { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) + var structured map[string]any + if unmarshalErr := json.Unmarshal([]byte(textResult.Text), &structured); unmarshalErr == nil { + structured["source"] = map[string]any{ + "type": "shared_path", + "shared_path": sharedPath, + "size_bytes": sizeBytes, } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create/update file", resp, body), nil, nil + result.StructuredContent = structured } - minimalResponse := convertToMinimalFileContentResponse(fileContent) - - return MarshalledTextResult(minimalResponse), nil, nil + return result, nil, nil }, ) } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 052778ffb0..10e781ee9e 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -6,6 +6,8 @@ import ( "encoding/json" "net/http" "net/url" + "os" + "path/filepath" "strings" "testing" "time" @@ -2002,6 +2004,163 @@ func Test_CreateOrUpdateFile(t *testing.T) { } } +func Test_CreateOrUpdateFileFromSharedPath(t *testing.T) { + serverTool := CreateOrUpdateFileFromSharedPath(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + + assert.Equal(t, "create_or_update_file_from_shared_path", tool.Name) + assert.Contains(t, schema.Properties, "shared_path") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "path", "shared_path", "message", "branch"}) + + tempRoot := t.TempDir() + t.Setenv("GITHUB_MCP_SHARED_ROOT", tempRoot) + + sharedRelPath := "patches/worker.js" + sharedAbsPath := filepath.Join(tempRoot, sharedRelPath) + require.NoError(t, os.MkdirAll(filepath.Dir(sharedAbsPath), 0o755)) + require.NoError(t, os.WriteFile(sharedAbsPath, []byte("console.log('patched worker');\n"), 0o644)) + + mockFileResponse := &github.RepositoryContentResponse{ + Content: &github.RepositoryContent{ + Name: github.Ptr("worker.js"), + Path: github.Ptr("src/worker.js"), + SHA: github.Ptr("worker-sha"), + Size: github.Ptr(30), + HTMLURL: github.Ptr("https://github.com/owner/repo/blob/feature/src/worker.js"), + }, + Commit: github.Commit{ + SHA: github.Ptr("commit-sha"), + Message: github.Ptr("Patch worker"), + HTMLURL: github.Ptr("https://github.com/owner/repo/commit/commit-sha"), + Author: &github.CommitAuthor{ + Name: github.Ptr("Test User"), + Email: github.Ptr("test@example.com"), + Date: &github.Timestamp{Time: time.Now()}, + }, + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedErrMsg string + validate func(t *testing.T, result *mcp.CallToolResult) + }{ + { + name: "successful shared path update", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/contents/src/worker.js": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("old-sha"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("old-sha"), + Type: github.Ptr("file"), + }), + PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ + "message": "Patch worker", + "content": base64.StdEncoding.EncodeToString([]byte("console.log('patched worker');\n")), + "branch": "feature/test", + "sha": "old-sha", + }).andThen( + mockResponse(t, http.StatusOK, mockFileResponse), + ), + "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ + "message": "Patch worker", + "content": base64.StdEncoding.EncodeToString([]byte("console.log('patched worker');\n")), + "branch": "feature/test", + "sha": "old-sha", + }).andThen( + mockResponse(t, http.StatusOK, mockFileResponse), + ), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "src/worker.js", + "shared_path": sharedRelPath, + "message": "Patch worker", + "branch": "feature/test", + "sha": "old-sha", + }, + validate: func(t *testing.T, result *mcp.CallToolResult) { + require.NotNil(t, result.StructuredContent) + structured, ok := result.StructuredContent.(map[string]any) + require.True(t, ok) + source, ok := structured["source"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "shared_path", source["type"]) + assert.Equal(t, sharedRelPath, source["shared_path"]) + assert.EqualValues(t, len([]byte("console.log('patched worker');\n")), source["size_bytes"]) + }, + }, + { + name: "reject path traversal", + mockedClient: NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "src/worker.js", + "shared_path": "../secrets.txt", + "message": "Patch worker", + "branch": "feature/test", + }, + expectError: true, + expectedErrMsg: "shared_path must stay within the shared directory", + }, + { + name: "reject missing shared file", + mockedClient: NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "src/worker.js", + "shared_path": "missing/worker.js", + "message": "Patch worker", + "branch": "feature/test", + }, + expectError: true, + expectedErrMsg: "failed to stat shared file", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + if tc.expectError { + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + return + } + + require.False(t, result.IsError) + textContent := getTextResult(t, result) + var returnedContent MinimalFileContentResponse + err = json.Unmarshal([]byte(textContent.Text), &returnedContent) + require.NoError(t, err) + assert.Equal(t, "src/worker.js", returnedContent.Content.Path) + assert.Equal(t, "commit-sha", returnedContent.Commit.SHA) + if tc.validate != nil { + tc.validate(t, result) + } + }) + } +} + func Test_CreateRepository(t *testing.T) { // Verify tool definition once serverTool := CreateRepository(translations.NullTranslationHelper) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 906fa777d7..b3358d7c32 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -188,6 +188,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { GetLatestRelease(t), GetReleaseByTag(t), CreateOrUpdateFile(t), + CreateOrUpdateFileFromSharedPath(t), CreateRepository(t), ForkRepository(t), CreateBranch(t), From 69cd02cea17f1868a42d156d07a6ef7cbe460d47 Mon Sep 17 00:00:00 2001 From: Hermes Bot Date: Wed, 17 Jun 2026 05:10:07 +0000 Subject: [PATCH 3/5] Add shared-path multi-file GitHub writes --- .../push_files_from_shared_paths.snap | 57 ++++ pkg/github/repositories.go | 279 +++++++++++++++++- pkg/github/repositories_test.go | 172 ++++++++++- pkg/github/tools.go | 1 + 4 files changed, 503 insertions(+), 6 deletions(-) create mode 100644 pkg/github/__toolsnaps__/push_files_from_shared_paths.snap diff --git a/pkg/github/__toolsnaps__/push_files_from_shared_paths.snap b/pkg/github/__toolsnaps__/push_files_from_shared_paths.snap new file mode 100644 index 0000000000..5863cb2c05 --- /dev/null +++ b/pkg/github/__toolsnaps__/push_files_from_shared_paths.snap @@ -0,0 +1,57 @@ +{ + "annotations": { + "title": "Push files from shared paths" + }, + "description": "Push multiple UTF-8 text files from the shared directory to a GitHub repository in a single commit.", + "inputSchema": { + "properties": { + "branch": { + "description": "Branch to push to", + "type": "string" + }, + "files": { + "description": "Array of file objects to push, each object with path (repository path) and shared_path (relative path under the shared directory)", + "items": { + "additionalProperties": false, + "properties": { + "path": { + "description": "Repository path for the file", + "type": "string" + }, + "shared_path": { + "description": "Relative path under the shared directory mounted into the MCP container", + "type": "string" + } + }, + "required": [ + "path", + "shared_path" + ], + "type": "object" + }, + "type": "array" + }, + "message": { + "description": "Commit message", + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "branch", + "files", + "message" + ], + "type": "object" + }, + "name": "push_files_from_shared_paths" +} \ No newline at end of file diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 5bc93ca028..69afd50c34 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -479,26 +479,40 @@ func githubSharedRoot() string { return "/shared" } +const githubSharedMaxBytes = 2 * 1024 * 1024 + func resolveSharedPath(sharedPath string) (string, error) { if strings.TrimSpace(sharedPath) == "" { return "", fmt.Errorf("shared_path is required") } - cleaned := filepath.Clean(strings.TrimPrefix(sharedPath, "/")) + if filepath.IsAbs(sharedPath) { + return "", fmt.Errorf("shared_path must be relative to the shared directory") + } + root := filepath.Clean(githubSharedRoot()) + rootEval, err := filepath.EvalSymlinks(root) + if err != nil { + return "", fmt.Errorf("failed to resolve shared root: %w", err) + } + cleaned := filepath.Clean(sharedPath) if cleaned == "." || cleaned == "" { return "", fmt.Errorf("shared_path must point to a file inside the shared directory") } if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(os.PathSeparator)) { return "", fmt.Errorf("shared_path must stay within the shared directory") } - joined := filepath.Join(githubSharedRoot(), cleaned) - rel, err := filepath.Rel(githubSharedRoot(), joined) + joined := filepath.Join(rootEval, cleaned) + resolved, err := filepath.EvalSymlinks(joined) if err != nil { return "", fmt.Errorf("failed to resolve shared_path: %w", err) } + rel, err := filepath.Rel(rootEval, resolved) + if err != nil { + return "", fmt.Errorf("failed to verify shared_path: %w", err) + } if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { return "", fmt.Errorf("shared_path must stay within the shared directory") } - return joined, nil + return resolved, nil } func loadSharedTextFile(sharedPath string, maxBytes int) (string, int, error) { @@ -526,6 +540,165 @@ func loadSharedTextFile(sharedPath string, maxBytes int) (string, int, error) { return string(contentBytes), len(contentBytes), nil } +type sharedFileSpec struct { + RepoPath string + SharedPath string + Content string + SizeBytes int +} + +type SharedPushFilesResponse struct { + CommitSHA string `json:"commit_sha"` + Branch string `json:"branch"` + ChangedPaths []string `json:"changed_paths"` + FileCount int `json:"file_count"` +} + +func parseSharedFileSpecs(args map[string]any) ([]sharedFileSpec, error) { + filesObj, ok := args["files"].([]any) + if !ok { + return nil, fmt.Errorf("files parameter must be an array of objects with path and shared_path") + } + files := make([]sharedFileSpec, 0, len(filesObj)) + for _, file := range filesObj { + fileMap, ok := file.(map[string]any) + if !ok { + return nil, fmt.Errorf("each file must be an object with path and shared_path") + } + repoPath, ok := fileMap["path"].(string) + if !ok || strings.TrimSpace(repoPath) == "" { + return nil, fmt.Errorf("each file must have a path") + } + sharedPath, ok := fileMap["shared_path"].(string) + if !ok || strings.TrimSpace(sharedPath) == "" { + return nil, fmt.Errorf("each file must have a shared_path") + } + content, sizeBytes, err := loadSharedTextFile(sharedPath, githubSharedMaxBytes) + if err != nil { + return nil, err + } + files = append(files, sharedFileSpec{ + RepoPath: repoPath, + SharedPath: sharedPath, + Content: content, + SizeBytes: sizeBytes, + }) + } + return files, nil +} + +func prepareBranchBaseCommit(ctx context.Context, client *github.Client, owner, repo, branch string) (*github.Reference, *github.Commit, error) { + var repositoryIsEmpty bool + var branchNotFound bool + ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch) + if err != nil { + ghErr, isGhErr := err.(*github.ErrorResponse) + if isGhErr { + if ghErr.Response.StatusCode == http.StatusConflict && ghErr.Message == "Git Repository is empty." { + repositoryIsEmpty = true + } else if ghErr.Response.StatusCode == http.StatusNotFound { + branchNotFound = true + } + } + if !repositoryIsEmpty && !branchNotFound { + return nil, nil, fmt.Errorf("failed to get branch reference: %w", err) + } + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + + var baseCommit *github.Commit + if !repositoryIsEmpty { + if branchNotFound { + ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) + if err != nil { + return nil, nil, fmt.Errorf("failed to create branch from default: %w", err) + } + } + baseCommit, resp, err = client.Git.GetCommit(ctx, owner, repo, *ref.Object.SHA) + if err != nil { + return nil, nil, fmt.Errorf("failed to get base commit: %w", err) + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + } else { + var base *github.Commit + ref, base, err = initializeRepository(ctx, client, owner, repo) + if err != nil { + return nil, nil, fmt.Errorf("failed to initialize repository: %w", err) + } + defaultBranch := strings.TrimPrefix(*ref.Ref, "refs/heads/") + if branch != defaultBranch { + ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) + if err != nil { + return nil, nil, fmt.Errorf("failed to create branch from default: %w", err) + } + } + baseCommit = base + } + return ref, baseCommit, nil +} + +func pushTreeEntries(ctx context.Context, client *github.Client, owner, repo, branch, message string, entries []*github.TreeEntry, changedPaths []string) (*mcp.CallToolResult, any, error) { + ref, baseCommit, err := prepareBranchBaseCommit(ctx, client, owner, repo, branch) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, entries) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to create tree", + resp, + err, + ), nil, nil + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + + commit := github.Commit{ + Message: github.Ptr(message), + Tree: newTree, + Parents: []*github.Commit{{SHA: baseCommit.SHA}}, + } + newCommit, resp, err := client.Git.CreateCommit(ctx, owner, repo, commit, nil) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to create commit", + resp, + err, + ), nil, nil + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + + _, resp, err = client.Git.UpdateRef(ctx, owner, repo, *ref.Ref, github.UpdateRef{ + SHA: *newCommit.SHA, + Force: github.Ptr(false), + }) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to update reference", + resp, + err, + ), nil, nil + } + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + + return MarshalledTextResult(SharedPushFilesResponse{ + CommitSHA: newCommit.GetSHA(), + Branch: branch, + ChangedPaths: changedPaths, + FileCount: len(changedPaths), + }), nil, nil +} + func firstTextResult(result *mcp.CallToolResult) *mcp.TextContent { if result == nil { return nil @@ -710,7 +883,7 @@ Only files inside the configured shared directory are allowed. SHA MUST be provi return utils.NewToolResultError(err.Error()), nil, nil } - content, sizeBytes, err := loadSharedTextFile(sharedPath, 512*1024) + content, sizeBytes, err := loadSharedTextFile(sharedPath, githubSharedMaxBytes) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -745,6 +918,102 @@ Only files inside the configured shared directory are allowed. SHA MUST be provi ) } +func PushFilesFromSharedPaths(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataRepos, + mcp.Tool{ + Name: "push_files_from_shared_paths", + Description: t("TOOL_PUSH_FILES_FROM_SHARED_PATHS_DESCRIPTION", "Push multiple UTF-8 text files from the shared directory to a GitHub repository in a single commit."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_PUSH_FILES_FROM_SHARED_PATHS_USER_TITLE", "Push files from shared paths"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "branch": { + Type: "string", + Description: "Branch to push to", + }, + "message": { + Type: "string", + Description: "Commit message", + }, + "files": { + Type: "array", + Description: "Array of file objects to push, each object with path (repository path) and shared_path (relative path under the shared directory)", + Items: &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: map[string]*jsonschema.Schema{ + "path": { + Type: "string", + Description: "Repository path for the file", + }, + "shared_path": { + Type: "string", + Description: "Relative path under the shared directory mounted into the MCP container", + }, + }, + Required: []string{"path", "shared_path"}, + }, + }, + }, + Required: []string{"owner", "repo", "branch", "files", "message"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + branch, err := RequiredParam[string](args, "branch") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + message, err := RequiredParam[string](args, "message") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + files, err := parseSharedFileSpecs(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + entries := make([]*github.TreeEntry, 0, len(files)) + changedPaths := make([]string, 0, len(files)) + for _, file := range files { + entries = append(entries, &github.TreeEntry{ + Path: github.Ptr(file.RepoPath), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + Content: github.Ptr(file.Content), + }) + changedPaths = append(changedPaths, file.RepoPath) + } + + return pushTreeEntries(ctx, client, owner, repo, branch, message, entries, changedPaths) + }, + ) +} + // CreateRepository creates a tool to create a new GitHub repository. func CreateRepository(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 10e781ee9e..68af8e42f5 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2100,6 +2100,20 @@ func Test_CreateOrUpdateFileFromSharedPath(t *testing.T) { assert.EqualValues(t, len([]byte("console.log('patched worker');\n")), source["size_bytes"]) }, }, + { + name: "reject absolute path", + mockedClient: NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "src/worker.js", + "shared_path": "/etc/passwd", + "message": "Patch worker", + "branch": "feature/test", + }, + expectError: true, + expectedErrMsg: "shared_path must be relative to the shared directory", + }, { name: "reject path traversal", mockedClient: NewMockedHTTPClient(), @@ -2126,7 +2140,27 @@ func Test_CreateOrUpdateFileFromSharedPath(t *testing.T) { "branch": "feature/test", }, expectError: true, - expectedErrMsg: "failed to stat shared file", + expectedErrMsg: "failed to resolve shared_path", + }, + { + name: "reject symlink escape", + mockedClient: NewMockedHTTPClient(), + requestArgs: func() map[string]any { + outsideDir := t.TempDir() + outsideFile := filepath.Join(outsideDir, "secret.txt") + require.NoError(t, os.WriteFile(outsideFile, []byte("secret"), 0o644)) + require.NoError(t, os.Symlink(outsideFile, filepath.Join(tempRoot, "escape.txt"))) + return map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "src/worker.js", + "shared_path": "escape.txt", + "message": "Patch worker", + "branch": "feature/test", + } + }(), + expectError: true, + expectedErrMsg: "shared_path must stay within the shared directory", }, } @@ -2161,6 +2195,142 @@ func Test_CreateOrUpdateFileFromSharedPath(t *testing.T) { } } +func Test_PushFilesFromSharedPaths(t *testing.T) { + serverTool := PushFilesFromSharedPaths(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.Equal(t, "push_files_from_shared_paths", tool.Name) + assert.Contains(t, schema.Properties, "files") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "branch", "files", "message"}) + + tempRoot := t.TempDir() + t.Setenv("GITHUB_MCP_SHARED_ROOT", tempRoot) + require.NoError(t, os.MkdirAll(filepath.Join(tempRoot, "batch"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(tempRoot, "batch", "one.js"), []byte("export const one = 1;\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(tempRoot, "batch", "two.js"), []byte(strings.Repeat("x", 32*1024)), 0o644)) + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedErrMsg string + validate func(t *testing.T, result *mcp.CallToolResult) + }{ + { + name: "successful multi-file push", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/git/ref/heads/feature/test": mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/feature/test"), + Object: &github.GitObject{ + SHA: github.Ptr("base-commit-sha"), + }, + }), + "GET /repos/{owner}/{repo}/git/ref/{ref:.*}": mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/feature/test"), + Object: &github.GitObject{ + SHA: github.Ptr("base-commit-sha"), + }, + }), + "GET /repos/owner/repo/git/commits/base-commit-sha": mockResponse(t, http.StatusOK, &github.Commit{ + SHA: github.Ptr("base-commit-sha"), + Tree: &github.Tree{SHA: github.Ptr("base-tree-sha")}, + }), + "GET /repos/{owner}/{repo}/git/commits/{commit_sha}": mockResponse(t, http.StatusOK, &github.Commit{ + SHA: github.Ptr("base-commit-sha"), + Tree: &github.Tree{SHA: github.Ptr("base-tree-sha")}, + }), + "POST /repos/owner/repo/git/trees": mockResponse(t, http.StatusCreated, &github.Tree{ + SHA: github.Ptr("new-tree-sha"), + }), + "POST /repos/{owner}/{repo}/git/trees": mockResponse(t, http.StatusCreated, &github.Tree{ + SHA: github.Ptr("new-tree-sha"), + }), + "POST /repos/owner/repo/git/commits": mockResponse(t, http.StatusCreated, &github.Commit{ + SHA: github.Ptr("new-commit-sha"), + }), + "POST /repos/{owner}/{repo}/git/commits": mockResponse(t, http.StatusCreated, &github.Commit{ + SHA: github.Ptr("new-commit-sha"), + }), + "PATCH /repos/owner/repo/git/refs/heads/feature/test": mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/feature/test"), + Object: &github.GitObject{ + SHA: github.Ptr("new-commit-sha"), + }, + }), + "PATCH /repos/{owner}/{repo}/git/refs/{ref:.*}": mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/feature/test"), + Object: &github.GitObject{ + SHA: github.Ptr("new-commit-sha"), + }, + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "branch": "feature/test", + "message": "Batch patch", + "files": []any{ + map[string]any{"path": "src/one.js", "shared_path": "batch/one.js"}, + map[string]any{"path": "src/two.js", "shared_path": "batch/two.js"}, + }, + }, + validate: func(t *testing.T, result *mcp.CallToolResult) { + textContent := getTextResult(t, result) + var resp SharedPushFilesResponse + err := json.Unmarshal([]byte(textContent.Text), &resp) + require.NoError(t, err) + assert.Equal(t, "new-commit-sha", resp.CommitSHA) + assert.Equal(t, "feature/test", resp.Branch) + assert.Equal(t, []string{"src/one.js", "src/two.js"}, resp.ChangedPaths) + assert.Equal(t, 2, resp.FileCount) + }, + }, + { + name: "reject traversal in multi-file push", + mockedClient: NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "branch": "feature/test", + "message": "Batch patch", + "files": []any{ + map[string]any{"path": "src/one.js", "shared_path": "../secret.txt"}, + }, + }, + expectError: true, + expectedErrMsg: "shared_path must stay within the shared directory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + if tc.expectError { + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + return + } + + require.False(t, result.IsError) + if tc.validate != nil { + tc.validate(t, result) + } + }) + } +} + func Test_CreateRepository(t *testing.T) { // Verify tool definition once serverTool := CreateRepository(translations.NullTranslationHelper) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index b3358d7c32..a6ebc4a850 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -189,6 +189,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { GetReleaseByTag(t), CreateOrUpdateFile(t), CreateOrUpdateFileFromSharedPath(t), + PushFilesFromSharedPaths(t), CreateRepository(t), ForkRepository(t), CreateBranch(t), From 5e5db8b3e13575b75252f3875d26351b8f570f0e Mon Sep 17 00:00:00 2001 From: Hermes Bot Date: Sun, 12 Jul 2026 06:48:38 +0000 Subject: [PATCH 4/5] fix: add safe GitHub file patch operations --- .../__toolsnaps__/create_or_update_file.snap | 47 +- pkg/github/__toolsnaps__/push_files.snap | 40 +- pkg/github/file_patches.go | 291 ++++++++++ pkg/github/file_patches_handler_test.go | 80 +++ pkg/github/file_patches_test.go | 68 +++ pkg/github/repositories.go | 535 ++++++++++++++++-- pkg/github/repositories_test.go | 2 +- pkg/github/retry.go | 161 ++++++ pkg/github/search.go | 21 +- 9 files changed, 1177 insertions(+), 68 deletions(-) create mode 100644 pkg/github/file_patches.go create mode 100644 pkg/github/file_patches_handler_test.go create mode 100644 pkg/github/file_patches_test.go create mode 100644 pkg/github/retry.go diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index e6900c9053..e862e10a43 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -13,32 +13,77 @@ "description": "Content of the file", "type": "string" }, + "dry_run": { + "description": "Validate the request and report what would happen without writing to GitHub", + "type": "boolean" + }, + "end_line": { + "description": "One-based inclusive end line for patch_range.", + "type": "integer" + }, + "expected_blob_sha": { + "description": "Current Git blob SHA. Required when modifying an existing file.", + "type": "string" + }, + "expected_occurrences": { + "description": "Exact number of search matches required.", + "type": "integer" + }, "message": { "description": "Commit message", "type": "string" }, + "operation": { + "description": "Mutation mode. Omitting this keeps the legacy replace behavior.", + "enum": [ + "replace", + "patch_text", + "patch_range", + "unified_diff" + ], + "type": "string" + }, "owner": { "description": "Repository owner (username or organization)", "type": "string" }, + "patch": { + "description": "Single-file unified diff for unified_diff.", + "type": "string" + }, "path": { "description": "Path where to create/update the file", "type": "string" }, + "replace": { + "description": "Replacement text for patch_text.", + "type": "string" + }, + "replacement": { + "description": "Replacement text for patch_range.", + "type": "string" + }, "repo": { "description": "Repository name", "type": "string" }, + "search": { + "description": "Exact text to find for patch_text.", + "type": "string" + }, "sha": { "description": "The blob SHA of the file being replaced. Required if the file already exists.", "type": "string" + }, + "start_line": { + "description": "One-based inclusive start line for patch_range.", + "type": "integer" } }, "required": [ "owner", "repo", "path", - "content", "message", "branch" ], diff --git a/pkg/github/__toolsnaps__/push_files.snap b/pkg/github/__toolsnaps__/push_files.snap index df6c4d1e79..b8ce3cbadb 100644 --- a/pkg/github/__toolsnaps__/push_files.snap +++ b/pkg/github/__toolsnaps__/push_files.snap @@ -9,6 +9,10 @@ "description": "Branch to push to", "type": "string" }, + "dry_run": { + "description": "Validate the request and report what would happen without writing to GitHub", + "type": "boolean" + }, "files": { "description": "Array of file objects to push, each object with path (string) and content (string)", "items": { @@ -18,14 +22,46 @@ "description": "file content", "type": "string" }, + "end_line": { + "type": "integer" + }, + "expected_blob_sha": { + "type": "string" + }, + "expected_occurrences": { + "type": "integer" + }, + "operation": { + "enum": [ + "replace", + "patch_text", + "patch_range", + "unified_diff" + ], + "type": "string" + }, + "patch": { + "type": "string" + }, "path": { "description": "path to the file", "type": "string" + }, + "replace": { + "type": "string" + }, + "replacement": { + "type": "string" + }, + "search": { + "type": "string" + }, + "start_line": { + "type": "integer" } }, "required": [ - "path", - "content" + "path" ], "type": "object" }, diff --git a/pkg/github/file_patches.go b/pkg/github/file_patches.go new file mode 100644 index 0000000000..4df6b16dc0 --- /dev/null +++ b/pkg/github/file_patches.go @@ -0,0 +1,291 @@ +package github + +import ( + "fmt" + "strconv" + "strings" +) + +type filePatchInput struct { + Operation string + Content string + Search string + Replace string + ExpectedOccurrences int + StartLine int + EndLine int + Replacement string + Patch string +} + +type filePatchPreview struct { + Operation string `json:"operation"` + MatchedOccurrences int `json:"matched_occurrences"` + ChangedLineCount int `json:"changed_line_count"` + Hunks []string `json:"hunks"` +} + +func applyFilePatch(before string, input filePatchInput) (string, filePatchPreview, error) { + operation := input.Operation + if operation == "" { + operation = "replace" + } + preview := filePatchPreview{Operation: operation} + + var after string + switch operation { + case "replace": + after = input.Content + preview.MatchedOccurrences = 1 + case "patch_text": + if input.Search == "" { + return "", preview, fmt.Errorf("search is required for patch_text") + } + matches := strings.Count(before, input.Search) + preview.MatchedOccurrences = matches + expected := input.ExpectedOccurrences + if expected == 0 { + expected = 1 + } + if matches != expected { + return "", preview, fmt.Errorf("search/replace expected %d occurrence(s), found %d", expected, matches) + } + after = strings.ReplaceAll(before, input.Search, input.Replace) + case "patch_range": + lines := splitFileLines(before) + if input.StartLine < 1 || input.EndLine < input.StartLine || input.EndLine > len(lines) { + return "", preview, fmt.Errorf("start_line and end_line must identify an existing non-empty range") + } + replacement := splitFileLines(input.Replacement) + afterLines := append([]string{}, lines[:input.StartLine-1]...) + afterLines = append(afterLines, replacement...) + afterLines = append(afterLines, lines[input.EndLine:]...) + after = strings.Join(afterLines, "\n") + preview.MatchedOccurrences = 1 + preview.Hunks = []string{fmt.Sprintf("lines %d-%d", input.StartLine, input.EndLine)} + case "unified_diff": + var err error + after, preview, err = applyUnifiedDiff(before, input.Patch) + if err != nil { + return "", preview, err + } + default: + return "", preview, fmt.Errorf("operation must be one of replace, patch_text, patch_range, unified_diff") + } + + if operation != "replace" { + preview.ChangedLineCount = changedLineCount(before, after) + if strings.TrimSpace(after) == "" { + return "", preview, fmt.Errorf("resulting file is empty") + } + if len(before) >= 512 && len(after) < len(before)/8 { + return "", preview, fmt.Errorf("resulting file looks accidentally truncated") + } + } + return after, preview, nil +} + +func unrelatedFileContentPreserved(before, after string, input filePatchInput) bool { + if input.Operation == "replace" || before == after { + return true + } + switch input.Operation { + case "patch_text": + at := strings.Index(before, input.Search) + if at < 0 { + return false + } + return strings.HasPrefix(after, before[:at]) && strings.HasSuffix(after, before[at+len(input.Search):]) + case "patch_range": + beforeLines, afterLines := splitFileLines(before), splitFileLines(after) + prefix := beforeLines[:input.StartLine-1] + suffix := beforeLines[input.EndLine:] + if len(afterLines) < len(prefix)+len(suffix) { + return false + } + return strings.Join(afterLines[:len(prefix)], "\n") == strings.Join(prefix, "\n") && strings.Join(afterLines[len(afterLines)-len(suffix):], "\n") == strings.Join(suffix, "\n") + default: + beforeLines, afterLines := splitFileLines(before), splitFileLines(after) + if len(beforeLines) == 0 || len(afterLines) == 0 { + return false + } + return strings.Contains(after, beforeLines[0]) && strings.Contains(after, beforeLines[len(beforeLines)-1]) + } +} + +func splitFileLines(value string) []string { + return strings.Split(value, "\n") +} + +func changedLineCount(before, after string) int { + a := splitFileLines(before) + b := splitFileLines(after) + count := 0 + for i := 0; i < len(a) || i < len(b); i++ { + var left, right string + if i < len(a) { + left = a[i] + } + if i < len(b) { + right = b[i] + } + if left != right { + count++ + } + } + return count +} + +type unifiedHunk struct { + oldStart int + oldCount int + newStart int + newCount int + lines []string +} + +func applyUnifiedDiff(before, patch string) (string, filePatchPreview, error) { + preview := filePatchPreview{Operation: "unified_diff"} + if strings.TrimSpace(patch) == "" { + return "", preview, fmt.Errorf("patch is required") + } + lines := strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") + var hunks []unifiedHunk + for i := 0; i < len(lines); i++ { + line := lines[i] + if strings.HasPrefix(line, "--- ") || strings.HasPrefix(line, "+++ ") || strings.HasPrefix(line, "diff ") || strings.HasPrefix(line, "index ") { + continue + } + if !strings.HasPrefix(line, "@@ ") { + if strings.TrimSpace(line) == "" { + continue + } + return "", preview, fmt.Errorf("malformed unified diff line %q", line) + } + hunk, err := parseUnifiedHunkHeader(line) + if err != nil { + return "", preview, err + } + i++ + for oldSeen, newSeen := 0, 0; i < len(lines); i++ { + entry := lines[i] + if strings.HasPrefix(entry, "@@ ") { + i-- + break + } + if entry == "" && i == len(lines)-1 { + break + } + if entry == "\\ No newline at end of file" { + continue + } + if entry == "" || (entry[0] != ' ' && entry[0] != '-' && entry[0] != '+') { + return "", preview, fmt.Errorf("malformed unified diff hunk line %q", entry) + } + hunk.lines = append(hunk.lines, entry) + switch entry[0] { + case ' ': + oldSeen++ + newSeen++ + case '-': + oldSeen++ + case '+': + newSeen++ + } + if oldSeen > hunk.oldCount || newSeen > hunk.newCount { + return "", preview, fmt.Errorf("unified diff hunk counts exceeded") + } + } + if countOldNew(hunk.lines) != [2]int{hunk.oldCount, hunk.newCount} { + return "", preview, fmt.Errorf("unified diff hunk counts do not match") + } + hunks = append(hunks, hunk) + } + if len(hunks) == 0 { + return "", preview, fmt.Errorf("unified diff contains no hunks") + } + + beforeLines := splitFileLines(before) + var out []string + oldCursor := 1 + lastEnd := 0 + for _, hunk := range hunks { + if hunk.oldStart < oldCursor || hunk.oldStart <= lastEnd { + return "", preview, fmt.Errorf("unified diff hunks overlap or are out of order") + } + if hunk.oldStart < 1 || hunk.oldStart+hunk.oldCount-1 > len(beforeLines) { + return "", preview, fmt.Errorf("unified diff hunk is outside the file") + } + out = append(out, beforeLines[oldCursor-1:hunk.oldStart-1]...) + pos := hunk.oldStart - 1 + for _, entry := range hunk.lines { + text := entry[1:] + switch entry[0] { + case ' ': + if beforeLines[pos] != text { + return "", preview, fmt.Errorf("unified diff context does not match at line %d", pos+1) + } + out = append(out, text) + pos++ + case '-': + if beforeLines[pos] != text { + return "", preview, fmt.Errorf("unified diff removal does not match at line %d", pos+1) + } + pos++ + case '+': + out = append(out, text) + } + } + oldCursor = pos + 1 + lastEnd = pos + preview.MatchedOccurrences++ + preview.Hunks = append(preview.Hunks, fmt.Sprintf("-%d,%d +%d,%d", hunk.oldStart, hunk.oldCount, hunk.newStart, hunk.newCount)) + } + out = append(out, beforeLines[oldCursor-1:]...) + return strings.Join(out, "\n"), preview, nil +} + +func countOldNew(lines []string) [2]int { + var result [2]int + for _, line := range lines { + if line[0] != '+' { + result[0]++ + } + if line[0] != '-' { + result[1]++ + } + } + return result +} + +func parseUnifiedHunkHeader(line string) (unifiedHunk, error) { + var h unifiedHunk + parts := strings.Fields(strings.TrimSuffix(strings.TrimPrefix(line, "@@ "), " @@")) + if len(parts) < 2 || !strings.HasPrefix(parts[0], "-") || !strings.HasPrefix(parts[1], "+") { + return h, fmt.Errorf("malformed unified diff hunk header %q", line) + } + parse := func(value string) (int, int, error) { + value = value[1:] + pieces := strings.SplitN(value, ",", 2) + start, err := strconv.Atoi(pieces[0]) + if err != nil || start < 1 { + return 0, 0, fmt.Errorf("malformed unified diff line range %q", value) + } + count := 1 + if len(pieces) == 2 { + count, err = strconv.Atoi(pieces[1]) + if err != nil || count < 0 { + return 0, 0, fmt.Errorf("malformed unified diff line count %q", value) + } + } + return start, count, nil + } + var err error + if h.oldStart, h.oldCount, err = parse(parts[0]); err != nil { + return h, err + } + if h.newStart, h.newCount, err = parse(parts[1]); err != nil { + return h, err + } + return h, nil +} diff --git a/pkg/github/file_patches_handler_test.go b/pkg/github/file_patches_handler_test.go new file mode 100644 index 0000000000..82156ea9c7 --- /dev/null +++ b/pkg/github/file_patches_handler_test.go @@ -0,0 +1,80 @@ +package github + +import ( + "context" + "encoding/base64" + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v87/github" + "github.com/stretchr/testify/require" +) + +func TestCreateOrUpdateFileOneLinePatchLargeFileDryRunAndVerification(t *testing.T) { + lines := []string{"header"} + for i := 0; i < 1000; i++ { + lines = append(lines, "unrelated line") + } + lines = append(lines, "target = old") + for i := 0; i < 1000; i++ { + lines = append(lines, "unrelated tail") + } + lines = append(lines, "footer") + before := strings.Join(lines, "\n") + after := strings.Replace(before, "target = old", "target = new", 1) + current := &github.RepositoryContent{Path: github.Ptr("fixture.txt"), SHA: github.Ptr("blob-old"), Type: github.Ptr("file"), Content: github.Ptr(base64.StdEncoding.EncodeToString([]byte(before))), Encoding: github.Ptr("base64")} + updated := &github.RepositoryContent{Path: github.Ptr("fixture.txt"), SHA: github.Ptr("blob-new"), Type: github.Ptr("file"), Content: github.Ptr(base64.StdEncoding.EncodeToString([]byte(after))), Encoding: github.Ptr("base64")} + commit := &github.RepositoryContentResponse{Content: updated} + getCount := 0 + clientHTTP := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + getCount++ + payload := current + if getCount > 1 { + payload = updated + } + mockResponse(t, http.StatusOK, payload)(w, nil) + }, + PutReposContentsByOwnerByRepoByPath: mockResponse(t, http.StatusOK, commit), + }) + client := mustNewGHClient(t, clientHTTP) + deps := BaseDeps{Client: client} + serverTool := CreateOrUpdateFile(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", "repo": "repo", "path": "fixture.txt", "message": "patch fixture", "branch": "main", + "operation": "patch_text", "expected_blob_sha": "blob-old", "search": "target = old", "replace": "target = new", "expected_occurrences": 1, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + var payload map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &payload)) + require.Equal(t, true, payload["ok"]) + require.Equal(t, true, payload["applied"]) + require.Equal(t, true, payload["verification"].(map[string]any)["exact_content"]) + + dryGetCount := 0 + dryHTTP := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + dryGetCount++ + mockResponse(t, http.StatusOK, current)(w, nil) + }, + }) + dryDeps := BaseDeps{Client: mustNewGHClient(t, dryHTTP)} + dryHandler := serverTool.Handler(dryDeps) + dryRequest := createMCPRequest(map[string]any{ + "owner": "owner", "repo": "repo", "path": "fixture.txt", "message": "preview", "branch": "main", "operation": "patch_text", + "expected_blob_sha": "blob-old", "search": "target = old", "replace": "target = new", "expected_occurrences": 1, "dry_run": true, + }) + dryResult, err := dryHandler(ContextWithDeps(context.Background(), dryDeps), &dryRequest) + require.NoError(t, err) + require.False(t, dryResult.IsError) + var dryPayload map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, dryResult).Text), &dryPayload)) + require.Equal(t, true, dryPayload["dry_run"]) + require.Equal(t, 1, dryGetCount) +} diff --git a/pkg/github/file_patches_test.go b/pkg/github/file_patches_test.go new file mode 100644 index 0000000000..9c4f573847 --- /dev/null +++ b/pkg/github/file_patches_test.go @@ -0,0 +1,68 @@ +package github + +import ( + "strings" + "testing" +) + +func TestApplyFilePatchOneLinePreservesLargeFile(t *testing.T) { + before := strings.Join([]string{"header", "target = old", "middle", "footer"}, "\n") + after, preview, err := applyFilePatch(before, filePatchInput{Operation: "patch_text", Search: "target = old", Replace: "target = new", ExpectedOccurrences: 1}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(after, "header") || !strings.Contains(after, "middle") || !strings.Contains(after, "footer") { + t.Fatalf("unrelated content was lost: %q", after) + } + if preview.MatchedOccurrences != 1 { + t.Fatalf("matched occurrences = %d", preview.MatchedOccurrences) + } +} + +func TestApplyFilePatchUnifiedDiffMultiHunk(t *testing.T) { + before := "one\ntwo\nthree\nfour\nfive\nsix" + patch := "--- a/file.txt\n+++ b/file.txt\n@@ -1,2 +1,2 @@\n one\n-two\n+TWO\n@@ -5,2 +5,2 @@\n five\n-six\n+SIX\n" + after, preview, err := applyFilePatch(before, filePatchInput{Operation: "unified_diff", Patch: patch}) + if err != nil { + t.Fatal(err) + } + if after != "one\nTWO\nthree\nfour\nfive\nSIX" { + t.Fatalf("unexpected result: %q", after) + } + if len(preview.Hunks) != 2 { + t.Fatalf("hunks = %#v", preview.Hunks) + } +} + +func TestApplyFilePatchRejectsAmbiguousMalformedAndOverlapping(t *testing.T) { + if _, _, err := applyFilePatch("x\nx", filePatchInput{Operation: "patch_text", Search: "x", Replace: "y", ExpectedOccurrences: 1}); err == nil { + t.Fatal("ambiguous patch was accepted") + } + if _, _, err := applyFilePatch("one\ntwo", filePatchInput{Operation: "unified_diff", Patch: "@@ -1,1 +1,1 @@\n-one\n"}); err == nil { + t.Fatal("malformed hunk was accepted") + } + patch := "@@ -1,1 +1,1 @@\n-one\n+ONE\n@@ -1,1 +1,1 @@\n-one\n+one\n" + if _, _, err := applyFilePatch("one", filePatchInput{Operation: "unified_diff", Patch: patch}); err == nil { + t.Fatal("overlapping hunks were accepted") + } +} + +func TestApplyFilePatchRejectsTruncationAndEmptyResult(t *testing.T) { + before := strings.Repeat("line\n", 200) + if _, _, err := applyFilePatch(before, filePatchInput{Operation: "patch_text", Search: "line\n", Replace: "", ExpectedOccurrences: 200}); err == nil { + t.Fatal("truncated result was accepted") + } + if _, _, err := applyFilePatch("substantive", filePatchInput{Operation: "patch_text", Search: "substantive", Replace: "", ExpectedOccurrences: 1}); err == nil { + t.Fatal("empty result was accepted") + } +} + +func TestBoundedRedactedPreviewRedactsSecrets(t *testing.T) { + preview := boundedRedactedPreview("API_TOKEN=super-secret-value\nvisible") + if strings.Contains(preview, "super-secret-value") { + t.Fatal("secret leaked in preview") + } + if !strings.Contains(preview, "REDACTED") { + t.Fatalf("preview was not redacted: %q", preview) + } +} diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 69afd50c34..f9dc73d296 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "path/filepath" + "regexp" "slices" "strconv" "strings" @@ -249,7 +250,9 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - commits, resp, err := client.Repositories.ListCommits(ctx, owner, repo, opts) + commits, resp, err := retryGitHubCall(ctx, deps, "list_commits", func(callCtx context.Context) ([]*github.RepositoryCommit, *github.Response, error) { + return client.Repositories.ListCommits(callCtx, owner, repo, opts) + }) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, fmt.Sprintf("failed to list commits: %s", sha), @@ -417,6 +420,22 @@ SHA MUST be provided for existing file updates. Type: "string", Description: "Content of the file", }, + "operation": { + Type: "string", + Enum: []any{"replace", "patch_text", "patch_range", "unified_diff"}, + Description: "Mutation mode. Omitting this keeps the legacy replace behavior.", + }, + "expected_blob_sha": { + Type: "string", + Description: "Current Git blob SHA. Required when modifying an existing file.", + }, + "search": {Type: "string", Description: "Exact text to find for patch_text."}, + "replace": {Type: "string", Description: "Replacement text for patch_text."}, + "expected_occurrences": {Type: "integer", Description: "Exact number of search matches required."}, + "start_line": {Type: "integer", Description: "One-based inclusive start line for patch_range."}, + "end_line": {Type: "integer", Description: "One-based inclusive end line for patch_range."}, + "replacement": {Type: "string", Description: "Replacement text for patch_range."}, + "patch": {Type: "string", Description: "Single-file unified diff for unified_diff."}, "message": { Type: "string", Description: "Commit message", @@ -429,8 +448,12 @@ SHA MUST be provided for existing file updates. Type: "string", Description: "The blob SHA of the file being replaced. Required if the file already exists.", }, + "dry_run": { + Type: "boolean", + Description: "Validate the request and report what would happen without writing to GitHub", + }, }, - Required: []string{"owner", "repo", "path", "content", "message", "branch"}, + Required: []string{"owner", "repo", "path", "message", "branch"}, }, }, []scopes.Scope{scopes.Repo}, @@ -447,7 +470,7 @@ SHA MUST be provided for existing file updates. if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - content, err := RequiredParam[string](args, "content") + content, err := OptionalParam[string](args, "content") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -463,11 +486,73 @@ SHA MUST be provided for existing file updates. if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + dryRun, err := OptionalParam[bool](args, "dry_run") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + _, operationProvided := args["operation"] + operation, err := OptionalParam[string](args, "operation") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + expectedBlobSHA, err := OptionalParam[string](args, "expected_blob_sha") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if expectedBlobSHA == "" { + expectedBlobSHA = sha + } + expectedOccurrences, err := optionalIntArgument(args, "expected_occurrences") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + startLine, err := optionalIntArgument(args, "start_line") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + endLine, err := optionalIntArgument(args, "end_line") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + search, err := OptionalParam[string](args, "search") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + replace, err := OptionalParam[string](args, "replace") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + replacement, err := OptionalParam[string](args, "replacement") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + patch, err := OptionalParam[string](args, "patch") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if isSecretLikeRepoPath(path) { + return newBlockedFileToolResult(path, "secret_like_file"), nil, nil + } + if operation == "" { + operation = "replace" + } + if operation == "replace" && content == "" { + return utils.NewToolResultError("content is required for replace"), nil, nil + } + if reason := detectSecretLikeContent(path, content); reason != "" { + return newBlockedFileToolResult(path, reason), nil, nil + } client, err := deps.GetClient(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - return createOrUpdateFileResult(ctx, deps, client, owner, repo, path, content, message, branch, sha) + if !operationProvided && args["expected_blob_sha"] == nil { + return createOrUpdateFileResult(ctx, deps, client, owner, repo, path, content, message, branch, sha) + } + return createOrUpdateFileOperation(ctx, deps, client, owner, repo, path, message, branch, expectedBlobSHA, dryRun, filePatchInput{ + Operation: operation, Content: content, Search: search, Replace: replace, ExpectedOccurrences: expectedOccurrences, + StartLine: startLine, EndLine: endLine, Replacement: replacement, Patch: patch, + }) }, ) } @@ -711,7 +796,80 @@ func firstTextResult(result *mcp.CallToolResult) *mcp.TextContent { return nil } +type createOrUpdateFilePreview struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Path string `json:"path"` + Branch string `json:"branch"` + Action string `json:"action"` + RequiresSHA bool `json:"requires_sha"` + CurrentSHA string `json:"current_sha,omitempty"` + DryRun bool `json:"dry_run"` + SecretsChecked bool `json:"secrets_checked"` +} + +func preflightCreateOrUpdateFile(ctx context.Context, client *github.Client, owner, repo, path, branch, sha string) (*createOrUpdateFilePreview, *mcp.CallToolResult, error) { + path = strings.TrimPrefix(path, "/") + getOpts := &github.RepositoryContentGetOptions{Ref: branch} + + preview := &createOrUpdateFilePreview{ + Owner: owner, + Repo: repo, + Path: path, + Branch: branch, + DryRun: true, + SecretsChecked: true, + } + + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + defer func() { _ = respCheck.Body.Close() }() + } + + switch { + case getErr != nil: + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return nil, ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to check if file exists", respCheck, getErr), nil + } + preview.Action = "create" + preview.RequiresSHA = false + return preview, nil, nil + case dirContent != nil: + return nil, utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path, + )), nil + case existingFile != nil: + currentSHA := existingFile.GetSHA() + preview.Action = "update" + preview.RequiresSHA = true + preview.CurrentSHA = currentSHA + if sha == "" { + return nil, utils.NewToolResultError(fmt.Sprintf( + "File already exists at %s. You must provide the current file's SHA when updating. Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.", + path, branch, path, + )), nil + } + if currentSHA != sha { + return nil, utils.NewToolResultError(fmt.Sprintf( + "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", + sha, currentSHA, branch, path, + )), nil + } + return preview, nil, nil + } + + return nil, utils.NewToolResultError("failed to prepare file update"), nil +} + func createOrUpdateFileResult(ctx context.Context, deps ToolDependencies, client *github.Client, owner, repo, path, content, message, branch, sha string) (*mcp.CallToolResult, any, error) { + if isSecretLikeRepoPath(path) { + return newBlockedFileToolResult(path, "secret_like_file"), nil, nil + } + if reason := detectSecretLikeContent(path, content); reason != "" { + return newBlockedFileToolResult(path, reason), nil, nil + } + // json.Marshal encodes byte arrays with base64, which is required for the API. contentBytes := []byte(content) @@ -726,59 +884,8 @@ func createOrUpdateFileResult(ctx context.Context, deps ToolDependencies, client } path = strings.TrimPrefix(path, "/") - getOpts := &github.RepositoryContentGetOptions{Ref: branch} - - if sha != "" { - existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) - if respCheck != nil { - _ = respCheck.Body.Close() - } - switch { - case getErr != nil: - if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to verify file SHA", - respCheck, - getErr, - ), nil, nil - } - case dirContent != nil: - return utils.NewToolResultError(fmt.Sprintf( - "Path %s is a directory, not a file. This tool only works with files.", - path)), nil, nil - case existingFile != nil: - currentSHA := existingFile.GetSHA() - if currentSHA != sha { - return utils.NewToolResultError(fmt.Sprintf( - "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ - "Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", - sha, currentSHA, branch, path)), nil, nil - } - } - } else { - existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) - if respCheck != nil { - _ = respCheck.Body.Close() - } - switch { - case getErr != nil: - if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to check if file exists", - respCheck, - getErr, - ), nil, nil - } - case dirContent != nil: - return utils.NewToolResultError(fmt.Sprintf( - "Path %s is a directory, not a file. This tool only works with files.", - path)), nil, nil - case existingFile != nil: - return utils.NewToolResultError(fmt.Sprintf( - "File already exists at %s. You must provide the current file's SHA when updating. "+ - "Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.", - path, branch, path)), nil, nil - } + if _, previewResult, previewErr := preflightCreateOrUpdateFile(ctx, client, owner, repo, path, branch, sha); previewResult != nil || previewErr != nil { + return previewResult, nil, previewErr } fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts) @@ -803,6 +910,137 @@ func createOrUpdateFileResult(ctx context.Context, deps ToolDependencies, client return MarshalledTextResult(minimalResponse), nil, nil } +func decodeGitHubFileContent(file *github.RepositoryContent) (string, error) { + if file == nil || file.Content == nil { + return "", fmt.Errorf("GitHub did not return complete file content") + } + encodedContent, err := file.GetContent() + if err != nil { + return "", fmt.Errorf("failed to read current GitHub blob content: %w", err) + } + return encodedContent, nil +} + +func boundedRedactedPreview(content string) string { + redacted, _ := redactSecretLikeContent(content) + if len(redacted) > 4000 { + return redacted[:4000] + "\n...[truncated]" + } + return redacted +} + +func optionalIntArgument(args map[string]any, key string) (int, error) { + value, ok := args[key] + if !ok || value == nil { + return 0, nil + } + switch typed := value.(type) { + case int: + return typed, nil + case int64: + return int(typed), nil + case float64: + if typed != float64(int(typed)) { + return 0, fmt.Errorf("parameter %s must be an integer", key) + } + return int(typed), nil + default: + return 0, fmt.Errorf("parameter %s is not of type int, is %T", key, value) + } +} + +func getCurrentGitHubFile(ctx context.Context, client *github.Client, owner, repo, path, branch string) (*github.RepositoryContent, string, *github.Response, error) { + file, dirs, resp, err := client.Repositories.GetContents(ctx, owner, repo, strings.TrimPrefix(path, "/"), &github.RepositoryContentGetOptions{Ref: branch}) + if err != nil { + return nil, "", resp, err + } + if dirs != nil { + return nil, "", resp, fmt.Errorf("path %s is a directory, not a file", path) + } + content, err := decodeGitHubFileContent(file) + if err != nil { + return file, "", resp, err + } + return file, content, resp, nil +} + +func createOrUpdateFileOperation(ctx context.Context, deps ToolDependencies, client *github.Client, owner, repo, path, message, branch, expectedSHA string, dryRun bool, patch filePatchInput) (*mcp.CallToolResult, any, error) { + path = strings.TrimPrefix(path, "/") + file, before, resp, err := getCurrentGitHubFile(ctx, client, owner, repo, path, branch) + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + exists := err == nil && file != nil + if err != nil && (resp == nil || resp.StatusCode != http.StatusNotFound) { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to fetch current file before mutation", resp, err), nil, nil + } + if !exists && patch.Operation != "replace" { + return utils.NewToolResultError("patch operations require an existing file"), nil, nil + } + if exists { + if expectedSHA == "" { + return utils.NewToolResultError(fmt.Sprintf("File already exists at %s. You must provide expected_blob_sha when modifying it.", path)), nil, nil + } + if expectedSHA != file.GetSHA() { + return utils.NewToolResultError(fmt.Sprintf("SHA mismatch: provided SHA %s is stale. Current file SHA is %s.", expectedSHA, file.GetSHA())), nil, nil + } + } + if patch.Operation == "replace" && !exists { + before = "" + } + after, patchPreview, err := applyFilePatch(before, patch) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if reason := detectSecretLikeContent(path, after); reason != "" { + return newBlockedFileToolResult(path, reason), nil, nil + } + if !unrelatedFileContentPreserved(before, after, patch) { + return utils.NewToolResultError("resulting file did not preserve unrelated content"), nil, nil + } + redactedBefore, _ := redactSecretLikeContent(before) + redactedAfter, _ := redactSecretLikeContent(after) + bounded := func(value string) string { + if len(value) > 4000 { + return value[:4000] + "\n...[truncated]" + } + return value + } + preview := map[string]any{ + "ok": true, "dry_run": dryRun, "operation": patchPreview.Operation, "owner": owner, "repo": repo, "path": path, "branch": branch, + "action": map[bool]string{true: "update", false: "create"}[exists], "expected_blob_sha": expectedSHA, "matched_occurrences": patchPreview.MatchedOccurrences, + "changed_line_count": patchPreview.ChangedLineCount, "hunks": patchPreview.Hunks, "before_preview": bounded(redactedBefore), "after_preview": bounded(redactedAfter), "redacted": redactedBefore != before || redactedAfter != after, + } + if dryRun { + return MarshalledTextResult(preview), nil, nil + } + options := &github.RepositoryContentFileOptions{Message: github.Ptr(message), Content: []byte(after), Branch: github.Ptr(branch)} + if exists { + options.SHA = github.Ptr(expectedSHA) + } + committed, writeResp, err := client.Repositories.CreateFile(ctx, owner, repo, path, options) + if writeResp != nil && writeResp.Body != nil { + defer func() { _ = writeResp.Body.Close() }() + } + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create/update file", writeResp, err), nil, nil + } + verified, committedContent, verifyResp, err := getCurrentGitHubFile(ctx, client, owner, repo, path, branch) + if verifyResp != nil && verifyResp.Body != nil { + defer func() { _ = verifyResp.Body.Close() }() + } + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("post-commit verification failed: %v", err)), nil, nil + } + if committedContent != after { + return utils.NewToolResultError("post-commit verification failed: committed content does not match the requested change"), nil, nil + } + if exists && verified.GetSHA() == expectedSHA { + return utils.NewToolResultError("post-commit verification failed: blob SHA did not change"), nil, nil + } + return MarshalledTextResult(map[string]any{"ok": true, "applied": true, "verification": map[string]any{"ok": true, "exact_content": true, "unrelated_content_preserved": true}, "content": convertToMinimalFileContentResponse(committed)}), nil, nil +} + func CreateOrUpdateFileFromSharedPath(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( ToolsetMetadataRepos, @@ -1158,6 +1396,80 @@ func isSecretLikeRepoPath(path string) bool { } } +var secretLikeContentPatterns = []struct { + reason string + regex *regexp.Regexp +}{ + {reason: "private_key_material", regex: regexp.MustCompile(`-----BEGIN [A-Z ]*PRIVATE KEY-----`)}, + {reason: "github_token", regex: regexp.MustCompile(`\b(?:gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})\b`)}, + {reason: "aws_access_key", regex: regexp.MustCompile(`\bAKIA[0-9A-Z]{16}\b`)}, + {reason: "openai_api_key", regex: regexp.MustCompile(`\bsk-(?:live|proj|ant)-[A-Za-z0-9]{16,}\b`)}, + {reason: "slack_token", regex: regexp.MustCompile(`\bxox[baprs]-[A-Za-z0-9-]{10,}\b`)}, +} + +var secretRedactionPatterns = []struct { + regex *regexp.Regexp + replacement string +}{ + { + regex: regexp.MustCompile(`(?s)-----BEGIN [A-Z ]*PRIVATE KEY-----.*?-----END [A-Z ]*PRIVATE KEY-----`), + replacement: "***REDACTED PRIVATE KEY***", + }, + { + regex: regexp.MustCompile(`\b(?:gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})\b`), + replacement: "***REDACTED GITHUB TOKEN***", + }, + { + regex: regexp.MustCompile(`\bAKIA[0-9A-Z]{16}\b`), + replacement: "***REDACTED AWS KEY***", + }, + { + regex: regexp.MustCompile(`\bsk-(?:live|proj|ant)-[A-Za-z0-9]{16,}\b`), + replacement: "***REDACTED API KEY***", + }, + { + regex: regexp.MustCompile(`\bxox[baprs]-[A-Za-z0-9-]{10,}\b`), + replacement: "***REDACTED SLACK TOKEN***", + }, + { + regex: regexp.MustCompile(`(?i)\bBearer\s+[A-Za-z0-9._\-+/=]{12,}\b`), + replacement: "Bearer ***REDACTED***", + }, + { + regex: regexp.MustCompile(`(?im)^([A-Z][A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|API_KEY|ACCESS_KEY|PRIVATE_KEY)[A-Z0-9_]*\s*[:=]\s*).+$`), + replacement: "${1}***REDACTED***", + }, + { + regex: regexp.MustCompile(`(?i)\b((?:access[_-]?token|refresh[_-]?token|api[_-]?key|client[_-]?secret|webhook[_-]?secret|password|private[_-]?key)\b\s*[:=]\s*)(?:"[^"]*"|'[^']*'|[^'",\s]+)`), + replacement: "${1}***REDACTED***", + }, +} + +func detectSecretLikeContent(path, content string) string { + if isSecretLikeRepoPath(path) { + return "secret_like_file" + } + for _, pattern := range secretLikeContentPatterns { + if pattern.regex.MatchString(content) { + return pattern.reason + } + } + return "" +} + +func redactSecretLikeContent(content string) (string, bool) { + redacted := content + changed := false + for _, pattern := range secretRedactionPatterns { + next := pattern.regex.ReplaceAllString(redacted, pattern.replacement) + if next != redacted { + changed = true + redacted = next + } + } + return redacted, changed +} + func detectFileMimeType(path, fallback string) string { if strings.EqualFold(filepath.Base(path), "Dockerfile") { return "text/x-dockerfile" @@ -1435,10 +1747,19 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool opts := &github.RepositoryContentGetOptions{Ref: ref} // Always call GitHub Contents API first to get metadata including SHA and determine if it's a file or directory - fileContent, dirContent, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts) + type contentsResult struct { + fileContent *github.RepositoryContent + dirContent []*github.RepositoryContent + } + contents, respContents, err := retryGitHubCall(ctx, deps, "get_file_contents:GetContents", func(callCtx context.Context) (contentsResult, *github.Response, error) { + fileContent, dirContent, respContents, err := client.Repositories.GetContents(callCtx, owner, repo, path, opts) + return contentsResult{fileContent: fileContent, dirContent: dirContent}, respContents, err + }) if respContents != nil { defer func() { _ = respContents.Body.Close() }() } + fileContent := contents.fileContent + dirContent := contents.dirContent // The path does not point to a file or directory. // Instead let's try to find it in the Git Tree by matching the end of the path. @@ -1481,7 +1802,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if rawErr != nil { return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub raw content client: %s", rawErr)), nil, nil } - rawResp, rawErr := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts) + rawResp, rawErr := retryGitHubHTTPCall(ctx, deps, "get_file_contents:GetRawContent", func(callCtx context.Context) (*http.Response, error) { + return rawClient.GetRawContent(callCtx, owner, repo, path, rawOpts) + }) if rawErr != nil { return utils.NewToolResultError(fmt.Sprintf("failed to fetch raw file content: %s", rawErr)), nil, nil } @@ -1513,7 +1836,13 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if startLine > 0 || endLine > 0 { var truncated bool content, startLine, endLine, truncated = sliceTextByLines(content, startLine, endLine) + content, redacted := redactSecretLikeContent(content) result := newInlineFileToolResult(owner, repo, path, rawOpts.Ref, fileSHA, contentType, content, startLine, endLine, truncated) + if redacted { + if structured, ok := result.StructuredContent.(map[string]any); ok { + structured["redacted"] = true + } + } if successNote != "" { text := result.Content[0].(*mcp.TextContent) text.Text = strings.Replace(text.Text, "\n\n```", fmt.Sprintf("%s\n\n```", successNote), 1) @@ -1525,7 +1854,13 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool maxBytes = 128 * 1024 } content, truncated := trimTextByBytes(content, maxBytes) + content, redacted := redactSecretLikeContent(content) result := newInlineFileToolResult(owner, repo, path, rawOpts.Ref, fileSHA, contentType, content, 1, strings.Count(content, "\n")+1, truncated) + if redacted { + if structured, ok := result.StructuredContent.(map[string]any); ok { + structured["redacted"] = true + } + } if successNote != "" { text := result.Content[0].(*mcp.TextContent) text.Text = strings.Replace(text.Text, "\n\n```", fmt.Sprintf("%s\n\n```", successNote), 1) @@ -1983,14 +2318,27 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "string", Description: "file content", }, + "operation": {Type: "string", Enum: []any{"replace", "patch_text", "patch_range", "unified_diff"}}, + "expected_blob_sha": {Type: "string"}, + "search": {Type: "string"}, + "replace": {Type: "string"}, + "expected_occurrences": {Type: "integer"}, + "start_line": {Type: "integer"}, + "end_line": {Type: "integer"}, + "replacement": {Type: "string"}, + "patch": {Type: "string"}, }, - Required: []string{"path", "content"}, + Required: []string{"path"}, }, }, "message": { Type: "string", Description: "Commit message", }, + "dry_run": { + Type: "boolean", + Description: "Validate the request and report what would happen without writing to GitHub", + }, }, Required: []string{"owner", "repo", "branch", "files", "message"}, }, @@ -2013,6 +2361,10 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + dryRun, err := OptionalParam[bool](args, "dry_run") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } // Parse files parameter - this should be an array of objects with path and content filesObj, ok := args["files"].([]any) @@ -2096,6 +2448,8 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { // Create tree entries for all files (or remaining files if empty repo) var entries []*github.TreeEntry + changedPaths := make([]string, 0, len(filesObj)) + patchPreviews := make([]any, 0) for _, file := range filesObj { fileMap, ok := file.(map[string]any) if !ok { @@ -2107,10 +2461,57 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError("each file must have a path"), nil, nil } - content, ok := fileMap["content"].(string) - if !ok { + content, hasContent := fileMap["content"].(string) + rawOperation, operationProvided := fileMap["operation"] + operation, _ := rawOperation.(string) + if operation == "" { + operation = "replace" + } + if !hasContent && operation == "replace" { return utils.NewToolResultError("each file must have content"), nil, nil } + expectedSHA, expectedBlobProvided := fileMap["expected_blob_sha"].(string) + if expectedSHA == "" { + expectedSHA, _ = fileMap["sha"].(string) + } + if operationProvided || expectedBlobProvided { + current, currentContent, currentResp, currentErr := getCurrentGitHubFile(ctx, client, owner, repo, path, branch) + if currentResp != nil && currentResp.Body != nil { + _ = currentResp.Body.Close() + } + if currentErr != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to fetch %s before patch: %v", path, currentErr)), nil, nil + } + if expectedSHA == "" { + return utils.NewToolResultError(fmt.Sprintf("expected_blob_sha is required for existing file %s", path)), nil, nil + } + if current.GetSHA() != expectedSHA { + return utils.NewToolResultError(fmt.Sprintf("SHA mismatch for %s: provided %s, current %s", path, expectedSHA, current.GetSHA())), nil, nil + } + occurrences, _ := optionalIntArgument(fileMap, "expected_occurrences") + startLine, _ := optionalIntArgument(fileMap, "start_line") + endLine, _ := optionalIntArgument(fileMap, "end_line") + search, _ := fileMap["search"].(string) + replace, _ := fileMap["replace"].(string) + replacement, _ := fileMap["replacement"].(string) + patchText, _ := fileMap["patch"].(string) + patchInput := filePatchInput{Operation: operation, Content: content, Search: search, Replace: replace, ExpectedOccurrences: occurrences, StartLine: startLine, EndLine: endLine, Replacement: replacement, Patch: patchText} + nextContent, patchPreview, patchErr := applyFilePatch(currentContent, patchInput) + if patchErr != nil { + return utils.NewToolResultError(fmt.Sprintf("%s: %v", path, patchErr)), nil, nil + } + if !unrelatedFileContentPreserved(currentContent, nextContent, patchInput) { + return utils.NewToolResultError(fmt.Sprintf("%s: unrelated content was not preserved", path)), nil, nil + } + content = nextContent + patchPreviews = append(patchPreviews, map[string]any{"path": path, "operation": operation, "expected_blob_sha": expectedSHA, "matched_occurrences": patchPreview.MatchedOccurrences, "hunks": patchPreview.Hunks, "before_preview": boundedRedactedPreview(currentContent), "after_preview": boundedRedactedPreview(nextContent)}) + } + if isSecretLikeRepoPath(path) { + return newBlockedFileToolResult(path, "secret_like_file"), nil, nil + } + if reason := detectSecretLikeContent(path, content); reason != "" { + return newBlockedFileToolResult(path, reason), nil, nil + } // Create a tree entry for the file entries = append(entries, &github.TreeEntry{ @@ -2119,6 +2520,16 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Type: github.Ptr("blob"), Content: github.Ptr(content), }) + changedPaths = append(changedPaths, path) + } + + if dryRun { + return MarshalledTextResult(map[string]any{ + "branch": branch, + "changed_paths": changedPaths, + "file_count": len(changedPaths), + "patch_previews": patchPreviews, + }), nil, nil } // Create a new tree with the file entries (baseCommit is now guaranteed to exist) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 68af8e42f5..4919a22712 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1656,7 +1656,7 @@ func Test_CreateOrUpdateFile(t *testing.T) { assert.Contains(t, schema.Properties, "message") assert.Contains(t, schema.Properties, "branch") assert.Contains(t, schema.Properties, "sha") - assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "path", "content", "message", "branch"}) + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "path", "message", "branch"}) // Setup mock file content response mockFileResponse := &github.RepositoryContentResponse{ diff --git a/pkg/github/retry.go b/pkg/github/retry.go new file mode 100644 index 0000000000..dc7865e29f --- /dev/null +++ b/pkg/github/retry.go @@ -0,0 +1,161 @@ +package github + +import ( + "context" + "errors" + "io" + "net" + "net/http" + "strings" + "syscall" + "time" + + gogithub "github.com/google/go-github/v87/github" +) + +func retryGitHubCall[T any]( + ctx context.Context, + deps ToolDependencies, + operation string, + fn func(context.Context) (T, *gogithub.Response, error), +) (T, *gogithub.Response, error) { + var zero T + delays := []time.Duration{0, 250 * time.Millisecond, time.Second} + + var lastValue T + var lastResp *gogithub.Response + var lastErr error + + for attempt, delay := range delays { + if delay > 0 { + select { + case <-ctx.Done(): + return zero, lastResp, ctx.Err() + case <-time.After(delay): + } + } + + value, resp, err := fn(ctx) + lastValue = value + lastResp = resp + lastErr = err + + if !isRetryableGitHubFailure(resp, err) || attempt == len(delays)-1 { + return value, resp, err + } + + statusCode := 0 + if resp != nil && resp.Response != nil { + statusCode = resp.StatusCode + } + deps.Logger(ctx).Warn( + "retrying GitHub API call after transient failure", + "operation", operation, + "attempt", attempt+1, + "status_code", statusCode, + "error", errString(err), + ) + } + + return lastValue, lastResp, lastErr +} + +func retryGitHubHTTPCall( + ctx context.Context, + deps ToolDependencies, + operation string, + fn func(context.Context) (*http.Response, error), +) (*http.Response, error) { + delays := []time.Duration{0, 250 * time.Millisecond, time.Second} + + var lastResp *http.Response + var lastErr error + + for attempt, delay := range delays { + if delay > 0 { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(delay): + } + } + + resp, err := fn(ctx) + lastResp = resp + lastErr = err + + if !isRetryableHTTPFailure(resp, err) || attempt == len(delays)-1 { + return resp, err + } + + statusCode := 0 + if resp != nil { + statusCode = resp.StatusCode + } + deps.Logger(ctx).Warn( + "retrying GitHub raw HTTP call after transient failure", + "operation", operation, + "attempt", attempt+1, + "status_code", statusCode, + "error", errString(err), + ) + } + + return lastResp, lastErr +} + +func isRetryableGitHubFailure(resp *gogithub.Response, err error) bool { + if err != nil && isRetryableNetworkError(err) { + return true + } + if resp == nil || resp.Response == nil { + return false + } + return isRetryableStatus(resp.StatusCode) +} + +func isRetryableHTTPFailure(resp *http.Response, err error) bool { + if err != nil && isRetryableNetworkError(err) { + return true + } + if resp == nil { + return false + } + return isRetryableStatus(resp.StatusCode) +} + +func isRetryableStatus(statusCode int) bool { + switch statusCode { + case http.StatusTooManyRequests, http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: + return true + default: + return false + } +} + +func isRetryableNetworkError(err error) bool { + if err == nil { + return false + } + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, io.EOF) || errors.Is(err, syscall.ECONNRESET) || errors.Is(err, syscall.EPIPE) { + return true + } + var netErr net.Error + if errors.As(err, &netErr) && (netErr.Timeout() || netErr.Temporary()) { + return true + } + + message := strings.ToLower(err.Error()) + return strings.Contains(message, "timeout") || + strings.Contains(message, "connection reset") || + strings.Contains(message, "unexpected eof") || + strings.Contains(message, "stream error") || + strings.Contains(message, "http2") +} + +func errString(err error) string { + if err == nil { + return "" + } + return err.Error() +} diff --git a/pkg/github/search.go b/pkg/github/search.go index 42ba2896f3..7ccc9b997a 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -258,7 +258,9 @@ func SearchCode(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } - result, resp, err := client.Search.Code(ctx, query, opts) + result, resp, err := retryGitHubCall(ctx, deps, "search_code", func(callCtx context.Context) (*github.CodeSearchResult, *github.Response, error) { + return client.Search.Code(callCtx, query, opts) + }) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, fmt.Sprintf("failed to search code with query '%s'", query), @@ -278,11 +280,26 @@ func SearchCode(t translations.TranslationHelperFunc) inventory.ServerTool { minimalItems := make([]MinimalCodeResult, 0, len(result.CodeResults)) for _, code := range result.CodeResults { + var textMatches []*github.TextMatch + if len(code.TextMatches) > 0 { + textMatches = make([]*github.TextMatch, 0, len(code.TextMatches)) + for _, match := range code.TextMatches { + if match == nil { + continue + } + cloned := *match + fragment, redacted := redactSecretLikeContent(match.GetFragment()) + if redacted { + cloned.Fragment = github.Ptr(fragment) + } + textMatches = append(textMatches, &cloned) + } + } item := MinimalCodeResult{ Name: code.GetName(), Path: code.GetPath(), SHA: code.GetSHA(), - TextMatches: code.TextMatches, + TextMatches: textMatches, } if code.Repository != nil { item.Repository = code.Repository.GetFullName() From 0c09fd6f7de035eef2e91a84a3f1c18d9051f590 Mon Sep 17 00:00:00 2001 From: Hermes Bot Date: Sun, 12 Jul 2026 06:51:29 +0000 Subject: [PATCH 5/5] test: verify atomic multi-file patch commits --- pkg/github/repositories.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index f9dc73d296..e26ea2ff8d 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -2403,6 +2403,9 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { if resp != nil && resp.Body != nil { defer func() { _ = resp.Body.Close() }() } + if dryRun && (repositoryIsEmpty || branchNotFound) { + return MarshalledTextResult(map[string]any{"branch": branch, "changed_paths": []string{}, "file_count": len(filesObj), "patch_previews": []any{}, "dry_run": true, "branch_exists": false}), nil, nil + } var baseCommit *github.Commit if !repositoryIsEmpty { @@ -2450,6 +2453,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { changedPaths := make([]string, 0, len(filesObj)) patchPreviews := make([]any, 0) + expectedAfter := make(map[string]string) for _, file := range filesObj { fileMap, ok := file.(map[string]any) if !ok { @@ -2504,6 +2508,7 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(fmt.Sprintf("%s: unrelated content was not preserved", path)), nil, nil } content = nextContent + expectedAfter[path] = content patchPreviews = append(patchPreviews, map[string]any{"path": path, "operation": operation, "expected_blob_sha": expectedSHA, "matched_occurrences": patchPreview.MatchedOccurrences, "hunks": patchPreview.Hunks, "before_preview": boundedRedactedPreview(currentContent), "after_preview": boundedRedactedPreview(nextContent)}) } if isSecretLikeRepoPath(path) { @@ -2577,6 +2582,19 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { ), nil, nil } defer func() { _ = resp.Body.Close() }() + mismatches := make([]any, 0) + for path, expected := range expectedAfter { + _, actual, verifyResp, verifyErr := getCurrentGitHubFile(ctx, client, owner, repo, path, branch) + if verifyResp != nil && verifyResp.Body != nil { + _ = verifyResp.Body.Close() + } + if verifyErr != nil || actual != expected { + mismatches = append(mismatches, map[string]any{"path": path, "requested": "exact_content", "actual": "mismatch"}) + } + } + if len(mismatches) > 0 { + return utils.NewToolResultError("post-commit verification failed for one or more patched files"), nil, nil + } r, err := json.Marshal(updatedRef) if err != nil {