diff --git a/apps/open-swe/src/__tests__/git-file-validation.test.ts b/apps/open-swe/src/__tests__/git-file-validation.test.ts new file mode 100644 index 00000000..a53e5e8a --- /dev/null +++ b/apps/open-swe/src/__tests__/git-file-validation.test.ts @@ -0,0 +1,341 @@ +import { describe, it, expect } from "@jest/globals"; +import { + shouldExcludeFile, + parseGitStatusOutput, +} from "../utils/github/git.js"; +import { DEFAULT_EXCLUDED_PATTERNS } from "../utils/github/constants.js"; + +describe("Git File Validation", () => { + describe("Realistic git status scenarios", () => { + it("should handle typical development workspace changes", () => { + const gitStatusOutput = ` M apps/open-swe/src/utils/github/git.ts +?? apps/open-swe/src/__tests__/git-file-validation.test.ts + M package.json +?? node_modules/.cache/package-lock.json + D old-config.json +?? dist/bundle.js +?? .env.local +?? logs/error.log +?? .DS_Store + M README.md +?? temp-backup.txt`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(validFiles).toEqual([ + "apps/open-swe/src/utils/github/git.ts", + "apps/open-swe/src/__tests__/git-file-validation.test.ts", + "package.json", + "old-config.json", + "README.md", + "temp-backup.txt", + ]); + + expect(excludedFiles).toEqual([ + "node_modules/.cache/package-lock.json", + "dist/bundle.js", + ".env.local", + "logs/error.log", + ".DS_Store", + ]); + }); + + it("should handle file moves and renames", () => { + // Git status with file moves (R) and renames + const gitStatusOutput = `R src/old-file.ts -> src/new-file.ts + M src/components/Button.tsx +?? node_modules/react/index.js +?? dist/assets/main.css +?? .env.production +?? logs/debug.log + M package.json +?? .DS_Store`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(validFiles).toEqual([ + "src/old-file.ts -> src/new-file.ts", + "src/components/Button.tsx", + "package.json", + ]); + + expect(excludedFiles).toEqual([ + "node_modules/react/index.js", + "dist/assets/main.css", + ".env.production", + "logs/debug.log", + ".DS_Store", + ]); + }); + + it("should handle nested directory structures", () => { + // Complex nested directory structure + const gitStatusOutput = ` M apps/web/src/components/ui/button.tsx +?? apps/web/node_modules/react/index.js +?? apps/web/dist/assets/main.css +?? apps/open-swe/src/langgraph_api/server.py +?? apps/open-swe/.env.development +?? packages/shared/src/utils.ts +?? .turbo/cache/file +?? coverage/lcov.info +?? logs/app.log +?? .DS_Store`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(validFiles).toEqual([ + "apps/web/src/components/ui/button.tsx", + "packages/shared/src/utils.ts", + ]); + + expect(excludedFiles).toEqual([ + "apps/web/node_modules/react/index.js", + "apps/web/dist/assets/main.css", + "apps/open-swe/src/langgraph_api/server.py", + "apps/open-swe/.env.development", + ".turbo/cache/file", + "coverage/lcov.info", + "logs/app.log", + ".DS_Store", + ]); + }); + + it("should handle Windows-style paths", () => { + // Git status with Windows backslashes + const gitStatusOutput = ` M src\\components\\Button.tsx +?? node_modules\\react\\index.js +?? dist\\bundle.js +?? .env.local +?? logs\\error.log + M package.json +?? .DS_Store`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(validFiles).toEqual([ + "src\\components\\Button.tsx", + "package.json", + ]); + + expect(excludedFiles).toEqual([ + "node_modules\\react\\index.js", + "dist\\bundle.js", + ".env.local", + "logs\\error.log", + ".DS_Store", + ]); + }); + + it("should handle empty git status", () => { + const gitStatusOutput = ""; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(allFiles).toEqual([]); + expect(validFiles).toEqual([]); + expect(excludedFiles).toEqual([]); + }); + + it("should handle git status with only whitespace and empty lines", () => { + const gitStatusOutput = ` + + `; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(allFiles).toEqual([]); + expect(validFiles).toEqual([]); + expect(excludedFiles).toEqual([]); + }); + }); + + describe("All git status indicators", () => { + it("should handle all possible git status indicators", () => { + const gitStatusOutput = ` M modified-file.txt +M staged-modified.txt +A new-file.txt + D deleted-file.txt +D staged-deleted.txt +R old-file.txt -> new-file.txt +C copied-file.txt +U unmerged-file.txt +?? untracked-file.txt +!! ignored-file.txt + T type-changed.txt +T staged-type-changed.txt`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + expect(allFiles).toEqual([ + "modified-file.txt", + "staged-modified.txt", + "new-file.txt", + "deleted-file.txt", + "staged-deleted.txt", + "old-file.txt -> new-file.txt", + "copied-file.txt", + "unmerged-file.txt", + "untracked-file.txt", + "ignored-file.txt", + "type-changed.txt", + "staged-type-changed.txt", + ]); + }); + }); + + describe("File names with special characters", () => { + it("should handle files with spaces in names", () => { + const gitStatusOutput = ` M "file with spaces.txt" +?? "another file with spaces.md" +?? node_modules/"package with spaces"`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + const excludedFiles = allFiles.filter((file) => + shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + + expect(validFiles).toEqual([ + '"file with spaces.txt"', + '"another file with spaces.md"', + ]); + + expect(excludedFiles).toEqual(['node_modules/"package with spaces"']); + }); + + it("should handle files with special characters", () => { + const gitStatusOutput = ` M file-with-dashes.txt +?? file_with_underscores.md +?? file.with.dots.js +?? file@symbol.com +?? file#hash.txt +?? file$dollar.txt +?? file%percent.txt +?? file^caret.txt +?? file&ersand.txt +?? file*asterisk.txt +?? file(open).txt +?? file)close.txt +?? file[open].txt +?? file]close.txt +?? file{open}.txt +?? file}close.txt +?? file|pipe.txt +?? file\\backslash.txt +?? file"quote.txt +?? file'apostrophe.txt +?? file;semicolon.txt +?? file,comma.txt +?? filegreater.txt +?? file=equals.txt +?? file+plus.txt +?? file~tilde.txt`; + + const allFiles = parseGitStatusOutput(gitStatusOutput); + // All should be valid files (no exclusions) + const validFiles = allFiles.filter( + (file) => !shouldExcludeFile(file, DEFAULT_EXCLUDED_PATTERNS), + ); + expect(validFiles).toEqual(allFiles); + }); + }); + + describe("Edge cases and security", () => { + it("should handle patterns with regex metacharacters safely", () => { + const dangerousPatterns = ["*.log[", "*.(log|txt)", "temp*", "*cache*"]; + const testFiles = [ + "error.log[", + "test.(log|txt)", + "temp.cache", + "mycache.file", + ]; + + testFiles.forEach((file) => { + const result = shouldExcludeFile(file, dangerousPatterns); + if (file === "error.log[") { + expect(result).toBe(true); + } else if (file === "test.(log|txt)") { + expect(result).toBe(true); + } else if (file === "temp.cache") { + expect(result).toBe(true); + } else if (file === "mycache.file") { + expect(result).toBe(true); + } + }); + }); + + it("should handle very long file paths", () => { + const longPath = "a".repeat(1000) + "/very/deep/nested/path/to/file.ts"; + const result = shouldExcludeFile(longPath, DEFAULT_EXCLUDED_PATTERNS); + expect(result).toBe(false); + }); + + it("should handle unicode characters in paths", () => { + const unicodePath = "src/测试/文件.ts"; + const result = shouldExcludeFile(unicodePath, DEFAULT_EXCLUDED_PATTERNS); + expect(result).toBe(false); + }); + + it("should handle empty and whitespace-only lines", () => { + const gitStatusOutput = ` + + `; + const allFiles = parseGitStatusOutput(gitStatusOutput); + expect(allFiles).toEqual([]); + }); + + it("should handle multiple consecutive spaces", () => { + const gitStatusOutput = ` M file-with-many-spaces.txt`; + const allFiles = parseGitStatusOutput(gitStatusOutput); + expect(allFiles).toEqual([" file-with-many-spaces.txt"]); + }); + + it("should handle files with leading/trailing spaces", () => { + const gitStatusOutput = ` M " file-with-leading-space.txt" + M "file-with-trailing-space.txt "`; + const allFiles = parseGitStatusOutput(gitStatusOutput); + expect(allFiles).toEqual([ + ' " file-with-leading-space.txt"', + ' "file-with-trailing-space.txt "', + ]); + }); + }); +}); diff --git a/apps/open-swe/src/utils/github/constants.ts b/apps/open-swe/src/utils/github/constants.ts new file mode 100644 index 00000000..c95eb9e7 --- /dev/null +++ b/apps/open-swe/src/utils/github/constants.ts @@ -0,0 +1,19 @@ +export const DEFAULT_EXCLUDED_PATTERNS = [ + "node_modules", + "langgraph_api", + ".env", + ".env.local", + ".env.production", + ".env.development", + "dist", + "build", + ".turbo", + ".next", + "coverage", + ".nyc_output", + "logs", + "*.log", + ".DS_Store", + "Thumbs.db", + "*.backup", +]; diff --git a/apps/open-swe/src/utils/github/git.ts b/apps/open-swe/src/utils/github/git.ts index 58d675bf..685851ad 100644 --- a/apps/open-swe/src/utils/github/git.ts +++ b/apps/open-swe/src/utils/github/git.ts @@ -17,9 +17,93 @@ import { } from "@open-swe/shared/open-swe/tasks"; import { createPullRequest } from "./api.js"; import { addTaskPlanToIssue } from "./issue-task.js"; +import { DEFAULT_EXCLUDED_PATTERNS } from "./constants.js"; +import { escapeRegExp } from "../string-utils.js"; const logger = createLogger(LogLevel.INFO, "GitHub-Git"); +/** + * Parses git status output and returns an array of file paths. + * Removes the git status indicators (first 3 characters) from each line. + */ +export function parseGitStatusOutput(gitStatusOutput: string): string[] { + return gitStatusOutput + .split("\n") + .filter((line) => line.trim() !== "") + .map((line) => line.substring(3)) + .filter(Boolean); +} + +/** + * Validates and filters files before git add operation. + * Excludes files/directories that should not be committed. + */ +async function getValidFilesToCommit( + absoluteRepoDir: string, + sandbox: Sandbox, + excludePatterns: string[] = DEFAULT_EXCLUDED_PATTERNS, +): Promise { + const gitStatusOutput = await sandbox.process.executeCommand( + "git status --porcelain", + absoluteRepoDir, + undefined, + TIMEOUT_SEC, + ); + + if (gitStatusOutput.exitCode !== 0) { + logger.error(`Failed to get git status for file validation`, { + gitStatusOutput, + }); + throw new Error("Failed to get git status for file validation"); + } + + const allFiles = parseGitStatusOutput(gitStatusOutput.result); + + const validFiles = allFiles.filter((filePath) => { + return !shouldExcludeFile(filePath, excludePatterns); + }); + + const excludedFiles = allFiles.filter((filePath) => { + return shouldExcludeFile(filePath, excludePatterns); + }); + + if (excludedFiles.length > 0) { + logger.info(`Excluded ${excludedFiles.length} files from commit:`, { + excludedFiles: excludedFiles, + }); + } + + return validFiles; +} + +/** + * Checks if a file should be excluded from commits based on patterns. + */ +export function shouldExcludeFile( + filePath: string, + excludePatterns: string[], +): boolean { + const normalizedPath = filePath.replace(/\\/g, "/"); + + return excludePatterns.some((pattern) => { + if (pattern.includes("*")) { + const escapedPattern = escapeRegExp(pattern); + const regexPattern = escapedPattern.replace(/\\\*/g, ".*"); + const regex = new RegExp( + `^${regexPattern}$|/${regexPattern}$|^${regexPattern}/|/${regexPattern}/`, + ); + return regex.test(normalizedPath); + } + + return ( + normalizedPath === pattern || + normalizedPath.startsWith(pattern + "/") || + normalizedPath.includes("/" + pattern + "/") || + normalizedPath.endsWith("/" + pattern) + ); + }); +} + export function getBranchName(configOrThreadId: GraphConfig | string): string { const threadId = typeof configOrThreadId === "string" @@ -50,10 +134,7 @@ export async function getChangedFilesStatus( return []; } - return gitStatusOutput.result - .split("\n") - .map((line) => line.trim()) - .filter((line) => line !== ""); + return parseGitStatusOutput(gitStatusOutput.result); } export async function stashAndClearChanges( @@ -103,8 +184,17 @@ export async function checkoutBranchAndCommit( const branchName = options.branchName || getBranchName(config); logger.info(`Committing changes to branch ${branchName}`); - // Commit the changes. We can use the sandbox executeCommand API for this since it doesn't require a token. - await sandbox.git.add(absoluteRepoDir, ["."]); + + // Validate and filter files before committing + const validFiles = await getValidFilesToCommit(absoluteRepoDir, sandbox); + + if (validFiles.length === 0) { + logger.info("No valid files to commit after filtering"); + return { branchName, updatedTaskPlan: options.taskPlan }; + } + + // Add only validated files instead of adding all files with "." + await sandbox.git.add(absoluteRepoDir, validFiles); const botAppName = process.env.GITHUB_APP_NAME; if (!botAppName) { diff --git a/apps/open-swe/src/utils/string-utils.ts b/apps/open-swe/src/utils/string-utils.ts new file mode 100644 index 00000000..b29e9dab --- /dev/null +++ b/apps/open-swe/src/utils/string-utils.ts @@ -0,0 +1,15 @@ +/** + * Escapes regex metacharacters in a string to treat them as literal characters. + * Useful for safely converting user input into regex patterns. + * + * @param string - The string to escape + * @returns The escaped string safe for use in RegExp constructor + * + * @example + * escapeRegExp("hello.world") // "hello\\.world" + * escapeRegExp("test*file") // "test\\*file" + * escapeRegExp("path[0]") // "path\\[0\\]" + */ +export function escapeRegExp(string: string): string { + return string.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +}