From c6b644a1b622a5f983bb42c4b171ccee61e35d38 Mon Sep 17 00:00:00 2001 From: matt-rudolph Date: Fri, 19 Sep 2025 14:28:11 -0600 Subject: [PATCH] refactor(marketplace): typed origin checks + helper; scoped deletion safety and i18n; atomic deleteCustomModeForSource; update tests and en locale --- src/core/config/CustomModesManager.ts | 50 +++++++-------- src/i18n/locales/en/marketplace.json | 3 + .../marketplace/MarketplaceManager.ts | 63 +++++++++++-------- src/services/marketplace/SimpleInstaller.ts | 23 +++---- .../__tests__/SimpleInstaller.spec.ts | 31 ++++----- 5 files changed, 92 insertions(+), 78 deletions(-) diff --git a/src/core/config/CustomModesManager.ts b/src/core/config/CustomModesManager.ts index 6a0b4113b5..08d100ae43 100644 --- a/src/core/config/CustomModesManager.ts +++ b/src/core/config/CustomModesManager.ts @@ -563,35 +563,35 @@ export class CustomModesManager { fromMarketplace = false, ): Promise { try { - const settingsPath = await this.getCustomModesFilePath() - const roomodesPath = await this.getWorkspaceRoomodes() - - let targetPath: string | undefined - let modeToDelete: ModeConfig | undefined - - if (source === "project") { - if (!roomodesPath) { - throw new Error(t("common:customModes.errors.noWorkspaceForProject")) - } - targetPath = roomodesPath - const roomodesModes = await this.loadModesFromFile(roomodesPath) - modeToDelete = roomodesModes.find((m) => m.slug === slug) - } else { - targetPath = settingsPath - const settingsModes = await this.loadModesFromFile(settingsPath) - modeToDelete = settingsModes.find((m) => m.slug === slug) - } - - if (!modeToDelete) { - throw new Error(t("common:customModes.errors.modeNotFound")) - } - await this.queueWrite(async () => { + const settingsPath = await this.getCustomModesFilePath() + const roomodesPath = await this.getWorkspaceRoomodes() + + let targetPath: string + let modeToDelete: ModeConfig | undefined + + if (source === "project") { + if (!roomodesPath) { + throw new Error(t("common:customModes.errors.noWorkspaceForProject")) + } + targetPath = roomodesPath + const roomodesModes = await this.loadModesFromFile(roomodesPath) + modeToDelete = roomodesModes.find((m) => m.slug === slug) + } else { + targetPath = settingsPath + const settingsModes = await this.loadModesFromFile(settingsPath) + modeToDelete = settingsModes.find((m) => m.slug === slug) + } + + if (!modeToDelete) { + throw new Error(t("common:customModes.errors.modeNotFound")) + } + // Delete only from the selected source file - await this.updateModesInFile(targetPath!, (modes) => modes.filter((m) => m.slug !== slug)) + await this.updateModesInFile(targetPath, (modes) => modes.filter((m) => m.slug !== slug)) // Delete associated rules folder using the located mode (preserves correct scope) - await this.deleteRulesFolder(slug, modeToDelete!, fromMarketplace) + await this.deleteRulesFolder(slug, modeToDelete, fromMarketplace) // Refresh state and clear caches this.clearCache() diff --git a/src/i18n/locales/en/marketplace.json b/src/i18n/locales/en/marketplace.json index 17ae20078a..7f473ab801 100644 --- a/src/i18n/locales/en/marketplace.json +++ b/src/i18n/locales/en/marketplace.json @@ -65,5 +65,8 @@ "removing": "Removing item: \"{{itemName}}\"", "removeSuccess": "\"{{itemName}}\" removed successfully", "removeError": "Failed to remove \"{{itemName}}\": {{errorMessage}}" + }, + "errors": { + "scopedDeletionNotSupported": "Scoped deletion is not supported in this version. Please update Roo Code to the latest version and try again." } } diff --git a/src/services/marketplace/MarketplaceManager.ts b/src/services/marketplace/MarketplaceManager.ts index 474a53ac36..704b930a21 100644 --- a/src/services/marketplace/MarketplaceManager.ts +++ b/src/services/marketplace/MarketplaceManager.ts @@ -4,7 +4,8 @@ import * as path from "path" import * as vscode from "vscode" import * as yaml from "yaml" -import type { OrganizationSettings, MarketplaceItem, MarketplaceItemType, McpMarketplaceItem } from "@roo-code/types" +import type { OrganizationSettings, MarketplaceItem, MarketplaceItemType, McpMarketplaceItem, ModeConfig } from "@roo-code/types" +import { customModesSettingsSchema } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { CloudService } from "@roo-code/cloud" @@ -241,6 +242,38 @@ export class MarketplaceManager { return metadata } + // Helper: identify marketplace-installed modes + private isMarketplaceInstalledMode( + mode: ModeConfig, + ): mode is ModeConfig & { installedFromMarketplace: true; marketplaceItemId: string } { + return ( + mode.installedFromMarketplace === true && + typeof mode.marketplaceItemId === "string" && + mode.marketplaceItemId.length > 0 + ) + } + + // Helper: parse YAML and collect installed mode metadata with proper typing + private collectInstalledModesFromYaml( + content: string, + out: Record, + ): void { + try { + const parsed = yaml.parse(content) + const result = customModesSettingsSchema.safeParse(parsed) + if (!result.success) { + return + } + for (const mode of result.data.customModes) { + if (this.isMarketplaceInstalledMode(mode)) { + out[mode.marketplaceItemId!] = { type: "mode" } + } + } + } catch { + // Ignore parse errors here; caller handles file existence/errors + } + } + /** * Check for project-level installed items */ @@ -255,19 +288,7 @@ export class MarketplaceManager { const projectModesPath = path.join(workspaceFolder.uri.fsPath, ".roomodes") try { const content = await fs.readFile(projectModesPath, "utf-8") - const data = yaml.parse(content) - if (data?.customModes && Array.isArray(data.customModes)) { - for (const mode of data.customModes) { - // Only consider marketplace-installed modes and key by marketplaceItemId - const fromMarketplace = (mode as any)?.installedFromMarketplace === true - const marketplaceItemId = (mode as any)?.marketplaceItemId - if (fromMarketplace && typeof marketplaceItemId === "string" && marketplaceItemId.length > 0) { - metadata[marketplaceItemId] = { - type: "mode", - } - } - } - } + this.collectInstalledModesFromYaml(content, metadata) } catch (error) { // File doesn't exist or can't be read, skip } @@ -303,19 +324,7 @@ export class MarketplaceManager { const globalModesPath = path.join(globalSettingsPath, GlobalFileNames.customModes) try { const content = await fs.readFile(globalModesPath, "utf-8") - const data = yaml.parse(content) - if (data?.customModes && Array.isArray(data.customModes)) { - for (const mode of data.customModes) { - // Only consider marketplace-installed modes and key by marketplaceItemId - const fromMarketplace = (mode as any)?.installedFromMarketplace === true - const marketplaceItemId = (mode as any)?.marketplaceItemId - if (fromMarketplace && typeof marketplaceItemId === "string" && marketplaceItemId.length > 0) { - metadata[marketplaceItemId] = { - type: "mode", - } - } - } - } + this.collectInstalledModesFromYaml(content, metadata) } catch (error) { // File doesn't exist or can't be read, skip } diff --git a/src/services/marketplace/SimpleInstaller.ts b/src/services/marketplace/SimpleInstaller.ts index f951c0054c..6c034a1def 100644 --- a/src/services/marketplace/SimpleInstaller.ts +++ b/src/services/marketplace/SimpleInstaller.ts @@ -2,10 +2,11 @@ import * as vscode from "vscode" import * as path from "path" import * as fs from "fs/promises" import * as yaml from "yaml" -import type { MarketplaceItem, MarketplaceItemType, InstallMarketplaceItemOptions, McpParameter } from "@roo-code/types" +import type { MarketplaceItem, MarketplaceItemType, InstallMarketplaceItemOptions, McpParameter, ModeConfig } from "@roo-code/types" import { GlobalFileNames } from "../../shared/globalFileNames" import { ensureSettingsDirectoryExists } from "../../utils/globalContext" import type { CustomModesManager } from "../../core/config/CustomModesManager" +import { t } from "../../i18n" export interface InstallOptions { target: "project" | "global" @@ -28,7 +29,7 @@ export class SimpleInstaller { case "mcp": return await this.installMcp(item, target, options) default: - throw new Error(`Unsupported item type: ${(item as any).type}`) + throw new Error("Unsupported item type") } } @@ -51,8 +52,8 @@ export class SimpleInstaller { const parsedMode = yaml.parse(item.content) // Annotate marketplace origin to disambiguate from user-created modes if (parsedMode && typeof parsedMode === "object") { - ;(parsedMode as any).installedFromMarketplace = true - ;(parsedMode as any).marketplaceItemId = item.id + ;(parsedMode as ModeConfig).installedFromMarketplace = true + ;(parsedMode as ModeConfig).marketplaceItemId = item.id } const importData = { customModes: [parsedMode], @@ -97,8 +98,8 @@ export class SimpleInstaller { const modeData = yaml.parse(item.content) // Annotate marketplace origin fields for fallback path as well if (modeData && typeof modeData === "object") { - ;(modeData as any).installedFromMarketplace = true - ;(modeData as any).marketplaceItemId = item.id + ;(modeData as ModeConfig).installedFromMarketplace = true + ;(modeData as ModeConfig).marketplaceItemId = item.id } // Read existing file or create new structure @@ -301,7 +302,7 @@ export class SimpleInstaller { await this.removeMcp(item, target) break default: - throw new Error(`Unsupported item type: ${(item as any).type}`) + throw new Error("Unsupported item type") } } @@ -334,7 +335,7 @@ export class SimpleInstaller { // Get the current modes and locate the exact marketplace-installed mode for the selected target const modes = await this.customModesManager.getCustomModes() const candidate = modes.find( - (m: any) => + (m: ModeConfig) => m.slug === modeSlug && m.installedFromMarketplace === true && m.marketplaceItemId === item.id && @@ -342,15 +343,15 @@ export class SimpleInstaller { ) if (!candidate) { - throw new Error("This mode was not installed from the marketplace for the selected target") + throw new Error(t("common:customModes.errors.modeNotFound")) } // Delete only from the selected source to avoid unintended removals if (typeof (this.customModesManager as any).deleteCustomModeForSource === "function") { await (this.customModesManager as any).deleteCustomModeForSource(modeSlug, target, true) } else { - // Fallback to legacy deletion if helper is not available - await this.customModesManager.deleteCustomMode(modeSlug, true) + // Scoped deletion not supported in this version + throw new Error(t("marketplace:errors.scopedDeletionNotSupported")) } } diff --git a/src/services/marketplace/__tests__/SimpleInstaller.spec.ts b/src/services/marketplace/__tests__/SimpleInstaller.spec.ts index bd2828b3cc..bdadbe2a54 100644 --- a/src/services/marketplace/__tests__/SimpleInstaller.spec.ts +++ b/src/services/marketplace/__tests__/SimpleInstaller.spec.ts @@ -42,6 +42,7 @@ describe("SimpleInstaller", () => { mockContext = {} as vscode.ExtensionContext mockCustomModesManager = { deleteCustomMode: vi.fn().mockResolvedValue(undefined), + deleteCustomModeForSource: vi.fn().mockResolvedValue(undefined), importModeWithRules: vi.fn().mockResolvedValue({ success: true }), getCustomModes: vi.fn().mockResolvedValue([]), } as any @@ -225,8 +226,8 @@ describe("SimpleInstaller", () => { await installer.removeItem(mockModeItem, { target: "project" }) - // Should call deleteCustomMode with fromMarketplace flag set to true - expect(mockCustomModesManager.deleteCustomMode).toHaveBeenCalledWith("test", true) + // Should call scoped deletion for the selected source with fromMarketplace flag set to true + expect((mockCustomModesManager as any).deleteCustomModeForSource).toHaveBeenCalledWith("test", "project", true) // The rules folder deletion is now handled by CustomModesManager, not SimpleInstaller expect(fileExistsAtPath).not.toHaveBeenCalled() expect(mockFs.rm).not.toHaveBeenCalled() @@ -240,8 +241,8 @@ describe("SimpleInstaller", () => { await installer.removeItem(mockModeItem, { target: "global" }) - // Should call deleteCustomMode with fromMarketplace flag set to true - expect(mockCustomModesManager.deleteCustomMode).toHaveBeenCalledWith("test", true) + // Should call scoped deletion for the selected source with fromMarketplace flag set to true + expect((mockCustomModesManager as any).deleteCustomModeForSource).toHaveBeenCalledWith("test", "global", true) // The rules folder deletion is now handled by CustomModesManager, not SimpleInstaller expect(fileExistsAtPath).not.toHaveBeenCalled() expect(mockFs.rm).not.toHaveBeenCalled() @@ -255,8 +256,8 @@ describe("SimpleInstaller", () => { await installer.removeItem(mockModeItem, { target: "project" }) - // Should call deleteCustomMode with fromMarketplace flag set to true - expect(mockCustomModesManager.deleteCustomMode).toHaveBeenCalledWith("test", true) + // Should call scoped deletion for the selected source with fromMarketplace flag set to true + expect((mockCustomModesManager as any).deleteCustomModeForSource).toHaveBeenCalledWith("test", "project", true) // The rules folder deletion is now handled by CustomModesManager, not SimpleInstaller expect(fileExistsAtPath).not.toHaveBeenCalled() expect(mockFs.rm).not.toHaveBeenCalled() @@ -267,24 +268,24 @@ describe("SimpleInstaller", () => { vi.mocked(mockCustomModesManager.getCustomModes).mockResolvedValueOnce([ { slug: "test", name: "Test Mode", source: "project", installedFromMarketplace: true, marketplaceItemId: "test-mode" } as any, ]) - // Mock that deleteCustomMode fails - mockCustomModesManager.deleteCustomMode = vi.fn().mockRejectedValueOnce(new Error("Permission denied")) + // Mock that scoped deletion fails + ;(mockCustomModesManager as any).deleteCustomModeForSource = vi + .fn() + .mockRejectedValueOnce(new Error("Permission denied")) - // Should throw the error from deleteCustomMode + // Should throw the error from scoped deletion await expect(installer.removeItem(mockModeItem, { target: "project" })).rejects.toThrow("Permission denied") - expect(mockCustomModesManager.deleteCustomMode).toHaveBeenCalledWith("test", true) + expect((mockCustomModesManager as any).deleteCustomModeForSource).toHaveBeenCalledWith("test", "project", true) }) it("should throw when mode is not marketplace-installed for selected target", async () => { // Mock that the mode doesn't exist in the list vi.mocked(mockCustomModesManager.getCustomModes).mockResolvedValueOnce([]) - await expect(installer.removeItem(mockModeItem, { target: "project" })).rejects.toThrow( - "This mode was not installed from the marketplace for the selected target", - ) + await expect(installer.removeItem(mockModeItem, { target: "project" })).rejects.toThrow(/Mode not found/) - expect(mockCustomModesManager.deleteCustomMode).not.toHaveBeenCalled() + expect((mockCustomModesManager as any).deleteCustomModeForSource).not.toHaveBeenCalled() }) it("should throw error when mode content is invalid YAML", async () => { @@ -339,7 +340,7 @@ describe("SimpleInstaller", () => { await installer.removeItem(arrayContentItem, { target: "project" }) - expect(mockCustomModesManager.deleteCustomMode).toHaveBeenCalledWith("test-array", true) + expect((mockCustomModesManager as any).deleteCustomModeForSource).toHaveBeenCalledWith("test-array", "project", true) }) it("should throw error when CustomModesManager is not available", async () => {