From acb360b9dea20ee48da8ce847b76b4e23f5d9528 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Fri, 18 Jul 2025 00:10:28 +0000 Subject: [PATCH] fix: resolve custom modes disappearing and creation issues - Fix race condition in updateCustomMode method by reordering state refresh and cache clearing - Improve error handling and state recovery in refreshMergedState method - Add defensive checks for state consistency in getCustomModes method - Add comprehensive logging for debugging state issues - Ensure proper cache invalidation timing to prevent stale data - Add fallback recovery from global state when file operations fail - Validate merged modes before updating state to filter out invalid entries Fixes #5855 --- src/core/config/CustomModesManager.ts | 269 +++++++++++++++++++------- 1 file changed, 195 insertions(+), 74 deletions(-) diff --git a/src/core/config/CustomModesManager.ts b/src/core/config/CustomModesManager.ts index 3da22d1be8..9308401908 100644 --- a/src/core/config/CustomModesManager.ts +++ b/src/core/config/CustomModesManager.ts @@ -352,51 +352,120 @@ export class CustomModesManager { } public async getCustomModes(): Promise { - // Check if we have a valid cached result. - const now = Date.now() + try { + // Check if we have a valid cached result. + const now = Date.now() - if (this.cachedModes && now - this.cachedAt < CustomModesManager.cacheTTL) { - return this.cachedModes - } + if (this.cachedModes && now - this.cachedAt < CustomModesManager.cacheTTL) { + logger.debug("Returning cached custom modes", { count: this.cachedModes.length }) + return this.cachedModes + } - // Get modes from settings file. - const settingsPath = await this.getCustomModesFilePath() - const settingsModes = await this.loadModesFromFile(settingsPath) + logger.debug("Loading custom modes from files") - // Get modes from .roomodes if it exists. - const roomodesPath = await this.getWorkspaceRoomodes() - const roomodesModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] + // Get modes from settings file. + const settingsPath = await this.getCustomModesFilePath() + const settingsModes = await this.loadModesFromFile(settingsPath) - // Create maps to store modes by source. - const projectModes = new Map() - const globalModes = new Map() + // Get modes from .roomodes if it exists. + const roomodesPath = await this.getWorkspaceRoomodes() + const roomodesModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] - // Add project modes (they take precedence). - for (const mode of roomodesModes) { - projectModes.set(mode.slug, { ...mode, source: "project" as const }) - } + logger.debug("Loaded modes from files", { + settingsCount: settingsModes.length, + roomodesCount: roomodesModes.length, + settingsPath, + roomodesPath + }) - // Add global modes. - for (const mode of settingsModes) { - if (!projectModes.has(mode.slug)) { - globalModes.set(mode.slug, { ...mode, source: "global" as const }) + // Create maps to store modes by source. + const projectModes = new Map() + const globalModes = new Map() + + // Add project modes (they take precedence). + for (const mode of roomodesModes) { + if (!mode.slug) { + logger.warn("Found mode without slug in roomodes, skipping", { mode }) + continue + } + projectModes.set(mode.slug, { ...mode, source: "project" as const }) + } + + // Add global modes. + for (const mode of settingsModes) { + if (!mode.slug) { + logger.warn("Found mode without slug in settings, skipping", { mode }) + continue + } + if (!projectModes.has(mode.slug)) { + globalModes.set(mode.slug, { ...mode, source: "global" as const }) + } + } + + // Combine modes in the correct order: project modes first, then global modes. + const mergedModes = [ + ...roomodesModes + .filter((mode) => mode.slug) // Defensive check + .map((mode) => ({ ...mode, source: "project" as const })), + ...settingsModes + .filter((mode) => mode.slug && !projectModes.has(mode.slug)) // Defensive checks + .map((mode) => ({ ...mode, source: "global" as const })), + ] + + logger.debug("Merged custom modes", { + totalCount: mergedModes.length, + projectCount: projectModes.size, + globalCount: globalModes.size + }) + + // Validate merged modes before updating state + const validModes = mergedModes.filter((mode) => { + const isValid = mode.slug && mode.name && mode.roleDefinition && Array.isArray(mode.groups) + if (!isValid) { + logger.warn("Found invalid mode, filtering out", { mode }) + } + return isValid + }) + + if (validModes.length !== mergedModes.length) { + logger.warn("Filtered out invalid modes", { + original: mergedModes.length, + valid: validModes.length + }) + } + + // Update global state with validated modes + await this.context.globalState.update("customModes", validModes) + + // Update cache + this.cachedModes = validModes + this.cachedAt = now + + logger.info("Successfully loaded custom modes", { count: validModes.length }) + return validModes + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error) + logger.error("Failed to get custom modes", { error: errorMessage }) + + // Try to recover from global state + try { + const fallbackModes = this.context.globalState.get("customModes", []) + logger.info("Recovered custom modes from global state", { count: fallbackModes.length }) + + // Update cache with fallback data + this.cachedModes = fallbackModes + this.cachedAt = Date.now() + + return fallbackModes + } catch (recoveryError) { + logger.error("Failed to recover custom modes from global state", { + error: recoveryError instanceof Error ? recoveryError.message : String(recoveryError) + }) + + // Return empty array as last resort + return [] } } - - // Combine modes in the correct order: project modes first, then global modes. - const mergedModes = [ - ...roomodesModes.map((mode) => ({ ...mode, source: "project" as const })), - ...settingsModes - .filter((mode) => !projectModes.has(mode.slug)) - .map((mode) => ({ ...mode, source: "global" as const })), - ] - - await this.context.globalState.update("customModes", mergedModes) - - this.cachedModes = mergedModes - this.cachedAt = now - - return mergedModes } public async updateCustomMode(slug: string, config: ModeConfig): Promise { @@ -404,7 +473,7 @@ export class CustomModesManager { // Validate the mode configuration before saving const validationResult = modeConfigSchema.safeParse(config) if (!validationResult.success) { - const errors = validationResult.error.errors.map((e) => e.message).join(", ") + const errors = validationResult.error.errors.map((e: any) => e.message).join(", ") logger.error(`Invalid mode configuration for ${slug}`, { errors: validationResult.error.errors }) throw new Error(`Invalid mode configuration: ${errors}`) } @@ -433,25 +502,43 @@ export class CustomModesManager { } await this.queueWrite(async () => { - // Ensure source is set correctly based on target file. - const modeWithSource = { - ...config, - source: isProjectMode ? ("project" as const) : ("global" as const), + try { + // Ensure source is set correctly based on target file. + const modeWithSource = { + ...config, + source: isProjectMode ? ("project" as const) : ("global" as const), + } + + await this.updateModesInFile(targetPath, (modes) => { + const updatedModes = modes.filter((m) => m.slug !== slug) + updatedModes.push(modeWithSource) + return updatedModes + }) + + // Refresh state before clearing cache to ensure consistency + await this.refreshMergedState() + this.clearCache() + + // Log successful update for debugging + logger.info(`Successfully updated custom mode: ${slug}`, { + source: modeWithSource.source, + targetPath + }) + } catch (writeError) { + // If file write fails, ensure cache is still cleared to prevent stale data + this.clearCache() + throw writeError } - - await this.updateModesInFile(targetPath, (modes) => { - const updatedModes = modes.filter((m) => m.slug !== slug) - updatedModes.push(modeWithSource) - return updatedModes - }) - - this.clearCache() - await this.refreshMergedState() }) } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error) logger.error("Failed to update custom mode", { slug, error: errorMessage }) + + // Clear cache on error to prevent inconsistent state + this.clearCache() + vscode.window.showErrorMessage(t("common:customModes.errors.updateFailed", { error: errorMessage })) + throw error // Re-throw to allow caller to handle } } @@ -487,18 +574,52 @@ export class CustomModesManager { } private async refreshMergedState(): Promise { - const settingsPath = await this.getCustomModesFilePath() - const roomodesPath = await this.getWorkspaceRoomodes() + try { + const settingsPath = await this.getCustomModesFilePath() + const roomodesPath = await this.getWorkspaceRoomodes() - const settingsModes = await this.loadModesFromFile(settingsPath) - const roomodesModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] - const mergedModes = await this.mergeCustomModes(roomodesModes, settingsModes) + logger.debug("Refreshing merged state", { settingsPath, roomodesPath }) - await this.context.globalState.update("customModes", mergedModes) + const settingsModes = await this.loadModesFromFile(settingsPath) + const roomodesModes = roomodesPath ? await this.loadModesFromFile(roomodesPath) : [] + const mergedModes = await this.mergeCustomModes(roomodesModes, settingsModes) - this.clearCache() + logger.debug("Merged modes loaded", { + settingsCount: settingsModes.length, + roomodesCount: roomodesModes.length, + mergedCount: mergedModes.length + }) - await this.onUpdate() + // Update global state with merged modes + await this.context.globalState.update("customModes", mergedModes) + + // Update cache with fresh data + this.cachedModes = mergedModes + this.cachedAt = Date.now() + + // Notify listeners of the update + await this.onUpdate() + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error) + logger.error("Failed to refresh merged state", { error: errorMessage }) + + // On error, clear cache to prevent stale data + this.clearCache() + + // Try to recover by loading from global state + try { + const fallbackModes = this.context.globalState.get("customModes", []) + logger.info("Recovered custom modes from global state", { count: fallbackModes.length }) + this.cachedModes = fallbackModes + this.cachedAt = Date.now() + } catch (recoveryError) { + logger.error("Failed to recover custom modes from global state", { + error: recoveryError instanceof Error ? recoveryError.message : String(recoveryError) + }) + } + + throw error + } } public async deleteCustomMode(slug: string): Promise { @@ -729,10 +850,10 @@ export class CustomModesManager { // Merge custom prompts if provided if (customPrompts) { - if (customPrompts.roleDefinition) exportMode.roleDefinition = customPrompts.roleDefinition - if (customPrompts.description) exportMode.description = customPrompts.description - if (customPrompts.whenToUse) exportMode.whenToUse = customPrompts.whenToUse - if (customPrompts.customInstructions) exportMode.customInstructions = customPrompts.customInstructions + if (customPrompts.roleDefinition) (exportMode as any).roleDefinition = customPrompts.roleDefinition + if (customPrompts.description) (exportMode as any).description = customPrompts.description + if (customPrompts.whenToUse) (exportMode as any).whenToUse = customPrompts.whenToUse + if (customPrompts.customInstructions) (exportMode as any).customInstructions = customPrompts.customInstructions } // Add rules files if any exist @@ -799,24 +920,24 @@ export class CustomModesManager { // Validate the mode configuration const validationResult = modeConfigSchema.safeParse(modeConfig) if (!validationResult.success) { - logger.error(`Invalid mode configuration for ${modeConfig.slug}`, { + logger.error(`Invalid mode configuration for ${(modeConfig as any).slug}`, { errors: validationResult.error.errors, }) return { success: false, - error: `Invalid mode configuration for ${modeConfig.slug}: ${validationResult.error.errors.map((e) => e.message).join(", ")}`, + error: `Invalid mode configuration for ${(modeConfig as any).slug}: ${validationResult.error.errors.map((e: any) => e.message).join(", ")}`, } } // Check for existing mode conflicts const existingModes = await this.getCustomModes() - const existingMode = existingModes.find((m) => m.slug === importMode.slug) + const existingMode = existingModes.find((m) => m.slug === (importMode as any).slug) if (existingMode) { - logger.info(`Overwriting existing mode: ${importMode.slug}`) + logger.info(`Overwriting existing mode: ${(importMode as any).slug}`) } // Import the mode configuration with the specified source - await this.updateCustomMode(importMode.slug, { + await this.updateCustomMode((importMode as any).slug, { ...modeConfig, source: source, // Use the provided source parameter }) @@ -827,13 +948,13 @@ export class CustomModesManager { // Always remove the existing rules folder for this mode if it exists // This ensures that if the imported mode has no rules, the folder is cleaned up - const rulesFolderPath = path.join(workspacePath, ".roo", `rules-${importMode.slug}`) + const rulesFolderPath = path.join(workspacePath, ".roo", `rules-${(importMode as any).slug}`) try { await fs.rm(rulesFolderPath, { recursive: true, force: true }) - logger.info(`Removed existing rules folder for mode ${importMode.slug}`) + logger.info(`Removed existing rules folder for mode ${(importMode as any).slug}`) } catch (error) { // It's okay if the folder doesn't exist - logger.debug(`No existing rules folder to remove for mode ${importMode.slug}`) + logger.debug(`No existing rules folder to remove for mode ${(importMode as any).slug}`) } // Only create new rules files if they exist in the import @@ -875,13 +996,13 @@ export class CustomModesManager { // Always remove the existing rules folder for this mode if it exists // This ensures that if the imported mode has no rules, the folder is cleaned up - const rulesFolderPath = path.join(globalRooDir, `rules-${importMode.slug}`) + const rulesFolderPath = path.join(globalRooDir, `rules-${(importMode as any).slug}`) try { await fs.rm(rulesFolderPath, { recursive: true, force: true }) - logger.info(`Removed existing global rules folder for mode ${importMode.slug}`) + logger.info(`Removed existing global rules folder for mode ${(importMode as any).slug}`) } catch (error) { // It's okay if the folder doesn't exist - logger.debug(`No existing global rules folder to remove for mode ${importMode.slug}`) + logger.debug(`No existing global rules folder to remove for mode ${(importMode as any).slug}`) } // Import the new rules files with path validation