Skip to content

Commit b457e74

Browse files
committed
test(checkpoints): fix GIT_DIR test to match Dev Container scenario
The previous test set GIT_DIR after initializing the checkpoint service, which didn't properly verify the fix. In real Dev Containers, GIT_DIR is already set when Roo starts and creates the checkpoint service. Changes: - Initialize workspace repo before setting GIT_DIR (workspace already exists in real Dev Containers before GIT_DIR is set) - Set GIT_DIR before creating the checkpoint service to properly test that sanitization works during service initialization - Temporarily clear GIT_DIR when verifying external repo wasn't modified, then restore it to continue testing with GIT_DIR set This ensures the test accurately simulates the Dev Container environment and verifies that checkpoint operations are properly isolated despite GIT_DIR being present during initialization.
1 parent 3e1cbbf commit b457e74

File tree

1 file changed

+84
-58
lines changed

1 file changed

+84
-58
lines changed

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

Lines changed: 84 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -825,67 +825,93 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
825825
})
826826

827827
it("isolates checkpoint operations from GIT_DIR environment variable", async () => {
828-
// This test verifies the fix for the issue where GIT_DIR environment variable
829-
// causes checkpoint commits to go to the wrong repository
830-
831-
// Create a separate git directory to simulate GIT_DIR pointing elsewhere
832-
const externalGitDir = path.join(tmpDir, `external-git-${Date.now()}`)
833-
await fs.mkdir(externalGitDir, { recursive: true })
834-
const externalGit = simpleGit(externalGitDir)
835-
await externalGit.init()
836-
await externalGit.addConfig("user.name", "External User")
837-
await externalGit.addConfig("user.email", "[email protected]")
838-
839-
// Create and commit a file in the external repo
840-
const externalFile = path.join(externalGitDir, "external.txt")
841-
await fs.writeFile(externalFile, "External content")
842-
await externalGit.add(".")
843-
await externalGit.commit("External commit")
844-
845-
// Store the original commit count in the external repo
846-
const externalLogBefore = await externalGit.log()
847-
const externalCommitCountBefore = externalLogBefore.total
848-
849-
// Set GIT_DIR to point to the external repository
850-
const originalGitDir = process.env.GIT_DIR
851-
const externalDotGit = path.join(externalGitDir, ".git")
852-
process.env.GIT_DIR = externalDotGit
828+
// This test verifies the fix for the issue where GIT_DIR environment variable
829+
// causes checkpoint commits to go to the wrong repository.
830+
// In the real-world Dev Container scenario, GIT_DIR is set BEFORE Roo starts,
831+
// so we need to set it BEFORE creating the checkpoint service.
832+
833+
// Create a separate git directory to simulate GIT_DIR pointing elsewhere
834+
const externalGitDir = path.join(tmpDir, `external-git-${Date.now()}`)
835+
await fs.mkdir(externalGitDir, { recursive: true })
836+
const externalGit = simpleGit(externalGitDir)
837+
await externalGit.init()
838+
await externalGit.addConfig("user.name", "External User")
839+
await externalGit.addConfig("user.email", "[email protected]")
840+
841+
// Create and commit a file in the external repo
842+
const externalFile = path.join(externalGitDir, "external.txt")
843+
await fs.writeFile(externalFile, "External content")
844+
await externalGit.add(".")
845+
await externalGit.commit("External commit")
846+
847+
// Store the original commit count in the external repo
848+
const externalLogBefore = await externalGit.log()
849+
const externalCommitCountBefore = externalLogBefore.total
850+
851+
// Initialize the workspace repo BEFORE setting GIT_DIR
852+
// (In Dev Containers, the workspace repo already exists before GIT_DIR is set)
853+
const testShadowDir = path.join(tmpDir, `shadow-git-dir-test-${Date.now()}`)
854+
const testWorkspaceDir = path.join(tmpDir, `workspace-git-dir-test-${Date.now()}`)
855+
const testRepo = await initWorkspaceRepo({ workspaceDir: testWorkspaceDir })
856+
857+
// Set GIT_DIR to point to the external repository BEFORE creating the service
858+
// This simulates the Dev Container environment where GIT_DIR is already set
859+
const originalGitDir = process.env.GIT_DIR
860+
const externalDotGit = path.join(externalGitDir, ".git")
861+
process.env.GIT_DIR = externalDotGit
862+
863+
try {
864+
// Create a new checkpoint service with GIT_DIR already set
865+
// This is the key difference - we're creating the service
866+
// while GIT_DIR is set, just like in a real Dev Container
867+
const testService = await klass.create({
868+
taskId: `test-git-dir-${Date.now()}`,
869+
shadowDir: testShadowDir,
870+
workspaceDir: testWorkspaceDir,
871+
log: () => {},
872+
})
873+
await testService.initShadowGit()
853874

854-
try {
855-
// Make a change in the workspace and save a checkpoint
856-
await fs.writeFile(testFile, "Modified with GIT_DIR set")
857-
const commit = await service.saveCheckpoint("Checkpoint with GIT_DIR set")
858-
expect(commit?.commit).toBeTruthy()
859-
860-
// Verify the checkpoint was saved in the shadow repo, not the external repo
861-
const externalLogAfter = await externalGit.log()
862-
const externalCommitCountAfter = externalLogAfter.total
863-
864-
// External repo should have the same number of commits (no new commits)
865-
expect(externalCommitCountAfter).toBe(externalCommitCountBefore)
866-
867-
// Verify the checkpoint is accessible in the shadow repo
868-
const diff = await service.getDiff({ to: commit!.commit })
869-
expect(diff).toHaveLength(1)
870-
expect(diff[0].paths.relative).toBe("test.txt")
871-
expect(diff[0].content.after).toBe("Modified with GIT_DIR set")
872-
873-
// Verify we can restore the checkpoint
874-
await fs.writeFile(testFile, "Another modification")
875-
await service.restoreCheckpoint(commit!.commit)
876-
expect(await fs.readFile(testFile, "utf-8")).toBe("Modified with GIT_DIR set")
877-
} finally {
878-
// Restore original GIT_DIR
879-
if (originalGitDir !== undefined) {
880-
process.env.GIT_DIR = originalGitDir
881-
} else {
882-
delete process.env.GIT_DIR
883-
}
875+
// Make a change in the workspace and save a checkpoint
876+
const testWorkspaceFile = path.join(testWorkspaceDir, "test.txt")
877+
await fs.writeFile(testWorkspaceFile, "Modified with GIT_DIR set")
878+
const commit = await testService.saveCheckpoint("Checkpoint with GIT_DIR set")
879+
expect(commit?.commit).toBeTruthy()
880+
881+
// Verify the checkpoint was saved in the shadow repo, not the external repo
882+
// Temporarily clear GIT_DIR to check the external repo
883+
delete process.env.GIT_DIR
884+
const externalGitCheck = simpleGit(externalGitDir)
885+
const externalLogAfter = await externalGitCheck.log()
886+
const externalCommitCountAfter = externalLogAfter.total
887+
// Restore GIT_DIR
888+
process.env.GIT_DIR = externalDotGit
884889

885-
// Clean up external git directory
886-
await fs.rm(externalGitDir, { recursive: true, force: true })
890+
// External repo should have the same number of commits (no new commits)
891+
expect(externalCommitCountAfter).toBe(externalCommitCountBefore)
892+
893+
// Verify the checkpoint is accessible in the shadow repo
894+
const diff = await testService.getDiff({ to: commit!.commit })
895+
expect(diff).toHaveLength(1)
896+
expect(diff[0].paths.relative).toBe("test.txt")
897+
expect(diff[0].content.after).toBe("Modified with GIT_DIR set")
898+
899+
// Verify we can restore the checkpoint
900+
await fs.writeFile(testWorkspaceFile, "Another modification")
901+
await testService.restoreCheckpoint(commit!.commit)
902+
expect(await fs.readFile(testWorkspaceFile, "utf-8")).toBe("Modified with GIT_DIR set")
903+
} finally {
904+
// Restore original GIT_DIR
905+
if (originalGitDir !== undefined) {
906+
process.env.GIT_DIR = originalGitDir
907+
} else {
908+
delete process.env.GIT_DIR
887909
}
888-
})
910+
911+
// Clean up external git directory
912+
await fs.rm(externalGitDir, { recursive: true, force: true })
913+
}
914+
})
889915
})
890916
},
891917
)

0 commit comments

Comments
 (0)