mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-04-29 12:11:09 +00:00
fix: prevent statusline script from corrupting settings.json (#3091)
* fix: prevent statusline script from corrupting settings.json Some models generate shell commands with complex quoting (e.g. single-quote escaping like '\'') that break JSON syntax when written to settings.json, causing qwen-code to fail to start with a FatalConfigError. This adds four layers of defense: 1. **Agent prompt** (builtin-agents.ts): Require commands using jq/pipes/quotes to be saved as script files instead of inline in settings.json. Mark examples as script-only to prevent models from copying them inline. 2. **Write validation** (commentJson.ts): Validate JSON output before writing to disk in updateSettingsFilePreservingFormat. 3. **Startup recovery** (settings.ts): When settings.json has invalid JSON, try .orig backup first, then degrade gracefully to empty settings instead of crashing. Rename corrupted file to .corrupted for manual recovery. Show warning to user via migrationWarnings. 4. **Test update** (settings.test.ts): Update test to verify graceful degradation behavior instead of expecting FatalConfigError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on statusline JSON corruption fix 1. Backup recovery now surfaces warning via migrationWarnings (reviewer: P2 correctness) 2. Corrupted file uses timestamped suffix to avoid overwriting (reviewer: P2 robustness) 3. Remove misleading underscore prefix on used catch variable (reviewer: P2 code quality) 4. updateSettingsFilePreservingFormat returns boolean (reviewer: P2 correctness) 5. Add 3 new tests: backup recovery, both-corrupted, rename-failure (reviewer: P2 testing) 6. Consistent shebang lines in agent prompt examples (reviewer: P3 nit) 7. Improve catch block error message for backup recovery (reviewer: P2 correctness) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: warningMsg says "renamed" even when rename fails Move warningMsg construction after renameSync so the message accurately reflects the outcome: "renamed to X" on success, "fix manually" on failure. Add assertion to rename-failure test verifying the fallback message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Shaojin Wen
GitHub
parent
ec1787b846
commit
2ac099caaf
4 changed files with 263 additions and 47 deletions
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue