diff --git a/.changes/next-release/bugfix-d13b5621-b191-455b-8918-1a5874216512.json b/.changes/next-release/bugfix-d13b5621-b191-455b-8918-1a5874216512.json new file mode 100644 index 00000000000..37255c44f88 --- /dev/null +++ b/.changes/next-release/bugfix-d13b5621-b191-455b-8918-1a5874216512.json @@ -0,0 +1,4 @@ +{ + "type" : "bugfix", + "description" : "Amazon Q: Fix cases where content may be incorrectly excluded from workspace." +} \ No newline at end of file diff --git a/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonq/workspace/context/WorkspaceTest.kt b/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonq/workspace/context/WorkspaceTest.kt index c0d443f89ce..daee081cff0 100644 --- a/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonq/workspace/context/WorkspaceTest.kt +++ b/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonq/workspace/context/WorkspaceTest.kt @@ -3,12 +3,19 @@ package software.aws.toolkits.jetbrains.services.amazonq.workspace.context +import com.intellij.openapi.vcs.changes.ChangeListManager import com.intellij.openapi.vfs.VirtualFile import com.intellij.openapi.vfs.refreshAndFindVirtualDirectory import com.intellij.openapi.vfs.refreshAndFindVirtualFile import com.intellij.testFramework.LightPlatformTestCase +import org.mockito.Mockito.mock +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.whenever +import software.aws.toolkits.jetbrains.services.amazonq.project.additionalGlobalIgnoreRules +import software.aws.toolkits.jetbrains.services.amazonq.project.additionalGlobalIgnoreRulesForStrictSources import software.aws.toolkits.jetbrains.services.amazonq.project.findWorkspaceRoot import software.aws.toolkits.jetbrains.services.amazonq.project.isContentInWorkspace +import software.aws.toolkits.jetbrains.services.amazonq.project.isWorkspaceSourceContent import java.nio.file.Files import java.nio.file.Path import kotlin.io.path.createDirectories @@ -106,4 +113,97 @@ class WorkspaceTest : LightPlatformTestCase() { assertFalse(isContentInWorkspace(testPath, setOf(projectPath, modulePath))) } + + fun `test isWorkspaceSourceContent returns true for non-ignored file or directory with default ignore rules`() { + val projectPath = createDir("test/project") + val directoryPath = createDir("test/project/module") + val codeFilePath = createDir("test/project/module/example.java") + val nonCodeFilePath = createDir("test/project/module/example.bin") + + val changeListManager = mock().apply { + whenever(isIgnoredFile(directoryPath)) doReturn false + } + + assertTrue( + isWorkspaceSourceContent( + directoryPath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRules + ) + ) + + assertTrue( + isWorkspaceSourceContent( + codeFilePath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRules + ) + ) + + assertTrue( + isWorkspaceSourceContent( + nonCodeFilePath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRules + ) + ) + + assertTrue( + isWorkspaceSourceContent( + directoryPath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRulesForStrictSources + ) + ) + + assertTrue( + isWorkspaceSourceContent( + codeFilePath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRulesForStrictSources + ) + ) + } + + fun `test isWorkspaceSourceContent returns false for ignored file or directory with default ignore rules`() { + val projectPath = createDir("test/project") + val directoryPath = createDir("test/project/node_modules") + val nonCodeFilePath = createDir("test/project/module/example.bin") + + val changeListManager = mock().apply { + whenever(isIgnoredFile(directoryPath)) doReturn false + } + + assertFalse( + isWorkspaceSourceContent( + directoryPath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRules + ) + ) + + assertFalse( + isWorkspaceSourceContent( + directoryPath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRulesForStrictSources + ) + ) + + assertFalse( + isWorkspaceSourceContent( + nonCodeFilePath, + setOf(projectPath), + changeListManager, + additionalGlobalIgnoreRulesForStrictSources + ) + ) + } } diff --git a/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonqFeatureDev/FeatureDevSessionContextTest.kt b/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonqFeatureDev/FeatureDevSessionContextTest.kt index 05ec05e3549..664141b9a5b 100644 --- a/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonqFeatureDev/FeatureDevSessionContextTest.kt +++ b/plugins/amazonq/chat/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/amazonqFeatureDev/FeatureDevSessionContextTest.kt @@ -15,13 +15,9 @@ import software.aws.toolkits.jetbrains.utils.rules.HeavyJavaCodeInsightTestFixtu import software.aws.toolkits.jetbrains.utils.rules.addFileToModule import java.util.zip.ZipFile -class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTestFixtureRule()) { - - private fun addFilesToProjectModule(vararg path: String) { - val module = projectRule.module - path.forEach { projectRule.fixture.addFileToModule(module, it, it) } - } +data class FileCase(val path: String, val content: String = "", val shouldInclude: Boolean = true) +class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTestFixtureRule()) { @Rule @JvmField val ruleChain = RuleChain(projectRule, disposableRule) @@ -35,90 +31,40 @@ class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTest featureDevSessionContext = FeatureDevSessionContext(featureDevService.project, 1024) } - // FIXME: Add deeper tests, replacing previous shallow tests - BLOCKING - - @Test - fun testZipProjectWithoutAutoDev() { - checkZipProject( - false, - setOf( - "src/MyClass.java", - "icons/menu.svg", - "assets/header.jpg", - "archive.zip", - "output.bin", - "gradle/wrapper/gradle-wrapper.jar", - "gradle/wrapper/gradle-wrapper.properties", - "images/logo.png", - "builder/GetTestBuilder.java", - "gradlew", - "README.md", - ".gitignore", - "License.md", - "gradlew.bat", - "license.txt", - "build.gradle", - "settings.gradle" - ) - ) - } - - @Test - fun testZipProjectWithAutoDev() { - checkZipProject( - true, - setOf( - "src/MyClass.java", - "icons/menu.svg", - "assets/header.jpg", - "archive.zip", - "output.bin", - "gradle/wrapper/gradle-wrapper.jar", - "gradle/wrapper/gradle-wrapper.properties", - "images/logo.png", - "builder/GetTestBuilder.java", - "gradlew", - "README.md", - ".gitignore", - "License.md", - "gradlew.bat", - "license.txt", - "build.gradle", - "devfile.yaml", - "settings.gradle" - ) - ) - } + private fun fileCases(autoBuildEnabled: Boolean) = listOf( + FileCase(path = ".gitignore"), + FileCase(path = ".gradle/cached.jar", shouldInclude = false), + FileCase(path = "src/MyClass.java"), + FileCase(path = "gradlew"), + FileCase(path = "gradlew.bat"), + FileCase(path = "README.md"), + FileCase(path = "settings.gradle"), + FileCase(path = "build.gradle"), + FileCase(path = "gradle/wrapper/gradle-wrapper.properties"), + FileCase(path = "builder/GetTestBuilder.java"), + FileCase(path = ".aws-sam/build/function1", shouldInclude = false), + FileCase(path = ".gem/specs.rb", shouldInclude = false), + FileCase(path = "archive.zip"), + FileCase(path = "output.bin"), + FileCase(path = "images/logo.png"), + FileCase(path = "assets/header.jpg"), + FileCase(path = "icons/menu.svg"), + FileCase(path = "license.txt"), + FileCase(path = "License.md"), + FileCase(path = "node_modules/express", shouldInclude = false), + FileCase(path = "build/outputs", shouldInclude = false), + FileCase(path = "dist/bundle.js", shouldInclude = false), + FileCase(path = "gradle/wrapper/gradle-wrapper.jar"), + FileCase(path = "big-file.txt", content = "blob".repeat(1024 * 1024), shouldInclude = false), + FileCase(path = "devfile.yaml", shouldInclude = autoBuildEnabled), + ) - fun checkZipProject(autoBuildEnabled: Boolean, expectedFiles: Set) { - addFilesToProjectModule( - ".gitignore", - ".gradle/cached.jar", - "src/MyClass.java", - "gradlew", - "gradlew.bat", - "README.md", - "settings.gradle", - "build.gradle", - "gradle/wrapper/gradle-wrapper.properties", - "builder/GetTestBuilder.java", // check for false positives - ".aws-sam/build/function1", - ".gem/specs.rb", - "archive.zip", - "output.bin", - "images/logo.png", - "assets/header.jpg", - "icons/menu.svg", - "license.txt", - "License.md", - "node_modules/express", - "build/outputs", - "dist/bundle.js", - "gradle/wrapper/gradle-wrapper.jar", - "devfile.yaml", - ) + private fun checkZipProject(autoBuildEnabled: Boolean, fileCases: Iterable, onBeforeZip: (() -> Unit)? = null) { + fileCases.forEach { + projectRule.fixture.addFileToModule(module, it.path, it.content) + } - projectRule.fixture.addFileToModule(module, "large-file.txt", "loblob".repeat(1024 * 1024)) + onBeforeZip?.invoke() val zipResult = featureDevSessionContext.getProjectZip(autoBuildEnabled) val zipPath = zipResult.payload.path @@ -132,6 +78,60 @@ class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTest } } - assertEquals(zippedFiles, expectedFiles) + // The input file paths are relative to the workspaceRoot, however the zip content is relative to the addressableRoot: + val addressableRoot = featureDevSessionContext.addressableRoot.path + val workspaceRoot = featureDevSessionContext.workspaceRoot.path + val base = addressableRoot.removePrefix(workspaceRoot).removePrefix("/") + fun addressablePathOf(path: String) = path.removePrefix(base).removePrefix("/") + + fileCases.forEach { + assertEquals(it.shouldInclude, zippedFiles.contains(addressablePathOf(it.path))) + } + } + + @Test + fun `test zip with autoBuild enabled`() { + checkZipProject(autoBuildEnabled = true, fileCases(autoBuildEnabled = true)) + } + + @Test + fun `test zip with autoBuild disabled`() { + checkZipProject(autoBuildEnabled = false, fileCases(autoBuildEnabled = false)) + } + + @Test + fun `test content is included when selection root is workspace root`() { + val fileCases = listOf( + FileCase(path = "file.txt", shouldInclude = true), + FileCase(path = "project/file.txt", shouldInclude = true), + FileCase(path = "deep/nested/file.txt", shouldInclude = true) + ) + + checkZipProject(autoBuildEnabled = false, fileCases = fileCases, onBeforeZip = { + featureDevSessionContext.selectionRoot = featureDevSessionContext.workspaceRoot + }) + } + + @Test + fun `test content is included within selection root which is deeper than content root`() { + val fileCases = listOf(FileCase(path = "project/module/deep/file.txt", shouldInclude = true)) + + checkZipProject(autoBuildEnabled = false, fileCases = fileCases, onBeforeZip = { + featureDevSessionContext.selectionRoot = featureDevSessionContext.workspaceRoot.findFileByRelativePath("project/module/deep") + ?: error("Failed to find fixture") + }) + } + + @Test + fun `test content is excluded outside of selection root`() { + val fileCases = listOf( + FileCase(path = "project/module/file.txt", shouldInclude = true), + FileCase(path = "project/outside/no.txt", shouldInclude = false), + ) + + checkZipProject(autoBuildEnabled = false, fileCases = fileCases, onBeforeZip = { + featureDevSessionContext.selectionRoot = featureDevSessionContext.workspaceRoot.findFileByRelativePath("project/module") + ?: error("Failed to find fixture") + }) } } diff --git a/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/FeatureDevSessionContext.kt b/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/FeatureDevSessionContext.kt index d61c62bf384..e02e1d5bd7a 100644 --- a/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/FeatureDevSessionContext.kt +++ b/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/FeatureDevSessionContext.kt @@ -18,7 +18,7 @@ import org.apache.commons.codec.digest.DigestUtils import org.apache.commons.io.FileUtils import software.aws.toolkits.jetbrains.core.coroutines.getCoroutineBgContext import software.aws.toolkits.jetbrains.services.amazonq.QConstants.MAX_FILE_SIZE_BYTES -import software.aws.toolkits.jetbrains.utils.isDevFile +import software.aws.toolkits.jetbrains.utils.isWorkspaceDevFile import software.aws.toolkits.resources.AwsCoreBundle import software.aws.toolkits.telemetry.AmazonqTelemetry import java.io.File @@ -73,32 +73,6 @@ open class FeatureDevSessionContext(val project: Project, val maxProjectSizeByte return ZipCreationResult(zippedProject, checkSum256, zippedProject.length()) } - private fun shouldIncludeInZipFile(file: VirtualFile, isAutoBuildFeatureEnabled: Boolean): Boolean { - // Large files always ignored: - if (file.length > MAX_FILE_SIZE_BYTES) { - return false - } - - // Exclude files specified by gitignore or outside the workspace: - if (!isWorkspaceSourceContent(file, workspaceContentRoots, changeListManager, additionalGlobalIgnoreRules)) { - return false - } - - // Exclude files outside the selection root working on a subset of the workspace: - if (!VfsUtil.isAncestor(selectionRoot, file, false)) { - return false - } - - // When auto build is enabled, include all other files: - return if (isAutoBuildFeatureEnabled) { - true - } else { - // When auto build is disabled, explicitly exclude devfile: - // FIXME: There should be a stronger signal to the agent than presence of the devfile in the uploaded files to enable auto build - !isDevFile(file) - } - } - private suspend fun zipFiles(isAutoBuildFeatureEnabled: Boolean?): File = withContext(getCoroutineBgContext()) { val files = mutableListOf() val ignoredExtensionMap = mutableMapOf().withDefault { 0L } @@ -109,21 +83,59 @@ open class FeatureDevSessionContext(val project: Project, val maxProjectSizeByte contentRoot, object : VirtualFileVisitor(NO_FOLLOW_SYMLINKS) { override fun visitFile(file: VirtualFile): Boolean { - val isIncluded = shouldIncludeInZipFile(file, isAutoBuildFeatureEnabled == true) - if (!isIncluded) { + fun markIgnoredContent() { val extension = file.extension.orEmpty() ignoredExtensionMap[extension] = (ignoredExtensionMap[extension] ?: 0) + 1 + } + + fun addContent() { + if (file.isFile) { + totalSize += file.length + files.add(file) + + if (maxProjectSizeBytes != null && totalSize > maxProjectSizeBytes) { + throw RepoSizeLimitError(AwsCoreBundle.message("amazonqFeatureDev.content_length.error_text")) + } + } + } + + // Always include DevFile if it is enabled and present, taking precedence over other conditions: + if (isAutoBuildFeatureEnabled == true && isWorkspaceDevFile(file, addressableRoot)) { + addContent() + return true + } + + // Large files always ignored: + if (file.length > MAX_FILE_SIZE_BYTES) { + markIgnoredContent() return false } - if (file.isFile) { - totalSize += file.length - files.add(file) + // Exclude files specified by gitignore or outside the workspace: + if (!isWorkspaceSourceContent(file, workspaceContentRoots, changeListManager, additionalGlobalIgnoreRules)) { + markIgnoredContent() + return false + } - if (maxProjectSizeBytes != null && totalSize > maxProjectSizeBytes) { - throw RepoSizeLimitError(AwsCoreBundle.message("amazonqFeatureDev.content_length.error_text")) + // Exclude files and directories outside the selection root when working on a subset of the workspace: + if (!VfsUtil.isAncestor(selectionRoot, file, false)) { + // Because we traverse from the content root, ensure we continue traverse toward the selection root: + // (Handles when selection root is inside a content root) + if (VfsUtil.isAncestor(file, selectionRoot, false)) { + return true } + markIgnoredContent() + return false + } + + // When auto build is disabled, explicitly exclude devfile: + // FIXME: There should be a stronger signal to the agent than presence of the devfile in the uploaded files to enable auto build + if (isAutoBuildFeatureEnabled == false && isWorkspaceDevFile(file, addressableRoot)) { + markIgnoredContent() + return false } + + addContent() return true } } diff --git a/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/Workspace.kt b/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/Workspace.kt index 0be91f33726..2cf503aa374 100644 --- a/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/Workspace.kt +++ b/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/project/Workspace.kt @@ -79,7 +79,7 @@ val additionalGlobalIgnoreRulesForStrictSources = listOf( // FIXME: It is incredibly brittle that this source of truth is a "telemetry" component // It would be worth considering sniffing text vs arbitrary binary file contents, and making decisions on that basis, rather than file extension. - fun (path: VirtualFile) = !ALLOWED_CODE_EXTENSIONS.contains(path.extension), + fun (path: VirtualFile) = path.extension != null && !ALLOWED_CODE_EXTENSIONS.contains(path.extension), ) /** diff --git a/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/utils/DevFileUtils.kt b/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/utils/DevFileUtils.kt index 9218620edb7..b9a2a7974c1 100644 --- a/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/utils/DevFileUtils.kt +++ b/plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/utils/DevFileUtils.kt @@ -5,4 +5,6 @@ package software.aws.toolkits.jetbrains.utils import com.intellij.openapi.vfs.VirtualFile -fun isDevFile(file: VirtualFile): Boolean = file.name.matches(Regex("devfile\\.ya?ml", RegexOption.IGNORE_CASE)) +fun isWorkspaceDevFile(file: VirtualFile, addressableRoot: VirtualFile): Boolean = + file.name.matches(Regex("devfile\\.ya?ml", RegexOption.IGNORE_CASE)) && + file.parent?.path == addressableRoot.path