fix(checkpoints): resolve incorrect commit location when GIT_DIR set in Dev Containers (#8811)

Author: heyseth

src/services/checkpoints/ShadowCheckpointService.ts

@@ -4,7 +4,7 @@ import * as path from "path"
 import crypto from "crypto"
 import EventEmitter from "events"
 
-import simpleGit, { SimpleGit } from "simple-git"
+import simpleGit, { SimpleGit, SimpleGitOptions } from "simple-git"
 import pWaitFor from "p-wait-for"
 import * as vscode from "vscode"
 
@@ -15,6 +15,65 @@ import { t } from "../../i18n"
 import { CheckpointDiff, CheckpointResult, CheckpointEventMap } from "./types"
 import { getExcludePatterns } from "./excludes"
 
+/**
+ * Creates a SimpleGit instance with sanitized environment variables to prevent
+ * interference from inherited git environment variables like GIT_DIR and GIT_WORK_TREE.
+ * This ensures checkpoint operations always target the intended shadow repository.
+ *
+ * @param baseDir - The directory where git operations should be executed
+ * @returns A SimpleGit instance with sanitized environment
+ */
+function createSanitizedGit(baseDir: string): SimpleGit {
+	// Create a clean environment by explicitly unsetting git-related environment variables
+	// that could interfere with checkpoint operations
+	const sanitizedEnv: Record<string, string> = {}
+	const removedVars: string[] = []
+
+	// Copy all environment variables except git-specific ones
+	for (const [key, value] of Object.entries(process.env)) {
+		// Skip git environment variables that would override repository location
+		if (
+			key === "GIT_DIR" ||
+			key === "GIT_WORK_TREE" ||
+			key === "GIT_INDEX_FILE" ||
+			key === "GIT_OBJECT_DIRECTORY" ||
+			key === "GIT_ALTERNATE_OBJECT_DIRECTORIES" ||
+			key === "GIT_CEILING_DIRECTORIES"
+		) {
+			removedVars.push(`${key}=${value}`)
+			continue
+		}
+
+		// Only include defined values
+		if (value !== undefined) {
+			sanitizedEnv[key] = value
+		}
+	}
+
+	// Log which git env vars were removed (helps with debugging Dev Container issues)
+	if (removedVars.length > 0) {
+		console.log(
+			`[createSanitizedGit] Removed git environment variables for checkpoint isolation: ${removedVars.join(", ")}`,
+		)
+	}
+
+	const options: Partial<SimpleGitOptions> = {
+		baseDir,
+		config: [],
+	}
+
+	// Create git instance and set the sanitized environment
+	const git = simpleGit(options)
+
+	// Use the .env() method to set the complete sanitized environment
+	// This replaces the inherited environment with our sanitized version
+	git.env(sanitizedEnv)
+
+	console.log(`[createSanitizedGit] Created git instance for baseDir: ${baseDir}`)
+
+	return git
+}
+
 export abstract class ShadowCheckpointService extends EventEmitter {
 	public readonly taskId: string
 	public readonly checkpointsDir: string
@@ -85,7 +144,7 @@ export abstract class ShadowCheckpointService extends EventEmitter {
 		}
 
 		await fs.mkdir(this.checkpointsDir, { recursive: true })
-		const git = simpleGit(this.checkpointsDir)
+		const git = createSanitizedGit(this.checkpointsDir)
 		const gitVersion = await git.version()
 		this.log(`[${this.constructor.name}#create] git = ${gitVersion}`)
 
@@ -391,7 +450,7 @@ export abstract class ShadowCheckpointService extends EventEmitter {
 	}) {
 		const workspaceRepoDir = this.workspaceRepoDir({ globalStorageDir, workspaceDir })
 		const branchName = `roo-${taskId}`
-		const git = simpleGit(workspaceRepoDir)
+		const git = createSanitizedGit(workspaceRepoDir)
 		const success = await this.deleteBranch(git, branchName)
 
 		if (success) {

src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts

@@ -823,6 +823,95 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
 				// File should be back to original state
 				expect(await fs.readFile(testFile, "utf-8")).toBe("Hello, world!")
 			})
+
+			it("isolates checkpoint operations from GIT_DIR environment variable", async () => {
+			// This test verifies the fix for the issue where GIT_DIR environment variable
+			// causes checkpoint commits to go to the wrong repository.
+			// In the real-world Dev Container scenario, GIT_DIR is set BEFORE Roo starts,
+			// so we need to set it BEFORE creating the checkpoint service.
+
+			// Create a separate git directory to simulate GIT_DIR pointing elsewhere
+			const externalGitDir = path.join(tmpDir, `external-git-${Date.now()}`)
+			await fs.mkdir(externalGitDir, { recursive: true })
+			const externalGit = simpleGit(externalGitDir)
+			await externalGit.init()
+			await externalGit.addConfig("user.name", "External User")
+			await externalGit.addConfig("user.email", "[email protected]")
+
+			// Create and commit a file in the external repo
+			const externalFile = path.join(externalGitDir, "external.txt")
+			await fs.writeFile(externalFile, "External content")
+			await externalGit.add(".")
+			await externalGit.commit("External commit")
+
+			// Store the original commit count in the external repo
+			const externalLogBefore = await externalGit.log()
+			const externalCommitCountBefore = externalLogBefore.total
+
+			// Initialize the workspace repo BEFORE setting GIT_DIR
+			// (In Dev Containers, the workspace repo already exists before GIT_DIR is set)
+			const testShadowDir = path.join(tmpDir, `shadow-git-dir-test-${Date.now()}`)
+			const testWorkspaceDir = path.join(tmpDir, `workspace-git-dir-test-${Date.now()}`)
+			const testRepo = await initWorkspaceRepo({ workspaceDir: testWorkspaceDir })
+
+			// Set GIT_DIR to point to the external repository BEFORE creating the service
+			// This simulates the Dev Container environment where GIT_DIR is already set
+			const originalGitDir = process.env.GIT_DIR
+			const externalDotGit = path.join(externalGitDir, ".git")
+			process.env.GIT_DIR = externalDotGit
+
+			try {
+				// Create a new checkpoint service with GIT_DIR already set
+				// This is the key difference - we're creating the service
+				// while GIT_DIR is set, just like in a real Dev Container
+				const testService = await klass.create({
+					taskId: `test-git-dir-${Date.now()}`,
+					shadowDir: testShadowDir,
+					workspaceDir: testWorkspaceDir,
+					log: () => {},
+				})
+				await testService.initShadowGit()
+
+				// Make a change in the workspace and save a checkpoint
+				const testWorkspaceFile = path.join(testWorkspaceDir, "test.txt")
+				await fs.writeFile(testWorkspaceFile, "Modified with GIT_DIR set")
+				const commit = await testService.saveCheckpoint("Checkpoint with GIT_DIR set")
+				expect(commit?.commit).toBeTruthy()
+
+				// Verify the checkpoint was saved in the shadow repo, not the external repo
+				// Temporarily clear GIT_DIR to check the external repo
+				delete process.env.GIT_DIR
+				const externalGitCheck = simpleGit(externalGitDir)
+				const externalLogAfter = await externalGitCheck.log()
+				const externalCommitCountAfter = externalLogAfter.total
+				// Restore GIT_DIR
+				process.env.GIT_DIR = externalDotGit
+
+				// External repo should have the same number of commits (no new commits)
+				expect(externalCommitCountAfter).toBe(externalCommitCountBefore)
+
+				// Verify the checkpoint is accessible in the shadow repo
+				const diff = await testService.getDiff({ to: commit!.commit })
+				expect(diff).toHaveLength(1)
+				expect(diff[0].paths.relative).toBe("test.txt")
+				expect(diff[0].content.after).toBe("Modified with GIT_DIR set")
+
+				// Verify we can restore the checkpoint
+				await fs.writeFile(testWorkspaceFile, "Another modification")
+				await testService.restoreCheckpoint(commit!.commit)
+				expect(await fs.readFile(testWorkspaceFile, "utf-8")).toBe("Modified with GIT_DIR set")
+			} finally {
+				// Restore original GIT_DIR
+				if (originalGitDir !== undefined) {
+					process.env.GIT_DIR = originalGitDir
+				} else {
+					delete process.env.GIT_DIR
+				}
+
+				// Clean up external git directory
+				await fs.rm(externalGitDir, { recursive: true, force: true })
+			}
+		})
 		})
 	},
 )