fix(cli): preserve comments and formatting in settings.json during migration write-back#3861
Conversation
be9ce3c to
d9c8eb5
Compare
- BerriAI/litellm#27266 handle response.incomplete in Responses->Chat transform [merge-after-nits] - BerriAI/litellm#27259 add module docstring + regression test for render smoke [merge-as-is] - google-gemini/gemini-cli#26559 implement OIDC auth provider for A2A remote agents [merge-after-nits] - QwenLM/qwen-code#3861 preserve comments via comment-json on settings migration [merge-after-nits]
…gration
The persistSettingsObject() helper in loadAndMigrate() previously used
writeWithBackupSync() with raw JSON.stringify(), which stripped all comments
and custom formatting from users' settings.json on every migration or version
normalization.
Fix: Refactored updateSettingsFilePreservingFormat() in commentJson.ts with:
1. Sync mode (sync=true): Removes keys from the original file that are not
present in the migrated object, preventing zombie keys from persisting
after migrations that remove deprecated settings.
2. Atomic writes: Replaced fs.writeFileSync with a writeFileSyncAtomic()
helper that uses temp-file + rename for crash-safe writes. This applies
to both the migration path (persistSettingsObject) and the runtime
setValue path (saveSettings).
3. Comment preservation: Uses comment-json's parse() during the load phase,
so comment metadata is retained in the parsed structure and preserved by
stringify() during write-back. Keys that exist in both the original file
and the migrated object keep their original comments.
persistSettingsObject now calls updateSettingsFilePreservingFormat with
{sync: true} to get all three guarantees: comment preservation, zombie key
removal, and atomic writes.
The test file updates (settings.test.ts, commentJson.test.ts) from the
previous iteration are reverted since the write mechanism now goes through
the same writeFileSyncAtomic path.
Fixes QwenLM#3843
The sync mode in updateSettingsFilePreservingFormat only removed top-level zombie keys. Nested deprecated keys (e.g. general.disableAutoUpdate) survived because applyUpdates performed a deep merge without removing keys absent from the migrated object. Fix: Added a sync parameter to applyUpdates() that recursively deletes keys not present in the updates object at every nesting level. This ensures deprecated keys like general.disableAutoUpdate are properly cleaned up during V2→V3 migration write-back. Added 3 new tests: - Nested zombie keys removed in sync mode - Top-level zombie keys removed in sync mode - Unrelated keys in nested objects preserved during sync
84e7de0 to
2d1afcb
Compare
| @@ -56,16 +89,94 @@ export function updateSettingsFilePreservingFormat( | |||
| return false; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[Critical] writeFileSyncAtomic completely duplicates the existing writeWithBackupSync logic in packages/cli/src/utils/writeWithBackup.ts (temp write → backup target → atomic rename). Compared to the original implementation, the new function lacks directory type checks, detailed error reporting on recovery failure, and silently swallows recovery errors.
The codebase now has two ~80-line atomic-write implementations that need independent maintenance — a bugfix in one won't propagate to the other. Recommend deleting writeFileSyncAtomic and importing/using writeWithBackupSync directly:
| import { writeWithBackupSync } from './writeWithBackup.js'; | |
| // Replace writeFileSyncAtomic(filePath, content) with: | |
| writeWithBackupSync(filePath, content); |
— deepseek-v4-pro via Qwen Code /review
| @@ -732,10 +731,20 @@ export function loadSettings( | |||
|
|
|||
| const persistSettingsObject = (warningPrefix: string) => { | |||
| try { | |||
There was a problem hiding this comment.
[Critical] The PR added an if (!written) branch to log an error when updateSettingsFilePreservingFormat returns false, but no test in settings.test.ts triggers the written=false path.
The new error-handling path is completely unverified. If updateSettingsFilePreservingFormat returns false during migration due to a validation failure, whether the log behavior is correct and loadSettings exits properly is unknown.
Suggested fix: Add a mocked-fs test that makes updateSettingsFilePreservingFormat return false during migration, and assert that debugLogger.error is called.
— deepseek-v4-pro via Qwen Code /review
| @@ -27,21 +58,23 @@ export function updateSettingsFilePreservingFormat( | |||
| let parsed: Record<string, unknown>; | |||
| try { | |||
| parsed = parse(originalContent) as Record<string, unknown>; | |||
There was a problem hiding this comment.
[Suggestion] The specific error details are discarded on parse errors. The old code output error.message (e.g. "Unexpected token / in JSON at position 143"), while the new code only outputs the generic "Settings file may be corrupted.". Users lose the specific location information for the corrupted JSON and must blindly restore backups or reset all settings.
| parsed = parse(originalContent) as Record<string, unknown>; | |
| } catch (_error) { | |
| writeStderrLine('Error parsing settings file.'); | |
| writeStderrLine( | |
| `Settings file may be corrupted: ${ | |
| _error instanceof Error ? _error.message : String(_error) | |
| }`, | |
| ); | |
| return false; | |
| } |
— deepseek-v4-pro via Qwen Code /review
| @@ -8,17 +8,48 @@ import * as fs from 'node:fs'; | |||
| import { parse, stringify } from 'comment-json'; | |||
| import { writeStderrLine } from './stdioHelpers.js'; | |||
There was a problem hiding this comment.
[Suggestion] The UpdateSettingsOptions interface contains only a single optional boolean field (sync?: boolean). The codebase convention (e.g. WriteWithBackupOptions) only uses an options interface when there are multiple configuration items. A single boolean adds unnecessary indirection — callers must wrap the boolean in an object literal ({ sync: true }).
Suggested fix: Accept a boolean parameter directly, or use a mode: 'merge' | 'sync' enum (which communicates intent more explicitly). If writeFileSyncAtomic is removed and writeWithBackupSync is reused, this parameter can be removed entirely — atomic writes become the default behavior.
— deepseek-v4-pro via Qwen Code /review
| @@ -80,6 +191,7 @@ export function applyUpdates( | |||
| result[key] = applyUpdates( | |||
| result[key] as Record<string, unknown>, | |||
| value as Record<string, unknown>, | |||
There was a problem hiding this comment.
[Suggestion] The edge case of sync=true with updates={} (empty object) is untested. Currently all keys are treated as zombie keys and deleted, resulting in {}, but this behavior is neither tested nor documented. If a migration inadvertently passes an empty migration output, the entire settings file would be silently wiped.
Suggested fix: Add a test to establish the expected semantics: expect(applyUpdates({ a: 1, b: { c: 2 } }, {}, true)).toEqual({}).
— deepseek-v4-pro via Qwen Code /review
…vation
- Replace writeFileSyncAtomic with writeWithBackupSync from writeWithBackup.ts
to eliminate duplicate atomic-write implementations
- Simplify UpdateSettingsOptions interface to a plain boolean `sync` parameter
- Include parse error details in stderr output (error.message with position info)
- Add test for written=false path in settings.test.ts
- Add test for sync=true with empty updates={} documenting zombie key removal
- Mock statSync in settings test to support writeWithBackupSync directory check
- Add mockDebugLogger with debug/warn/error/info to settings.test.ts - Enhance existing written=false test to assert debugLogger.error is called - All 96 settings tests + 15 commentJson tests pass
|
Hi @wenshao — I have addressed all 5 review comments in the latest two commits. Here is the mapping:
CI is green across all platforms. Could you take another look? |
|
This PR is green and narrowly scoped to preserving comments/formatting during settings migration write-back. All 5 previous review comments have been addressed in the latest commits. Requesting maintainer review when available. |
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — DeepSeek/deepseek-v4-pro via Qwen Code /review
|
Reviewed the latest revision — the original 5 inline threads are all addressed:
Scope is clean for — Reviewed with Claude Code (Opus 4.7, 1M context) |
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
The fix correctly routes the migration write-back through the format-preserving helper, and the two follow-up commits cleanly resolved the prior cleanup items (helper consolidation, JSDoc, restored parse-error message, and a new test for the "write refused" branch). Approving — two non-blocking observations below for follow-up.
1. Heads-up: runtime saves now break symlinks and drop file mode (severity: medium · confidence: very high)
Switching the shared format-preserving write helper to a temp-file + rename strategy changes behavior for the runtime save path, not just the migration path. Previously, saving a setting wrote through the existing file, which preserves both symlinks and the file's mode bits. Now every save renames the existing settings.json aside as .orig and renames a freshly created temp file into its place. Two consequences for a class of users that didn't have these problems before this PR:
- If
~/.qwen/settings.jsonis symlinked into a dotfiles repo, the first runtime save replaces the symlink with a regular file — subsequent edits no longer flow back to the dotfiles repo, and the user has to manually restore the symlink. - If the user chmodded the file to
0600because it can carry MCPauthorizationheaders or API tokens, the new file inherits the process umask (typically0644). Permissions are silently loosened.
Possibly an intentional tradeoff for atomicity — flagging in case it isn't.
2. Sync-mode pruning matches inherited prototype keys (severity: low · confidence: very high)
The prune step in sync mode decides what to delete with key in updates, which walks the prototype chain. Any current-file key whose name collides with Object.prototype — constructor, toString, hasOwnProperty, __proto__ — is treated as "present" in every updates object, including the documented {} "wipe everything" case, so keys with those names will not be pruned. Pathological for normal settings, but Object.hasOwn(updates, key) matches the supplied object exactly.
Verdict
APPROVE — fix is correct and the cleanup commits landed cleanly; observations above are non-blocking and can be follow-ups if you decide they're worth addressing.
Summary
Why
Fixes #3843. When starting qwen, the `settings.json` configuration file was being completely rewritten with `JSON.stringify(settingsObject, null, 2)` during migration or version normalization. This stripped all user comments and custom formatting from the file on every affected startup.
The `saveSettings()` path (used for runtime `setValue`) already correctly used `updateSettingsFilePreservingFormat()` from the `comment-json` library, but the migration write-back path did not.
What Changed
Validation
Scope
Risk / Compatibility Notes