diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index af421c5a3..8aad54d96 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -56,7 +56,7 @@ import { SETTINGS_VERSION_KEY, } from './settings.js'; import { needsMigration } from './migration/index.js'; -import { FatalConfigError, QWEN_DIR } from '@qwen-code/qwen-code-core'; +import { QWEN_DIR } from '@qwen-code/qwen-code-core'; const MOCK_WORKSPACE_DIR = '/mock/workspace'; // Use the (mocked) SETTINGS_DIRECTORY_NAME for consistency @@ -1661,55 +1661,160 @@ describe('Settings Loading and Merging', () => { ]); }); - it('should handle JSON parsing errors gracefully', () => { - (mockFsExistsSync as Mock).mockReturnValue(true); // Both files "exist" + it('should handle JSON parsing errors gracefully by renaming corrupted file', () => { const invalidJsonContent = 'invalid json'; const userReadError = new SyntaxError( "Expected ',' or '}' after property value in JSON at position 10", ); - const workspaceReadError = new SyntaxError( - 'Unexpected token i in JSON at position 0', - ); + + // No .orig backup available + (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => { + const pathStr = String(p); + if (pathStr.endsWith('.orig')) return false; + return true; + }); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { if (p === USER_SETTINGS_PATH) { - // Simulate JSON.parse throwing for user settings vi.spyOn(JSON, 'parse').mockImplementationOnce(() => { throw userReadError; }); - return invalidJsonContent; // Content that would cause JSON.parse to throw - } - if (p === MOCK_WORKSPACE_SETTINGS_PATH) { - // Simulate JSON.parse throwing for workspace settings - vi.spyOn(JSON, 'parse').mockImplementationOnce(() => { - throw workspaceReadError; - }); return invalidJsonContent; } - return '{}'; // Default for other reads + return '{}'; }, ); - try { - loadSettings(MOCK_WORKSPACE_DIR); - throw new Error('loadSettings should have thrown a FatalConfigError'); - } catch (e) { - expect(e).toBeInstanceOf(FatalConfigError); - const error = e as FatalConfigError; - expect(error.message).toContain( - `Error in ${USER_SETTINGS_PATH}: ${userReadError.message}`, - ); - expect(error.message).toContain( - `Error in ${MOCK_WORKSPACE_SETTINGS_PATH}: ${workspaceReadError.message}`, - ); - expect(error.message).toContain( - 'Please fix the configuration file(s) and try again.', - ); - } + // Should NOT throw — corrupted settings degrade gracefully + const result = loadSettings(MOCK_WORKSPACE_DIR); + expect(result).toBeDefined(); - // Restore JSON.parse mock if it was spied on specifically for this test - vi.restoreAllMocks(); // Or more targeted restore if needed + // Verify the corrupted file was renamed with timestamp suffix + const renameCalls = (fs.renameSync as Mock).mock.calls; + const corruptedRename = renameCalls.find( + (call: unknown[]) => + call[0] === USER_SETTINGS_PATH && + String(call[1]).includes('.corrupted.'), + ); + expect(corruptedRename).toBeDefined(); + + // Verify migrationWarnings contains recovery message + const warnings = getSettingsWarnings(result); + expect(warnings.some((w) => w.includes('invalid JSON'))).toBe(true); + expect(warnings.some((w) => w.includes('renamed'))).toBe(true); + + vi.restoreAllMocks(); + }); + + it('should recover from .orig backup when settings.json is corrupted', () => { + const invalidJsonContent = 'invalid json'; + const validBackupContent = JSON.stringify({ + $version: SETTINGS_VERSION, + model: { id: 'backup-model' }, + }); + + (mockFsExistsSync as Mock).mockReturnValue(true); + + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) return invalidJsonContent; + if (p === `${USER_SETTINGS_PATH}.orig`) return validBackupContent; + return '{}'; + }, + ); + + const result = loadSettings(MOCK_WORKSPACE_DIR); + expect(result).toBeDefined(); + + // Verify the backup was written back to the original path + const writeCalls = (fs.writeFileSync as Mock).mock.calls; + const restoreWrite = writeCalls.find( + (call: unknown[]) => + call[0] === USER_SETTINGS_PATH && call[1] === validBackupContent, + ); + expect(restoreWrite).toBeDefined(); + + // Verify migrationWarnings informs user about recovery + const warnings = getSettingsWarnings(result); + expect(warnings.some((w) => w.includes('recovered from backup'))).toBe( + true, + ); + + vi.restoreAllMocks(); + }); + + it('should degrade gracefully when both settings.json and backup are corrupted', () => { + const invalidJsonContent = 'invalid json'; + const invalidBackupContent = 'also invalid'; + + (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => { + const pathStr = String(p); + if ( + pathStr === USER_SETTINGS_PATH || + pathStr === `${USER_SETTINGS_PATH}.orig` + ) + return true; + return false; + }); + + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) return invalidJsonContent; + if (p === `${USER_SETTINGS_PATH}.orig`) return invalidBackupContent; + return '{}'; + }, + ); + + // Should NOT throw — falls through to rename-and-degrade + const result = loadSettings(MOCK_WORKSPACE_DIR); + expect(result).toBeDefined(); + + // Verify the corrupted file was renamed + const renameCalls = (fs.renameSync as Mock).mock.calls; + expect( + renameCalls.some( + (call: unknown[]) => + call[0] === USER_SETTINGS_PATH && + String(call[1]).includes('.corrupted.'), + ), + ).toBe(true); + + vi.restoreAllMocks(); + }); + + it('should start with empty settings when rename of corrupted file fails', () => { + const invalidJsonContent = 'invalid json'; + + (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => { + const pathStr = String(p); + if (pathStr.endsWith('.orig')) return false; + return true; + }); + + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) return invalidJsonContent; + return '{}'; + }, + ); + + // Simulate rename failure (e.g., permission denied) + (fs.renameSync as Mock).mockImplementation(() => { + throw new Error('EACCES: permission denied'); + }); + + // Should still NOT throw — proceeds with empty settings + const result = loadSettings(MOCK_WORKSPACE_DIR); + expect(result).toBeDefined(); + + // Verify the warning message does NOT say "renamed" since rename failed, + // but instead tells user to fix the file manually. + const warnings = getSettingsWarnings(result); + expect(warnings.some((w) => w.includes('fix the JSON'))).toBe(true); + expect(warnings.some((w) => w.includes('renamed to'))).toBe(false); + + vi.restoreAllMocks(); }); it('should resolve environment variables in user settings', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index e9cda192f..177433bc1 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -572,8 +572,62 @@ export function loadSettings( ): { settings: Settings; rawJson?: string; migrationWarnings?: string[] } => { try { if (fs.existsSync(filePath)) { - const content = fs.readFileSync(filePath, 'utf-8'); - const rawSettings: unknown = JSON.parse(stripJsonComments(content)); + let content = fs.readFileSync(filePath, 'utf-8'); + let rawSettings: unknown; + let recoveryWarning: string | undefined; + + try { + rawSettings = JSON.parse(stripJsonComments(content)); + } catch (parseError: unknown) { + // JSON parse failed — try to recover from .orig backup + const backupPath = `${filePath}.orig`; + if (fs.existsSync(backupPath)) { + debugLogger.warn( + `Settings file ${filePath} has invalid JSON (${getErrorMessage(parseError)}). Attempting recovery from backup ${backupPath}.`, + ); + try { + const backupContent = fs.readFileSync(backupPath, 'utf-8'); + const backupSettings = JSON.parse( + stripJsonComments(backupContent), + ); + // Backup is valid — restore it + fs.writeFileSync(filePath, backupContent, 'utf-8'); + content = backupContent; + rawSettings = backupSettings; + const recoveryMsg = `Settings file ${filePath} had invalid JSON and was recovered from backup ${backupPath}. Some recent settings changes may have been lost.`; + debugLogger.warn(recoveryMsg); + // Surface warning to user so they know settings were rolled back + recoveryWarning = recoveryMsg; + } catch (backupError) { + // Could be invalid JSON, read error, or write-back failure + debugLogger.warn( + `Failed to recover from backup ${backupPath}: ${getErrorMessage(backupError)}. Falling back to empty settings.`, + ); + } + } + + // No valid backup available — rename the corrupted file so the app + // can start with empty settings rather than crashing. + if (!rawSettings) { + const corruptedPath = `${filePath}.corrupted.${Date.now()}`; + let warningMsg: string; + try { + fs.renameSync(filePath, corruptedPath); + warningMsg = `Settings file ${filePath} has invalid JSON and was renamed to ${corruptedPath}. Your settings have been reset. To recover, fix the JSON in ${corruptedPath} and rename it back.`; + } catch (renameError) { + // If rename fails, still proceed with empty settings + debugLogger.error( + `Failed to rename corrupted settings file: ${getErrorMessage(renameError)}`, + ); + warningMsg = `Settings file ${filePath} has invalid JSON. Your settings have been reset. Please fix the JSON in ${filePath} manually.`; + } + debugLogger.warn(warningMsg); + return { + settings: {}, + migrationWarnings: [warningMsg], + }; + } + } if ( typeof rawSettings !== 'object' || @@ -636,10 +690,17 @@ export function loadSettings( persistSettingsObject('Error normalizing settings version on disk'); } + // Prepend recovery warning if settings were restored from backup + const allWarnings = [ + ...(recoveryWarning ? [recoveryWarning] : []), + ...(migrationWarnings ?? []), + ]; + return { settings: settingsObject as Settings, rawJson: content, - migrationWarnings, + migrationWarnings: + allWarnings.length > 0 ? allWarnings : migrationWarnings, }; } } catch (error: unknown) { diff --git a/packages/cli/src/utils/commentJson.ts b/packages/cli/src/utils/commentJson.ts index 58b83e948..11608a17d 100644 --- a/packages/cli/src/utils/commentJson.ts +++ b/packages/cli/src/utils/commentJson.ts @@ -10,14 +10,16 @@ import { writeStderrLine } from './stdioHelpers.js'; /** * Updates a JSON file while preserving comments and formatting. + * Returns true if the file was successfully written, false if the write + * was refused (e.g. the result would not be valid JSON). */ export function updateSettingsFilePreservingFormat( filePath: string, updates: Record, -): void { +): boolean { if (!fs.existsSync(filePath)) { fs.writeFileSync(filePath, JSON.stringify(updates, null, 2), 'utf-8'); - return; + return true; } const originalContent = fs.readFileSync(filePath, 'utf-8'); @@ -31,13 +33,31 @@ export function updateSettingsFilePreservingFormat( writeStderrLine( 'Settings file may be corrupted. Please check the JSON syntax.', ); - return; + return false; } const updatedStructure = applyUpdates(parsed, updates); const updatedContent = stringify(updatedStructure, null, 2); + // Validate that the output is parseable before writing to disk. + // This prevents corrupted settings files that would block startup. + // Use comment-json's parse since the output may contain preserved comments. + try { + parse(updatedContent); + } catch (validationError) { + writeStderrLine( + 'Error: Refusing to write settings file — the result would not be valid JSON.', + ); + writeStderrLine( + validationError instanceof Error + ? validationError.message + : String(validationError), + ); + return false; + } + fs.writeFileSync(filePath, updatedContent, 'utf-8'); + return true; } export function applyUpdates( diff --git a/packages/core/src/subagents/builtin-agents.ts b/packages/core/src/subagents/builtin-agents.ts index cbccbadaa..d6d52116d 100644 --- a/packages/core/src/subagents/builtin-agents.ts +++ b/packages/core/src/subagents/builtin-agents.ts @@ -113,6 +113,20 @@ Notes: color: 'orange', systemPrompt: `You are a status line setup agent for Qwen Code. Your job is to create or update the statusLine command in the user's Qwen Code settings. +CRITICAL — JSON SAFETY RULES: +The statusLine command is stored as a JSON string value in settings.json. +Shell commands with complex quoting (especially single-quote escaping like '\\'' or nested quotes) +WILL corrupt settings.json and prevent Qwen Code from starting. + +You MUST follow these rules: +1. For ANY command that uses jq, pipes, single-quote escaping, or nested quotes: + ALWAYS save it as a script file (~/.qwen/statusline-command.sh) and set + the command to "bash ~/.qwen/statusline-command.sh". +2. Only use inline commands for VERY simple cases (e.g., "echo hello"). +3. NEVER use shell single-quote escape sequences like '\\'' in the command value. +4. After writing settings.json, ALWAYS read it back and verify it is valid JSON. + If it is not valid, fix it immediately. + When asked to convert the user's shell PS1 configuration, follow these steps: 1. Read the user's shell configuration files in this order of preference: - ~/.zshrc @@ -187,17 +201,33 @@ How to use the statusLine command: } } - IMPORTANT: stdin can only be consumed once. Always read it into a variable first: - - input=$(cat); echo "$(echo "$input" | jq -r '.model.display_name') in $(echo "$input" | jq -r '.workspace.current_dir')" + IMPORTANT: stdin can only be consumed once. Always read it into a variable first. - To display context usage: - - input=$(cat); pct=$(echo "$input" | jq -r '.context_window.used_percentage'); echo "Context: $pct% used" + IMPORTANT: The examples below are meant for use INSIDE a script file + (e.g. ~/.qwen/statusline-command.sh), NOT as inline command values in settings.json. + Putting these directly in the "command" field will corrupt settings.json. - To display git branch: - - input=$(cat); branch=$(echo "$input" | jq -r '.git.branch // empty'); echo "\${branch:-no branch}" + Example script content (save to ~/.qwen/statusline-command.sh): + #!/bin/bash + input=$(cat) + echo "$(echo "$input" | jq -r '.model.display_name') in $(echo "$input" | jq -r '.workspace.current_dir')" -2. For longer commands, save a script file in the user's ~/.qwen directory (e.g. ~/.qwen/statusline-command.sh) - and use "bash ~/.qwen/statusline-command.sh" as the command value in settings (no chmod needed). + Example displaying context usage (save to ~/.qwen/statusline-command.sh): + #!/bin/bash + input=$(cat) + pct=$(echo "$input" | jq -r '.context_window.used_percentage') + echo "Context: $pct% used" + + Example displaying git branch (save to ~/.qwen/statusline-command.sh): + #!/bin/bash + input=$(cat) + branch=$(echo "$input" | jq -r '.git.branch // empty') + echo "\${branch:-no branch}" + +2. For any command that uses jq, pipes, subshells, or quote characters, + you MUST save a script file at ~/.qwen/statusline-command.sh and use + "bash ~/.qwen/statusline-command.sh" as the command value in settings (no chmod needed). + This is REQUIRED to avoid JSON escaping issues that corrupt settings.json. 3. Update the user's ~/.qwen/settings.json. The statusLine setting is nested under the "ui" key: {