fix: handle null/empty custom modes files to prevent 'Cannot read properties of null' error (#5526)

* fix: handle null/empty custom modes files to prevent 'Cannot read properties of null' error

- Fix SimpleInstaller to ensure existingData is always an object after yaml.parse
- Fix CustomModesManager.parseYamlSafely to return empty object instead of null
- Ensure customModes array is always initialized in both install and remove operations
- Add tests for empty/null file handling scenarios
- Update existing tests to match correct behavior

* fix: handle null/undefined settings in updateModesInFile and loadModesFromFile

- Ensure settings object exists before accessing customModes property
- Initialize customModes as empty array if undefined
- Prevent 'Cannot read properties of null' error during mode import
- Add proper validation in loadModesFromFile before schema check

Author: daniel-lxs

src/core/config/CustomModesManager.ts

@@ -148,7 +148,9 @@ export class CustomModesManager {
 		cleanedContent = this.cleanInvisibleCharacters(cleanedContent)
 
 		try {
-			return yaml.parse(cleanedContent)
+			const parsed = yaml.parse(cleanedContent)
+			// Ensure we never return null or undefined
+			return parsed ?? {}
 		} catch (yamlError) {
 			// For .roomodes files, try JSON as fallback
 			if (filePath.endsWith(ROOMODES_FILENAME)) {
@@ -180,6 +182,12 @@ export class CustomModesManager {
 		try {
 			const content = await fs.readFile(filePath, "utf-8")
 			const settings = this.parseYamlSafely(content, filePath)
+
+			// Ensure settings has customModes property
+			if (!settings || typeof settings !== "object" || !settings.customModes) {
+				return []
+			}
+
 			const result = customModesSettingsSchema.safeParse(settings)
 
 			if (!result.success) {
@@ -458,7 +466,15 @@ export class CustomModesManager {
 			settings = { customModes: [] }
 		}
 
-		settings.customModes = operation(settings.customModes || [])
+		// Ensure settings is an object and has customModes property
+		if (!settings || typeof settings !== "object") {
+			settings = { customModes: [] }
+		}
+		if (!settings.customModes) {
+			settings.customModes = []
+		}
+
+		settings.customModes = operation(settings.customModes)
 		await fs.writeFile(filePath, yaml.stringify(settings, { lineWidth: 0 }), "utf-8")
 	}
 

src/services/marketplace/SimpleInstaller.ts

@@ -47,7 +47,9 @@ export class SimpleInstaller {
 		let existingData: any = { customModes: [] }
 		try {
 			const existing = await fs.readFile(filePath, "utf-8")
-			existingData = yaml.parse(existing) || { customModes: [] }
+			const parsed = yaml.parse(existing)
+			// Ensure we have a valid object with customModes array
+			existingData = parsed && typeof parsed === "object" ? parsed : { customModes: [] }
 		} catch (error: any) {
 			if (error.code === "ENOENT") {
 				// File doesn't exist, use default structure - this is fine
@@ -253,7 +255,9 @@ export class SimpleInstaller {
 			let existingData: any
 
 			try {
-				existingData = yaml.parse(existing)
+				const parsed = yaml.parse(existing)
+				// Ensure we have a valid object
+				existingData = parsed && typeof parsed === "object" ? parsed : {}
 			} catch (parseError) {
 				// If we can't parse the file, we can't safely remove a mode
 				const fileName = target === "project" ? ".roomodes" : "custom-modes.yaml"
@@ -263,27 +267,30 @@ export class SimpleInstaller {
 				)
 			}
 
-			if (existingData?.customModes) {
-				// Parse the item content to get the slug
-				let content: string
-				if (Array.isArray(item.content)) {
-					// Array of McpInstallationMethod objects - use first method
-					content = item.content[0].content
-				} else {
-					content = item.content
-				}
-				const modeData = yaml.parse(content || "")
-
-				if (!modeData.slug) {
-					return // Nothing to remove if no slug
-				}
+			// Ensure customModes array exists
+			if (!existingData.customModes) {
+				existingData.customModes = []
+			}
 
-				// Remove mode with matching slug
-				existingData.customModes = existingData.customModes.filter((mode: any) => mode.slug !== modeData.slug)
+			// Parse the item content to get the slug
+			let content: string
+			if (Array.isArray(item.content)) {
+				// Array of McpInstallationMethod objects - use first method
+				content = item.content[0].content
+			} else {
+				content = item.content
+			}
+			const modeData = yaml.parse(content || "")
 
-				// Always write back the file, even if empty
-				await fs.writeFile(filePath, yaml.stringify(existingData, { lineWidth: 0 }), "utf-8")
+			if (!modeData.slug) {
+				return // Nothing to remove if no slug
 			}
+
+			// Remove mode with matching slug
+			existingData.customModes = existingData.customModes.filter((mode: any) => mode.slug !== modeData.slug)
+
+			// Always write back the file, even if empty
+			await fs.writeFile(filePath, yaml.stringify(existingData, { lineWidth: 0 }), "utf-8")
 		} catch (error: any) {
 			if (error.code === "ENOENT") {
 				// File doesn't exist, nothing to remove

src/services/marketplace/__tests__/SimpleInstaller.spec.ts

@@ -89,6 +89,59 @@ describe("SimpleInstaller", () => {
 			expect(writtenData.customModes.find((m: any) => m.slug === "test")).toBeDefined()
 		})
 
+		it("should handle empty .roomodes file", async () => {
+			// Empty file content
+			mockFs.readFile.mockResolvedValueOnce("")
+			mockFs.writeFile.mockResolvedValueOnce(undefined as any)
+
+			const result = await installer.installItem(mockModeItem, { target: "project" })
+
+			expect(result.filePath).toBe(path.join("/test/workspace", ".roomodes"))
+			expect(mockFs.writeFile).toHaveBeenCalled()
+
+			// Verify the written content contains the new mode
+			const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
+			const writtenData = yaml.parse(writtenContent)
+			expect(writtenData.customModes).toHaveLength(1)
+			expect(writtenData.customModes[0].slug).toBe("test")
+		})
+
+		it("should handle .roomodes file with null content", async () => {
+			// File exists but yaml.parse returns null
+			mockFs.readFile.mockResolvedValueOnce("---\n")
+			mockFs.writeFile.mockResolvedValueOnce(undefined as any)
+
+			const result = await installer.installItem(mockModeItem, { target: "project" })
+
+			expect(result.filePath).toBe(path.join("/test/workspace", ".roomodes"))
+			expect(mockFs.writeFile).toHaveBeenCalled()
+
+			// Verify the written content contains the new mode
+			const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
+			const writtenData = yaml.parse(writtenContent)
+			expect(writtenData.customModes).toHaveLength(1)
+			expect(writtenData.customModes[0].slug).toBe("test")
+		})
+
+		it("should handle .roomodes file without customModes property", async () => {
+			// File has valid YAML but no customModes property
+			const contentWithoutCustomModes = yaml.stringify({ someOtherProperty: "value" })
+			mockFs.readFile.mockResolvedValueOnce(contentWithoutCustomModes)
+			mockFs.writeFile.mockResolvedValueOnce(undefined as any)
+
+			const result = await installer.installItem(mockModeItem, { target: "project" })
+
+			expect(result.filePath).toBe(path.join("/test/workspace", ".roomodes"))
+			expect(mockFs.writeFile).toHaveBeenCalled()
+
+			// Verify the written content contains the new mode and preserves other properties
+			const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
+			const writtenData = yaml.parse(writtenContent)
+			expect(writtenData.customModes).toHaveLength(1)
+			expect(writtenData.customModes[0].slug).toBe("test")
+			expect(writtenData.someOtherProperty).toBe("value")
+		})
+
 		it("should throw error when .roomodes contains invalid YAML", async () => {
 			const invalidYaml = "invalid: yaml: content: {"
 
@@ -224,5 +277,52 @@ describe("SimpleInstaller", () => {
 
 			expect(mockFs.writeFile).not.toHaveBeenCalled()
 		})
+
+		it("should handle empty .roomodes file during removal", async () => {
+			// Empty file content
+			mockFs.readFile.mockResolvedValueOnce("")
+			mockFs.writeFile.mockResolvedValueOnce(undefined as any)
+
+			// Should not throw
+			await installer.removeItem(mockModeItem, { target: "project" })
+
+			// Should write back a valid structure with empty customModes
+			expect(mockFs.writeFile).toHaveBeenCalled()
+			const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
+			const writtenData = yaml.parse(writtenContent)
+			expect(writtenData.customModes).toEqual([])
+		})
+
+		it("should handle .roomodes file with null content during removal", async () => {
+			// File exists but yaml.parse returns null
+			mockFs.readFile.mockResolvedValueOnce("---\n")
+			mockFs.writeFile.mockResolvedValueOnce(undefined as any)
+
+			// Should not throw
+			await installer.removeItem(mockModeItem, { target: "project" })
+
+			// Should write back a valid structure with empty customModes
+			expect(mockFs.writeFile).toHaveBeenCalled()
+			const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
+			const writtenData = yaml.parse(writtenContent)
+			expect(writtenData.customModes).toEqual([])
+		})
+
+		it("should handle .roomodes file without customModes property during removal", async () => {
+			// File has valid YAML but no customModes property
+			const contentWithoutCustomModes = yaml.stringify({ someOtherProperty: "value" })
+			mockFs.readFile.mockResolvedValueOnce(contentWithoutCustomModes)
+			mockFs.writeFile.mockResolvedValueOnce(undefined as any)
+
+			// Should not throw
+			await installer.removeItem(mockModeItem, { target: "project" })
+
+			// Should write back the file with the same content (no modes to remove)
+			expect(mockFs.writeFile).toHaveBeenCalled()
+			const writtenContent = mockFs.writeFile.mock.calls[0][1] as string
+			const writtenData = yaml.parse(writtenContent)
+			expect(writtenData.customModes).toEqual([])
+			expect(writtenData.someOtherProperty).toBe("value")
+		})
 	})
 })