From b02bf485a4767b59757f18f6e0e0bffa3231c131 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 27 Oct 2025 12:13:17 -0500 Subject: [PATCH 1/5] fix: handle nested git repos gracefully in checkpoints Instead of blocking checkpoints when nested git repos are detected, now excludes them from staging to prevent submodule-related errors. Changes: - Removed initialization-time nested repo check that blocked feature - Added findNestedRepos() to detect repos from multiple sources: * .gitmodules (declared submodules) * git index (gitlinks with mode 160000) * filesystem (.git directories and worktrees) - Modified stageAll() to exclude nested repos using pathspec - Added safety check to prevent submodule changes in staging - Updated tests to verify nested repos are excluded from checkpoints This allows checkpoints to work in monorepos and projects with submodules while preventing git errors from nested repo changes. --- .../checkpoints/ShadowCheckpointService.ts | 158 ++++++++++++------ .../__tests__/ShadowCheckpointService.spec.ts | 47 +++++- 2 files changed, 142 insertions(+), 63 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index cdc6bd8a695..3a3d9be43e2 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -10,7 +10,6 @@ import * as vscode from "vscode" import { fileExistsAtPath } from "../../utils/fs" import { executeRipgrep } from "../../services/search/file-search" -import { t } from "../../i18n" import { CheckpointDiff, CheckpointResult, CheckpointEventMap } from "./types" import { getExcludePatterns } from "./excludes" @@ -70,20 +69,6 @@ export abstract class ShadowCheckpointService extends EventEmitter { throw new Error("Shadow git repo already initialized") } - const nestedGitPath = await this.getNestedGitRepository() - - if (nestedGitPath) { - // Show persistent error message with the offending path - const relativePath = path.relative(this.workspaceDir, nestedGitPath) - const message = t("common:errors.nested_git_repos_warning", { path: relativePath }) - vscode.window.showErrorMessage(message) - - throw new Error( - `Checkpoints are disabled because a nested git repository was detected at: ${relativePath}. ` + - "Please remove or relocate nested git repositories to use the checkpoints feature.", - ) - } - await fs.mkdir(this.checkpointsDir, { recursive: true }) const git = simpleGit(this.checkpointsDir) const gitVersion = await git.version() @@ -152,63 +137,126 @@ export abstract class ShadowCheckpointService extends EventEmitter { private async stageAll(git: SimpleGit) { try { - await git.add([".", "--ignore-errors"]) + // Find all nested repos to exclude + const nestedRepos = await this.findNestedRepos(git) + + if (nestedRepos.length > 0) { + this.log( + `[${this.constructor.name}#stageAll] excluding ${nestedRepos.length} nested repos: ${nestedRepos.join(", ")}`, + ) + } + + // Build add command with pathspec excludes + const addArgs: string[] = ["-A", ":/"] + for (const repoPath of nestedRepos) { + addArgs.push(`:(exclude)${repoPath}/`) + } + + // Stage files + await git.add(addArgs) + + // Safety check: ensure no submodule changes staged + const diffSummary = await git.raw(["diff", "--cached", "--summary"]) + if (diffSummary.includes("Submodule")) { + throw new Error("Submodule changes detected in staging area - this should not happen") + } } catch (error) { this.log( `[${this.constructor.name}#stageAll] failed to add files to git: ${error instanceof Error ? error.message : String(error)}`, ) + throw error } } - private async getNestedGitRepository(): Promise { - try { - // Find all .git/HEAD files that are not at the root level. - const args = ["--files", "--hidden", "--follow", "-g", "**/.git/HEAD", this.workspaceDir] - - const gitPaths = await executeRipgrep({ args, workspacePath: this.workspaceDir }) - - // Filter to only include nested git directories (not the root .git). - // Since we're searching for HEAD files, we expect type to be "file" - const nestedGitPaths = gitPaths.filter(({ type, path: filePath }) => { - // Check if it's a file and is a nested .git/HEAD (not at root) - if (type !== "file") return false - - // Ensure it's a .git/HEAD file and not the root one - const normalizedPath = filePath.replace(/\\/g, "/") - return ( - normalizedPath.includes(".git/HEAD") && - !normalizedPath.startsWith(".git/") && - normalizedPath !== ".git/HEAD" - ) - }) + private async findNestedRepos(git: SimpleGit): Promise { + const nestedRepos = new Set() - if (nestedGitPaths.length > 0) { - // Get the first nested git repository path - // Remove .git/HEAD from the path to get the repository directory - const headPath = nestedGitPaths[0].path + // 1. From .gitmodules (declared submodules) + try { + const config = await git.raw(["config", "-f", ".gitmodules", "--get-regexp", "^submodule\\..*\\.path$"]) + for (const line of config.split("\n")) { + const match = line.match(/submodule\..*\.path\s+(.+)/) + if (match) nestedRepos.add(match[1]) + } + } catch { + // No .gitmodules file or error reading it + } - // Use path module to properly extract the repository directory - // The HEAD file is at .git/HEAD, so we need to go up two directories - const gitDir = path.dirname(headPath) // removes HEAD, gives us .git - const repoDir = path.dirname(gitDir) // removes .git, gives us the repo directory + // 2. From index (gitlinks with mode 160000) + try { + const lsFiles = await git.raw(["ls-files", "-s"]) + for (const line of lsFiles.split("\n")) { + if (line.startsWith("160000")) { + const parts = line.split(/\s+/) + if (parts[3]) nestedRepos.add(parts[3]) + } + } + } catch { + // Ignore errors + } - const absolutePath = path.join(this.workspaceDir, repoDir) + // 3. From filesystem (any nested .git directory or worktree) + try { + const gitDirs = await executeRipgrep({ + args: ["--files", "--hidden", "--follow", "-g", "**/.git/HEAD", this.workspaceDir], + workspacePath: this.workspaceDir, + }) - this.log( - `[${this.constructor.name}#getNestedGitRepository] found ${nestedGitPaths.length} nested git repositories, first at: ${repoDir}`, - ) - return absolutePath + for (const result of gitDirs) { + if (result.type === "file") { + const normalizedPath = result.path.replace(/\\/g, "/") + if ( + normalizedPath.includes(".git/HEAD") && + !normalizedPath.startsWith(".git/") && + normalizedPath !== ".git/HEAD" + ) { + // Extract repo directory (remove .git/HEAD) + const gitDir = path.dirname(result.path) + const repoDir = path.dirname(gitDir) + nestedRepos.add(repoDir) + } + } } - - return null } catch (error) { this.log( - `[${this.constructor.name}#getNestedGitRepository] failed to check for nested git repos: ${error instanceof Error ? error.message : String(error)}`, + `[${this.constructor.name}#findNestedRepos] failed to search filesystem: ${error instanceof Error ? error.message : String(error)}`, ) + } - // If we can't check, assume there are no nested repos to avoid blocking the feature. - return null + // 4. From filesystem (git worktrees - .git files pointing to worktree) + try { + const gitFiles = await executeRipgrep({ + args: ["--files", "--hidden", "--follow", "-g", "**/.git", this.workspaceDir], + workspacePath: this.workspaceDir, + }) + + for (const result of gitFiles) { + if (result.type === "file") { + const normalizedPath = result.path.replace(/\\/g, "/") + // Check if this is a .git file (not directory) and not the root + if (normalizedPath.endsWith("/.git") && normalizedPath !== ".git") { + try { + // Read the .git file to check if it's a worktree + const gitFilePath = path.join(this.workspaceDir, result.path) + const content = await fs.readFile(gitFilePath, "utf8") + if (content.trim().startsWith("gitdir:")) { + // This is a worktree - exclude its directory + const repoDir = path.dirname(result.path) + nestedRepos.add(repoDir) + } + } catch { + // Ignore errors reading .git file + } + } + } + } + } catch (error) { + this.log( + `[${this.constructor.name}#findNestedRepos] failed to search for worktrees: ${error instanceof Error ? error.message : String(error)}`, + ) } + + return Array.from(nestedRepos).filter((p) => p && p !== ".") } private async getShadowGitConfigWorktree(git: SimpleGit) { diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 622a90f39ab..4941cadfe57 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -378,8 +378,8 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) }) - describe(`${klass.name}#hasNestedGitRepositories`, () => { - it("throws error when nested git repositories are detected during initialization", async () => { + describe(`${klass.name}#nestedGitRepositories`, () => { + it("succeeds when nested git repositories are detected and excludes them from checkpoints", async () => { // Create a new temporary workspace and service for this test. const shadowDir = path.join(tmpDir, `${prefix}-nested-git-${Date.now()}`) const workspaceDir = path.join(tmpDir, `workspace-nested-git-${Date.now()}`) @@ -435,13 +435,33 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( } }) - const service = new klass(taskId, shadowDir, workspaceDir, () => {}) + const logMessages: string[] = [] + const service = new klass(taskId, shadowDir, workspaceDir, (msg: string) => logMessages.push(msg)) - // Verify that initialization throws an error when nested git repos are detected - // The error message now includes the specific path of the nested repository - await expect(service.initShadowGit()).rejects.toThrowError( - /Checkpoints are disabled because a nested git repository was detected at:/, - ) + // Verify that initialization succeeds even with nested git repos + await expect(service.initShadowGit()).resolves.not.toThrow() + expect(service.isInitialized).toBe(true) + + // Modify files in both main workspace and nested repo + await fs.writeFile(mainFile, "Modified content in main repo") + await fs.writeFile(nestedFile, "Modified content in nested repo") + + // Save a checkpoint + const checkpoint = await service.saveCheckpoint("Test with nested repos") + expect(checkpoint?.commit).toBeTruthy() + + // Verify that only the main file was included in the checkpoint + const diff = await service.getDiff({ to: checkpoint!.commit }) + const mainFileChange = diff.find((change) => change.paths.relative === "main-file.txt") + const nestedFileChange = diff.find((change) => change.paths.relative.includes("nested-file.txt")) + + expect(mainFileChange).toBeDefined() + expect(mainFileChange?.content.after).toBe("Modified content in main repo") + expect(nestedFileChange).toBeUndefined() // Nested repo changes should be excluded + + // Verify that the log includes information about excluding nested repos + const excludeLog = logMessages.find((msg) => msg.includes("excluding") && msg.includes("nested repos")) + expect(excludeLog).toBeDefined() // Clean up. vitest.restoreAllMocks() @@ -478,6 +498,17 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await expect(service.initShadowGit()).resolves.not.toThrow() expect(service.isInitialized).toBe(true) + // Modify the main file and save a checkpoint + await fs.writeFile(mainFile, "Modified content") + const checkpoint = await service.saveCheckpoint("Test without nested repos") + expect(checkpoint?.commit).toBeTruthy() + + // Verify the change was included in the checkpoint + const diff = await service.getDiff({ to: checkpoint!.commit }) + expect(diff).toHaveLength(1) + expect(diff[0].paths.relative).toBe("main-file.txt") + expect(diff[0].content.after).toBe("Modified content") + // Clean up. vitest.restoreAllMocks() await fs.rm(shadowDir, { recursive: true, force: true }) From 0d089b193c328da2250c7611d549a3cc8b846228 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 27 Oct 2025 12:19:13 -0500 Subject: [PATCH 2/5] fix: address review feedback on nested git repo handling Improvements based on reviewer bot feedback: 1. Enhanced safety check (HIGH priority): - Now uses 'git ls-files -s --cached' to detect mode 160000 entries - Catches newly added/removed gitlinks, not just pointer changes - Added check for .gitmodules changes with warning log 2. Improved pathspec exclusion (MEDIUM priority): - Added 'git rm --cached' to remove existing gitlinks before staging - Prevents staging of gitlink entries already in the index - Ensures complete exclusion of nested repo changes 3. Better error logging (LOW priority): - Added warning logs for git command failures in findNestedRepos() - Helps debugging while avoiding feature breakage - Distinguishes expected errors (no .gitmodules) from real issues All tests continue to pass with these enhancements. --- .../checkpoints/ShadowCheckpointService.ts | 54 +++++++++++++++---- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 3a3d9be43e2..be9985a5930 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -146,6 +146,17 @@ export abstract class ShadowCheckpointService extends EventEmitter { ) } + // Remove any existing gitlinks from the index before staging + for (const repoPath of nestedRepos) { + try { + await git.raw(["rm", "--cached", "--ignore-unmatch", "-r", repoPath]) + } catch (error) { + this.log( + `[${this.constructor.name}#stageAll] failed to remove cached gitlink ${repoPath}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + // Build add command with pathspec excludes const addArgs: string[] = ["-A", ":/"] for (const repoPath of nestedRepos) { @@ -155,10 +166,24 @@ export abstract class ShadowCheckpointService extends EventEmitter { // Stage files await git.add(addArgs) - // Safety check: ensure no submodule changes staged - const diffSummary = await git.raw(["diff", "--cached", "--summary"]) - if (diffSummary.includes("Submodule")) { - throw new Error("Submodule changes detected in staging area - this should not happen") + // Enhanced safety check: verify no mode 160000 entries in staging area + const stagedFiles = await git.raw(["ls-files", "-s", "--cached"]) + const gitlinkEntries = stagedFiles + .split("\n") + .filter((line) => line.startsWith("160000")) + .map((line) => line.split(/\s+/)[3]) + .filter(Boolean) + + if (gitlinkEntries.length > 0) { + throw new Error( + `Gitlink entries detected in staging area: ${gitlinkEntries.join(", ")} - this should not happen`, + ) + } + + // Additional check for .gitmodules changes + const diffSummary = await git.raw(["diff", "--cached", "--name-only"]) + if (diffSummary.includes(".gitmodules")) { + this.log(`[${this.constructor.name}#stageAll] warning: .gitmodules changes detected in staging area`) } } catch (error) { this.log( @@ -178,8 +203,13 @@ export abstract class ShadowCheckpointService extends EventEmitter { const match = line.match(/submodule\..*\.path\s+(.+)/) if (match) nestedRepos.add(match[1]) } - } catch { - // No .gitmodules file or error reading it + } catch (error) { + // No .gitmodules file is expected in most cases, only log if it's a real error + if (error instanceof Error && !error.message.includes("exit code 1")) { + this.log( + `[${this.constructor.name}#findNestedRepos] warning: failed to read .gitmodules: ${error.message}`, + ) + } } // 2. From index (gitlinks with mode 160000) @@ -191,8 +221,10 @@ export abstract class ShadowCheckpointService extends EventEmitter { if (parts[3]) nestedRepos.add(parts[3]) } } - } catch { - // Ignore errors + } catch (error) { + this.log( + `[${this.constructor.name}#findNestedRepos] warning: failed to list files from index: ${error instanceof Error ? error.message : String(error)}`, + ) } // 3. From filesystem (any nested .git directory or worktree) @@ -244,8 +276,10 @@ export abstract class ShadowCheckpointService extends EventEmitter { const repoDir = path.dirname(result.path) nestedRepos.add(repoDir) } - } catch { - // Ignore errors reading .git file + } catch (error) { + this.log( + `[${this.constructor.name}#findNestedRepos] warning: failed to read .git file at ${result.path}: ${error instanceof Error ? error.message : String(error)}`, + ) } } } From fd22d0f080ad80ace522dedfec1af0f59c453522 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 6 Nov 2025 18:14:29 -0500 Subject: [PATCH 3/5] checkpoints: normalize nested repo paths to POSIX and use workspace .gitmodules absolute path; add tests; verify no gitlinks staged and log .gitmodules changes --- .../checkpoints/ShadowCheckpointService.ts | 25 +- .../__tests__/ShadowCheckpointService.spec.ts | 287 ++++++++++++++++++ 2 files changed, 306 insertions(+), 6 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index be9985a5930..41bdad9f362 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -149,7 +149,9 @@ export abstract class ShadowCheckpointService extends EventEmitter { // Remove any existing gitlinks from the index before staging for (const repoPath of nestedRepos) { try { - await git.raw(["rm", "--cached", "--ignore-unmatch", "-r", repoPath]) + // Normalize to POSIX for git pathspec compatibility + const posixPath = repoPath.replace(/\\/g, "/") + await git.raw(["rm", "--cached", "--ignore-unmatch", "-r", posixPath]) } catch (error) { this.log( `[${this.constructor.name}#stageAll] failed to remove cached gitlink ${repoPath}: ${error instanceof Error ? error.message : String(error)}`, @@ -160,7 +162,9 @@ export abstract class ShadowCheckpointService extends EventEmitter { // Build add command with pathspec excludes const addArgs: string[] = ["-A", ":/"] for (const repoPath of nestedRepos) { - addArgs.push(`:(exclude)${repoPath}/`) + // Normalize to POSIX for git pathspec compatibility + const posixPath = repoPath.replace(/\\/g, "/") + addArgs.push(`:(exclude)${posixPath}/`) } // Stage files @@ -198,10 +202,15 @@ export abstract class ShadowCheckpointService extends EventEmitter { // 1. From .gitmodules (declared submodules) try { - const config = await git.raw(["config", "-f", ".gitmodules", "--get-regexp", "^submodule\\..*\\.path$"]) + const modulesPath = path.join(this.workspaceDir, ".gitmodules") + const config = await git.raw(["config", "-f", modulesPath, "--get-regexp", "^submodule\\..*\\.path$"]) for (const line of config.split("\n")) { const match = line.match(/submodule\..*\.path\s+(.+)/) - if (match) nestedRepos.add(match[1]) + if (match) { + // Normalize paths to POSIX format for git pathspec compatibility + const normalizedPath = match[1].replace(/\\/g, "/") + nestedRepos.add(normalizedPath) + } } } catch (error) { // No .gitmodules file is expected in most cases, only log if it's a real error @@ -242,8 +251,10 @@ export abstract class ShadowCheckpointService extends EventEmitter { !normalizedPath.startsWith(".git/") && normalizedPath !== ".git/HEAD" ) { + // Normalize path first to handle Windows-style backslashes + const normalizedPath = result.path.replace(/\\/g, "/") // Extract repo directory (remove .git/HEAD) - const gitDir = path.dirname(result.path) + const gitDir = path.dirname(normalizedPath) const repoDir = path.dirname(gitDir) nestedRepos.add(repoDir) } @@ -273,7 +284,9 @@ export abstract class ShadowCheckpointService extends EventEmitter { const content = await fs.readFile(gitFilePath, "utf8") if (content.trim().startsWith("gitdir:")) { // This is a worktree - exclude its directory - const repoDir = path.dirname(result.path) + // Normalize path first to handle Windows-style backslashes + const normalizedPath = result.path.replace(/\\/g, "/") + const repoDir = path.dirname(normalizedPath) nestedRepos.add(repoDir) } } catch (error) { diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 4941cadfe57..9a434a74464 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -379,6 +379,166 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) describe(`${klass.name}#nestedGitRepositories`, () => { + it("uses absolute path for .gitmodules lookup and handles absent file gracefully", async () => { + // Create a new temporary workspace and service for this test + const shadowDir = path.join(tmpDir, `${prefix}-gitmodules-absolute-${Date.now()}`) + const workspaceDir = path.join(tmpDir, `workspace-gitmodules-absolute-${Date.now()}`) + + // Create workspace directory + await fs.mkdir(workspaceDir, { recursive: true }) + + // Create a primary workspace repo + const mainGit = simpleGit(workspaceDir) + await mainGit.init() + await mainGit.addConfig("user.name", "Roo Code") + await mainGit.addConfig("user.email", "support@roocode.com") + + // Create a test file in the main workspace + const mainFile = path.join(workspaceDir, "main-file.txt") + await fs.writeFile(mainFile, "Content in main repo") + await mainGit.add(".") + await mainGit.commit("Initial commit") + + // Create a .gitmodules file with submodule paths + const gitmodulesPath = path.join(workspaceDir, ".gitmodules") + await fs.writeFile( + gitmodulesPath, + `[submodule "test-submodule"] + path = test-submodule + url = https://github.com/example/test-submodule.git`, + ) + + // Add .gitmodules to the repo + await mainGit.add(".gitmodules") + await mainGit.commit("Add gitmodules") + + const logMessages: string[] = [] + const service = new klass(taskId, shadowDir, workspaceDir, (msg: string) => logMessages.push(msg)) + + // Initialize the service + await service.initShadowGit() + + // Create nested directories to simulate submodule structure + const testSubmoduleDir = path.join(workspaceDir, "test-submodule") + await fs.mkdir(testSubmoduleDir, { recursive: true }) + const nestedFile = path.join(testSubmoduleDir, "nested-file.txt") + await fs.writeFile(nestedFile, "Content that should be excluded") + + // Modify main file and save a checkpoint to trigger findNestedRepos + await fs.writeFile(mainFile, "Modified content") + await service.saveCheckpoint("Test with gitmodules") + + // The service should have detected and excluded the submodule path + const excludeLog = logMessages.find((msg) => msg.includes("excluding") && msg.includes("nested repos")) + expect(excludeLog).toBeDefined() + expect(excludeLog).toContain("test-submodule") + + // Now test graceful handling when .gitmodules is missing + await fs.unlink(gitmodulesPath) + + // Modify file and save another checkpoint + await fs.writeFile(mainFile, "Modified content again") + + // This should not throw despite missing .gitmodules file + await expect(service.saveCheckpoint("Test missing gitmodules")).resolves.not.toThrow() + + // The error should be handled gracefully (exit code 1 is expected for missing file) + const gitmodulesErrors = logMessages.filter( + (msg) => msg.includes("failed to read .gitmodules") && !msg.includes("exit code 1"), + ) + expect(gitmodulesErrors).toHaveLength(0) // Should not log exit code 1 as an error + + // Clean up + await fs.rm(shadowDir, { recursive: true, force: true }) + await fs.rm(workspaceDir, { recursive: true, force: true }) + }) + + it("normalizes Windows-style backslash paths from .gitmodules to POSIX format", async () => { + // Create a new temporary workspace and service for this test + const shadowDir = path.join(tmpDir, `${prefix}-gitmodules-normalize-${Date.now()}`) + const workspaceDir = path.join(tmpDir, `workspace-gitmodules-normalize-${Date.now()}`) + + // Create workspace directory + await fs.mkdir(workspaceDir, { recursive: true }) + + // Create a primary workspace repo + const mainGit = simpleGit(workspaceDir) + await mainGit.init() + await mainGit.addConfig("user.name", "Roo Code") + await mainGit.addConfig("user.email", "support@roocode.com") + + // Create a test file in the main workspace + const mainFile = path.join(workspaceDir, "main-file.txt") + await fs.writeFile(mainFile, "Content in main repo") + await mainGit.add(".") + await mainGit.commit("Initial commit") + + // Simulate a .gitmodules file with Windows-style backslash paths by directly creating it + // and adding it to the git repo (to pass git config parsing) + const gitmodulesPath = path.join(workspaceDir, ".gitmodules") + await fs.writeFile( + gitmodulesPath, + `[submodule "nested\\\\project"] + path = nested\\\\project + url = https://github.com/example/nested-project.git +[submodule "sub\\\\folder\\\\repo"] + path = sub\\\\folder\\\\repo + url = https://github.com/example/sub-folder-repo.git`, + ) + + // Add .gitmodules to the repo + await mainGit.add(".gitmodules") + await mainGit.commit("Add gitmodules with backslash paths") + + // Create the nested directories to simulate the submodule structure + const nestedProjectDir = path.join(workspaceDir, "nested\\project") + const subFolderRepoDir = path.join(workspaceDir, "sub\\folder\\repo") + await fs.mkdir(nestedProjectDir, { recursive: true }) + await fs.mkdir(subFolderRepoDir, { recursive: true }) + + await fs.writeFile(path.join(nestedProjectDir, "file1.txt"), "Content that should be excluded") + await fs.writeFile(path.join(subFolderRepoDir, "file2.txt"), "Content that should be excluded") + + const logMessages: string[] = [] + const service = new klass(taskId, shadowDir, workspaceDir, (msg: string) => logMessages.push(msg)) + + // Initialize the service + await service.initShadowGit() + + // Create a test file and save a checkpoint to trigger path normalization + await fs.writeFile(mainFile, "Test content") + + // This will trigger findNestedRepos and path normalization + await service.saveCheckpoint("Test path normalization") + + // Verify that paths were normalized to POSIX format (forward slashes) + // The log should show normalized paths + const excludeLog = logMessages.find((msg) => msg.includes("excluding") && msg.includes("nested repos")) + expect(excludeLog).toBeDefined() + + // Check that the log contains POSIX paths (normalized from backslash paths) + // The exact format may vary based on path.join behavior, but should contain forward slashes + expect(excludeLog).toMatch(/nested[\\\/]project/) // Should contain the path, normalized or not + expect(excludeLog).toMatch(/sub[\\\/]folder[\\\/]repo/) // Should contain the path, normalized or not + + // Most importantly, verify no gitlink entries were created (which would happen if paths aren't normalized) + // This is the key safety check: ensuring Windows backslash paths don't break git pathspecs + const git = service["git"] // Access private git instance + expect(git).toBeDefined() // Ensure git is initialized + const stagedFiles = await git!.raw(["ls-files", "-s", "--cached"]) + const gitlinkEntries = stagedFiles + .split("\n") + .filter((line) => line.startsWith("160000")) + .map((line) => line.split(/\s+/)[3]) + .filter(Boolean) + + expect(gitlinkEntries).toHaveLength(0) // No gitlink entries should exist + + // Clean up + await fs.rm(shadowDir, { recursive: true, force: true }) + await fs.rm(workspaceDir, { recursive: true, force: true }) + }) + it("succeeds when nested git repositories are detected and excludes them from checkpoints", async () => { // Create a new temporary workspace and service for this test. const shadowDir = path.join(tmpDir, `${prefix}-nested-git-${Date.now()}`) @@ -469,6 +629,133 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await fs.rm(workspaceDir, { recursive: true, force: true }) }) + it("normalizes Windows-style backslash paths from filesystem scans to POSIX for git pathspecs", async () => { + // Create a new temporary workspace and service for this test. + const shadowDir = path.join(tmpDir, `${prefix}-windows-paths-${Date.now()}`) + const workspaceDir = path.join(tmpDir, `workspace-windows-paths-${Date.now()}`) + + // Create a primary workspace repo. + await fs.mkdir(workspaceDir, { recursive: true }) + const mainGit = simpleGit(workspaceDir) + await mainGit.init() + await mainGit.addConfig("user.name", "Roo Code") + await mainGit.addConfig("user.email", "support@roocode.com") + + // Create test file in main workspace + const mainFile = path.join(workspaceDir, "main-file.txt") + await fs.writeFile(mainFile, "Content in main repo") + await mainGit.add(".") + await mainGit.commit("Initial commit in main repo") + + // Mock executeRipgrep to simulate Windows-style backslash paths from filesystem + vitest.spyOn(fileSearch, "executeRipgrep").mockImplementation(({ args }) => { + const searchPattern = args[4] + + if (searchPattern.includes(".git/HEAD")) { + // Return Windows-style paths with backslashes to simulate filesystem scan on Windows + return Promise.resolve([ + { + path: "nested-project\\.git\\HEAD", // Backslash path from Windows filesystem + type: "file", + label: "HEAD", + }, + { + path: "another-repo\\subdir\\.git\\HEAD", // Another backslash path + type: "file", + label: "HEAD", + }, + ]) + } else if (searchPattern.includes("**/.git")) { + // Return Windows-style worktree .git file paths with backslashes + return Promise.resolve([ + { + path: "worktree-repo\\.git", // Backslash path from Windows filesystem + type: "file", + label: ".git", + }, + ]) + } else { + return Promise.resolve([]) + } + }) + + // Mock fs.readFile to simulate a worktree .git file for the worktree test + const originalReadFile = fs.readFile + vitest.spyOn(fs, "readFile").mockImplementation(async (filePath: any, encoding?: any) => { + const pathStr = typeof filePath === "string" ? filePath : filePath.toString() + if (pathStr.includes("worktree-repo") && pathStr.endsWith(".git")) { + return "gitdir: /some/worktree/path/.git/worktrees/worktree-repo" + } + return originalReadFile(filePath, encoding) + }) + + const logMessages: string[] = [] + const service = new klass(taskId, shadowDir, workspaceDir, (msg: string) => logMessages.push(msg)) + + // Initialize the service and verify it handles Windows paths correctly + await expect(service.initShadowGit()).resolves.not.toThrow() + expect(service.isInitialized).toBe(true) + + // Create files that would conflict with the "nested" repos if they weren't properly excluded + const nestedFile1 = path.join(workspaceDir, "nested-project", "nested-file.txt") + const nestedFile2 = path.join(workspaceDir, "another-repo", "subdir", "another-file.txt") + const worktreeFile = path.join(workspaceDir, "worktree-repo", "worktree-file.txt") + + await fs.mkdir(path.dirname(nestedFile1), { recursive: true }) + await fs.mkdir(path.dirname(nestedFile2), { recursive: true }) + await fs.mkdir(path.dirname(worktreeFile), { recursive: true }) + + await fs.writeFile(nestedFile1, "Content that should be excluded") + await fs.writeFile(nestedFile2, "Content that should be excluded") + await fs.writeFile(worktreeFile, "Content that should be excluded") + await fs.writeFile(mainFile, "Updated main content") + + // Save a checkpoint + const checkpoint = await service.saveCheckpoint("Test Windows path normalization") + expect(checkpoint?.commit).toBeTruthy() + + // Verify that the log shows POSIX-normalized paths for exclusion + const excludeLog = logMessages.find((msg) => msg.includes("excluding") && msg.includes("nested repos")) + expect(excludeLog).toBeDefined() + + // The log should contain POSIX paths (forward slashes), not Windows paths (backslashes) + expect(excludeLog).toContain("nested-project") + expect(excludeLog).toContain("another-repo/subdir") + expect(excludeLog).toContain("worktree-repo") + + // Verify that only the main file was included in the checkpoint + const diff = await service.getDiff({ to: checkpoint!.commit }) + const mainFileChange = diff.find((change) => change.paths.relative === "main-file.txt") + const nestedFileChanges = diff.filter( + (change) => + change.paths.relative.includes("nested-project") || + change.paths.relative.includes("another-repo") || + change.paths.relative.includes("worktree-repo"), + ) + + expect(mainFileChange).toBeDefined() + expect(mainFileChange?.content.after).toBe("Updated main content") + expect(nestedFileChanges).toHaveLength(0) // No nested repo files should be included + + // Enhanced safety check: Verify no mode 160000 gitlink entries were created + // This is the key test - ensuring Windows backslash paths don't break git pathspecs + const git = service["git"] // Access private git instance + expect(git).toBeDefined() // Ensure git is initialized + const stagedFiles = await git!.raw(["ls-files", "-s", "--cached"]) + const gitlinkEntries = stagedFiles + .split("\n") + .filter((line) => line.startsWith("160000")) + .map((line) => line.split(/\s+/)[3]) + .filter(Boolean) + + expect(gitlinkEntries).toHaveLength(0) // No gitlink entries should exist + + // Clean up. + vitest.restoreAllMocks() + await fs.rm(shadowDir, { recursive: true, force: true }) + await fs.rm(workspaceDir, { recursive: true, force: true }) + }) + it("succeeds when no nested git repositories are detected", async () => { // Create a new temporary workspace and service for this test. const shadowDir = path.join(tmpDir, `${prefix}-no-nested-git-${Date.now()}`) From 05fb2a3645cb555f76a25e8fd6f6b6d57f926268 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 6 Nov 2025 18:32:20 -0500 Subject: [PATCH 4/5] chore(checkpoints): remove redundant normalizedPath redeclarations --- src/services/checkpoints/ShadowCheckpointService.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 41bdad9f362..1d43d2877ae 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -251,8 +251,6 @@ export abstract class ShadowCheckpointService extends EventEmitter { !normalizedPath.startsWith(".git/") && normalizedPath !== ".git/HEAD" ) { - // Normalize path first to handle Windows-style backslashes - const normalizedPath = result.path.replace(/\\/g, "/") // Extract repo directory (remove .git/HEAD) const gitDir = path.dirname(normalizedPath) const repoDir = path.dirname(gitDir) @@ -284,8 +282,6 @@ export abstract class ShadowCheckpointService extends EventEmitter { const content = await fs.readFile(gitFilePath, "utf8") if (content.trim().startsWith("gitdir:")) { // This is a worktree - exclude its directory - // Normalize path first to handle Windows-style backslashes - const normalizedPath = result.path.replace(/\\/g, "/") const repoDir = path.dirname(normalizedPath) nestedRepos.add(repoDir) } From c98980d10b27223678cccda3835ffcea20d10ccb Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 6 Nov 2025 18:51:36 -0500 Subject: [PATCH 5/5] chore(checkpoints): correctly parse git ls-files entries with spaces (mode 160000) --- .../checkpoints/ShadowCheckpointService.ts | 15 +- .../__tests__/ShadowCheckpointService.spec.ts | 172 ++++++++++++++++++ 2 files changed, 184 insertions(+), 3 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 1d43d2877ae..e59ec0eeaa2 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -175,7 +175,12 @@ export abstract class ShadowCheckpointService extends EventEmitter { const gitlinkEntries = stagedFiles .split("\n") .filter((line) => line.startsWith("160000")) - .map((line) => line.split(/\s+/)[3]) + .map((line) => { + // Parse git ls-files output: + // Handle filenames with spaces by splitting only on first 3 whitespace groups + const match = line.match(/^(\S+)\s+(\S+)\s+(\S+)\s+(.+)$/) + return match ? match[4] : null + }) .filter(Boolean) if (gitlinkEntries.length > 0) { @@ -226,8 +231,12 @@ export abstract class ShadowCheckpointService extends EventEmitter { const lsFiles = await git.raw(["ls-files", "-s"]) for (const line of lsFiles.split("\n")) { if (line.startsWith("160000")) { - const parts = line.split(/\s+/) - if (parts[3]) nestedRepos.add(parts[3]) + // Parse git ls-files output: + // Handle filenames with spaces by splitting only on first 3 whitespace groups + const match = line.match(/^(\S+)\s+(\S+)\s+(\S+)\s+(.+)$/) + if (match && match[4]) { + nestedRepos.add(match[4]) + } } } } catch (error) { diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 9a434a74464..61ddee7029f 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -629,6 +629,178 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await fs.rm(workspaceDir, { recursive: true, force: true }) }) + it("correctly parses gitlink entries with spaces in paths", async () => { + // Create a new temporary workspace and service for this test + const shadowDir = path.join(tmpDir, `${prefix}-spaces-gitlinks-${Date.now()}`) + const workspaceDir = path.join(tmpDir, `workspace-spaces-gitlinks-${Date.now()}`) + + // Create workspace directory + await fs.mkdir(workspaceDir, { recursive: true }) + + // Create a primary workspace repo + const mainGit = simpleGit(workspaceDir) + await mainGit.init() + await mainGit.addConfig("user.name", "Roo Code") + await mainGit.addConfig("user.email", "support@roocode.com") + + // Create a test file in the main workspace + const mainFile = path.join(workspaceDir, "main-file.txt") + await fs.writeFile(mainFile, "Content in main repo") + await mainGit.add(".") + await mainGit.commit("Initial commit") + + const logMessages: string[] = [] + const service = await klass.create({ + taskId: `${taskId}-spaces`, + shadowDir, + workspaceDir, + log: (msg: string) => logMessages.push(msg), + }) + + // Mock the git.raw calls to simulate ls-files output with spaces in paths + const mockGit = { + raw: vitest.fn().mockImplementation((args: string[]) => { + if (args[0] === "ls-files" && args.includes("-s")) { + // Simulate git ls-files -s output with gitlinks that have spaces in paths + return Promise.resolve( + [ + "100644 abc123... 0 regular-file.txt", + "160000 def456... 0 nested repo with spaces", + "160000 ghi789... 0 another nested/repo with spaces", + "100644 jkl012... 0 another-regular-file.txt", + ].join("\n"), + ) + } + // For other git commands, use a mock that returns empty strings + return Promise.resolve("") + }), + init: vitest.fn().mockResolvedValue(undefined), + addConfig: vitest.fn().mockResolvedValue(undefined), + revparse: vitest.fn().mockResolvedValue("abc123456"), + commit: vitest.fn().mockResolvedValue({ commit: "def789012" }), + add: vitest.fn().mockResolvedValue(undefined), + } + + // Replace the git instance after service creation (already initialized) + service["git"] = mockGit as any + + // Modify main file to trigger stageAll which should detect gitlinks + await fs.writeFile(mainFile, "Modified content") + + // This should trigger the parsing logic for gitlinks with spaces + await expect(service.saveCheckpoint("Test gitlinks with spaces")).rejects.toThrow( + /Gitlink entries detected/, + ) + + // Verify that both paths with spaces were correctly parsed + expect(mockGit.raw).toHaveBeenCalledWith(["ls-files", "-s", "--cached"]) + + // The error message should include the correctly parsed paths with spaces + try { + await service.saveCheckpoint("Test gitlinks with spaces") + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error) + expect(errorMessage).toContain("nested repo with spaces") + expect(errorMessage).toContain("another nested/repo with spaces") + // Should not contain partial paths that would result from incorrect split() + expect(errorMessage).not.toContain("nested,") + expect(errorMessage).not.toContain("another,") + } + + // Clean up + await fs.rm(shadowDir, { recursive: true, force: true }) + await fs.rm(workspaceDir, { recursive: true, force: true }) + }) + + it("correctly detects nested repos with spaces in paths from index", async () => { + // Create a new temporary workspace and service for this test + const shadowDir = path.join(tmpDir, `${prefix}-spaces-nested-repos-${Date.now()}`) + const workspaceDir = path.join(tmpDir, `workspace-spaces-nested-repos-${Date.now()}`) + + // Create workspace directory + await fs.mkdir(workspaceDir, { recursive: true }) + + // Create a primary workspace repo + const mainGit = simpleGit(workspaceDir) + await mainGit.init() + await mainGit.addConfig("user.name", "Roo Code") + await mainGit.addConfig("user.email", "support@roocode.com") + + // Create a test file in the main workspace + const mainFile = path.join(workspaceDir, "main-file.txt") + await fs.writeFile(mainFile, "Content in main repo") + await mainGit.add(".") + await mainGit.commit("Initial commit") + + const logMessages: string[] = [] + const service = await klass.create({ + taskId: `${taskId}-nested-spaces`, + shadowDir, + workspaceDir, + log: (msg: string) => logMessages.push(msg), + }) + + // Mock git.raw to simulate different ls-files outputs and other commands + const mockGit = { + raw: vitest.fn().mockImplementation((args: string[]) => { + if (args[0] === "ls-files" && args.includes("-s") && !args.includes("--cached")) { + // For findNestedRepos call (ls-files -s without --cached) + return Promise.resolve( + [ + "100644 abc123... 0 regular-file.txt", + "160000 def456... 0 nested repo with spaces", + "160000 ghi789... 0 path with multiple spaces", + "100644 jkl012... 0 another-regular-file.txt", + ].join("\n"), + ) + } else if (args[0] === "ls-files" && args.includes("--cached")) { + // For stageAll safety check (should be empty after proper exclusion) + return Promise.resolve("") + } else if (args[0] === "config" && args.includes(".gitmodules")) { + // No .gitmodules file + const error = new Error("exit code 1") + throw error + } else if (args[0] === "add") { + // Mock successful add operation + return Promise.resolve("") + } else if (args[0] === "rm") { + // Mock successful rm operation + return Promise.resolve("") + } + return Promise.resolve("") + }), + init: vitest.fn().mockResolvedValue(undefined), + addConfig: vitest.fn().mockResolvedValue(undefined), + revparse: vitest.fn().mockResolvedValue("abc123456"), + commit: vitest.fn().mockResolvedValue({ commit: "def789012" }), + add: vitest.fn().mockResolvedValue(undefined), + } + + // Mock executeRipgrep to return empty results (no filesystem-based detections) + vitest.spyOn(fileSearch, "executeRipgrep").mockResolvedValue([]) + + // Replace the git instance after service creation (already initialized) + service["git"] = mockGit as any + + // Modify main file and save checkpoint + await fs.writeFile(mainFile, "Modified content") + await service.saveCheckpoint("Test nested repos with spaces") + + // Verify git.raw was called for ls-files to detect nested repos + expect(mockGit.raw).toHaveBeenCalledWith(["ls-files", "-s"]) + + // Verify that paths with spaces were correctly detected and logged + const excludeLog = logMessages.find((msg) => msg.includes("excluding") && msg.includes("nested repos")) + expect(excludeLog).toBeDefined() + expect(excludeLog).toContain("nested repo with spaces") + expect(excludeLog).toContain("path with multiple spaces") + + // Clean up + vitest.restoreAllMocks() + await fs.rm(shadowDir, { recursive: true, force: true }) + await fs.rm(workspaceDir, { recursive: true, force: true }) + }) + it("normalizes Windows-style backslash paths from filesystem scans to POSIX for git pathspecs", async () => { // Create a new temporary workspace and service for this test. const shadowDir = path.join(tmpDir, `${prefix}-windows-paths-${Date.now()}`)