feat(cli): Add settings JSON corrupted warning dialog#4560
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] 3 existing tests in settings.test.ts fail because they assert the old renameSync + .corrupted.<timestamp> behavior:
"should handle JSON parsing errors gracefully by renaming corrupted file"— assertsrenameSynccalled with.corrupted.pattern"should degrade gracefully when both settings.json and backup are corrupted"— samerenameSyncassertion"should start with empty settings when rename of corrupted file fails"— mocksrenameSyncto throw, asserts warning contains"fix the JSON"(old message)
All three must be rewritten to assert copyFileSync (not renameSync), the new .corrupted path (no timestamp), and the new warning text ("Your settings have been reset.").
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test failures (10 tests, 3 CI checks):
App.test.tsx: 7 tests crash —useSettings()added toAppbutrenderWithProviderslacksSettingsContext.Provider(see existing inline comment at App.tsx:20).settings.test.ts: 3 tests fail — assertions still referencefs.renameSync+.corrupted.<timestamp>pattern, but PR switched tofs.copyFileSync+ fixed.corruptedsuffix. Affected tests: "should handle JSON parsing errors gracefully" (~line 1884), "should degrade gracefully when both settings.json and backup are corrupted" (~line 1965), "should start with empty settings when rename of corrupted file fails" (~line 1998).
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No new Critical findings. 7 new Suggestions below (in addition to R1/R2 comments). Downgraded from Approve to Comment: CI failing (Test on macos-latest, windows-latest, ubuntu-latest). — qwen3.7-max via Qwen Code /review
|
E2E 验证报告 — PR #4560环境：Debian GNU/Linux 13 (trixie) · Linux 6.12 x86_64 · Node v22.22.2 · tmux 3.5a · 新 worktree 为避免污染本机真实的 1. 单元 / typecheck
2. E2E 场景CLI 入口： 2.1 Scenario A — 用户 settings 损坏，无
|
| 文件 | 大小 | 内容 | 期望 |
|---|---|---|---|
settings.json |
99 | 备份恢复后的合法 JSON | ✅ |
settings.json.corrupted |
25 | 原始损坏内容（保留供用户排查） | ✅ |
settings.json.orig |
99 | 备份原封不动 | ✅ |
env var 跨子进程透传是这个 case 的隐式断言：父进程 loadSettings() 已经把 settings.json 写回合法内容，relaunchAppInChildProcess() fork 出来的子进程 JSON.parse(stripJsonComments(content)) 一定成功 —— 此时对话框还能出现，只能是 因为 QWEN_CODE_SETTINGS_CORRUPTED_PATH / QWEN_CODE_SETTINGS_WAS_RECOVERED 这两个 env var 把损坏标记带过了 fork 边界（settings.ts L952-960）。这条路径 == PR 的核心修复。✅
2.3 Scenario C — Exit 路径：还原损坏文件 + exit 1
承接 Scenario B 状态（settings.json 已是 99 bytes 恢复内容），在 dialog 中按 Enter（默认选中 Exit）：
$ echo "EXIT_CODE=$?"
EXIT_CODE=1 ✅
$ ls -la $QWEN_HOME/settings.json*
-rw-r--r-- 25 settings.json ← 被 .corrupted 覆盖回原始损坏内容 ✅
-rw-r--r-- 25 settings.json.corrupted ← 仍保留 ✅
-rw-r--r-- 99 settings.json.orig ← 不动 ✅
$ cat $QWEN_HOME/settings.json
this is NOT valid JSON { ✅
App.tsx L36-52 的语义就是这条："Exit and restore corrupted file" = 把 .corrupted 拷回 settings.json，让下次启动用户能在原位置看到自己写错的内容，然后 process.exit(1)。
2.4 Scenario D — Continue 路径在 Scenario A 已覆盖
承接 A 中按 Esc 后：
.corrupted文件被删除 ✅dismissed=true之后正常渲染 DefaultAppLayout（Provider 选择界面）✅
2.5 Scenario E — workspace 级别 settings 损坏 不应 弹对话框
设置：
$QWEN_HOME/settings.json= 合法 JSON$CWD/.qwen/settings.json= "this is WORKSPACE NOT valid JSON {"
启动后：
（没有红框对话框）
╭─ Warning: Settings file .../scenarioE/work/.qwen/settings.json has invalid JSON. Your settings have been reset. ─╮
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
（继续进入 Provider 选择）
- 红框
SettingsCorruptedDialog未 出现 ✅ - workspace 的
.qwen/settings.json.corrupted被创建 ✅（recovery 逻辑对任意 scope 都跑，磁盘文件仍然保留） - 只在
settings.tsL1180-1181 把userResult.corruptedPath抽出来透传到LoadedSettings，所以非 User scope 不会触发 dialog（仅经标准 Notification 系统提示一次 warning）✅
2.6 Sanity — 二次启动不应再弹
承接 B 之后手动把 settings.json 改为合法、删掉 .corrupted，再次 node $QWEN_CLI：
$ env | grep QWEN_CODE_SETTINGS
（空） ← env var 没遗留 ✅
$ node $QWEN_CLI
（直接进入 Provider 选择，无对话框） ← 干净启动 ✅
证明 settings.ts L958-959 delete process.env['QWEN_CODE_SETTINGS_CORRUPTED_PATH'] 的清理是工作的，不会在合法启动时误触发。
3. 小问题（不阻塞 merge）
PR 描述与代码行为不一致：PR 描述写
ESC/Ctrl+C maps to the first option (Exit), following convention
但 SettingsCorruptedDialog.tsx L31-38、对应单元测试 L174 + L197、以及对话框自带的 Continue with ... (esc) 文案三处都是：
- ESC → onContinue
- Ctrl+C → onExit
代码 + 测试 + UI 文案三者一致，建议把 PR 描述中的这一句改成 "ESC continues with recovered/empty settings; Ctrl+C exits and restores the corrupted file"，避免后续 reviewer 按字面意思去读源码时被绊一下。
另外两个 PR 自述的 TODO 仍然是 follow-up：i18n 注册 + 文档更新；本次未在测试范围内。
4. 结论
- 单元：123 / 123 通过，typecheck clean
- 6 个 E2E 场景（A 无备份、B 有备份的核心 fix、C Exit 还原、D Continue 清理、E workspace 不弹、Sanity 二次启动）全部通过
- 关键不变量已落实：
fs.copyFileSync而不是renameSync——.corrupted文件磁盘可见 ✅- env var 跨
relaunchAppInChildProcessfork 边界透传 ✅ - 子进程消费后立刻
deleteenv var，避免后续 loadSettings 误触发 ✅ - 仅 User scope 触发 dialog，workspace 走 Notification ✅
- Exit/Continue 双路径磁盘副作用与代码一致 ✅
- 唯一遗留：PR 描述里 ESC/Ctrl+C 语义那句与代码相反，建议在 merge 前更正文案
- 跨平台留白：Linux x86_64 / Node 22 已覆盖
- 从 maintainer 视角，建议 merge（文案勘误可以放在合并后小 patch 处理）
完整测试痕迹：tmux session pr4560（3 windows），磁盘工件保留在 /tmp/pr4560-tests/。
Co-authored-by: Shaojin Wen <[email protected]>
Co-authored-by: Shaojin Wen <[email protected]>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional note (not inline due to overlap): App.tsx:51 — process.exit(1) uses error exit code for a user-initiated recovery action. Consider process.exit(0) since the user explicitly chose to exit. — qwen3.7-max via Qwen Code /review
|
@wenshao Critical items have all been addressed. Suggestions are partially addressed. Fixed. Removed the filter in Notifications.tsx since the corruption warning is no longer added to migrationWarnings when the dialog is shown, so there's nothing to filter. Also added truncateToItem to the mock. For the env var issue - added !corruptedSaved guard so the child process doesn't trigger a normalization write after recovering from corruption. Env vars are cleaned up after startInteractiveUI and in loadSettings after being read. Added tests for the env var propagation. |
Co-authored-by: Shaojin Wen <[email protected]>
…view update Co-authored-by: Shaojin Wen <[email protected]>
|
@wenshao Resolved the merge conflicts and cleaned up the comment. Could you please take another look? I'll re-request review now. |
| : '0'; | ||
| } | ||
| await cleanupCheckpoints(); | ||
| // Performance checkpoint |
There was a problem hiding this comment.
[Critical] Env vars pre-consumed by validateAuthMethod() before sandbox launches.
When sandbox is configured, validateAuthMethod() (line ~507) internally calls loadSettings() (auth.ts:169), whose env-var propagation block (settings.ts:960-966) reads and deletes both QWEN_CODE_SETTINGS_CORRUPTED_PATH and QWEN_CODE_SETTINGS_WAS_RECOVERED from process.env. By the time start_sandbox() runs (~line 591), the corruption markers are gone — the sandbox child process inherits clean env vars, re-reads the already-recovered settings.json, and never shows the corruption dialog.
This affects the common case: sandbox configured + auth type explicitly set + useExternal: false. The non-sandbox path (relaunchAppInChildProcess) works correctly because no intermediate loadSettings() call occurs between env var setup and child fork.
Suggested fix: Move the env var setup to immediately before each child-launch call site (start_sandbox and relaunchAppInChildProcess), or make loadSettings() accept a skipEnvVarPropagation parameter that intermediate callers like validateAuthMethod can pass.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed. Added consumeCorruptionEnvVars optional parameter to loadSettings() (defaults true, no behavior change for existing callers). validateAuthMethod() passes false so it doesn't consume the corruption env vars before start_sandbox() inherits them. Added test coverage for both the new parameter and the scope guard.
| return { | ||
| settings: {}, | ||
| migrationWarnings: [warningMsg], | ||
| corruptedPath: corruptedSaved ? corruptedPath : undefined, |
There was a problem hiding this comment.
[Suggestion] No-backup path emits corruption warning through BOTH the dialog AND migrationWarnings.
migrationWarnings: [warningMsg] is set unconditionally alongside corruptedPath. In interactive mode, the dialog renders (conveying "Your settings file had invalid JSON"), and after dismissal the same message reappears as a startup notification via getSettingsWarnings() → startupWarnings → Notifications.
This is inconsistent with the backup-recovery path (line ~1046), which does NOT include the recovery message in migrationWarnings.
Suggested fix: Filter corruption-specific warnings from startupWarnings in gemini.tsx when the dialog will render:
const filteredWarnings = settings.corruptedPath
? startupWarnings.filter((w) => !w.includes('has invalid JSON'))
: startupWarnings;Or restructure the no-backup return to separate the corruption warning from migration warnings.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Done. Corruption warnings now go through corruptedPath only, with early stderr emitting directly from the startup path in gemini.tsx.
| }); | ||
|
|
||
| it('should only consume env vars for SettingScope.User', () => { | ||
| (mockFsExistsSync as Mock).mockImplementation( |
There was a problem hiding this comment.
[Suggestion] This test doesn't exercise the scope === SettingScope.User guard.
The test doesn't set QWEN_CODE_SETTINGS_CORRUPTED_PATH or QWEN_CODE_SETTINGS_WAS_RECOVERED in process.env — it only mocks existsSync for the workspace path and asserts corruptedPath is undefined. Since no env vars are present, the scope guard at settings.ts:961 (if (envCorruptedPath && scope === SettingScope.User)) is never exercised. A bug that removed the scope check would not be caught.
Suggested fix: Set the env vars before calling loadSettings(), then verify user scope consumed them while workspace scope did not:
process.env['QWEN_CODE_SETTINGS_CORRUPTED_PATH'] = '/test/path.corrupted';
process.env['QWEN_CODE_SETTINGS_WAS_RECOVERED'] = '1';
const result = loadSettings(MOCK_WORKSPACE_DIR);
expect(result.corruptedPath).toBeDefined(); // user scope consumed
expect(process.env['QWEN_CODE_SETTINGS_CORRUPTED_PATH']).toBeUndefined(); // deleted— qwen3.7-max via Qwen Code /review
|
Fixed a security vulnerability where
Critical issues have been addressed. Multiple Suggestions have been implemented as well. Could you take another look when convenient? |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R12b Critical (env var injection via .qwen/.env) is fully resolved with a sound two-layer defense: (1) ENV_CORRUPTED_PATH/ENV_WAS_RECOVERED added to PROJECT_ENV_HARDCODED_EXCLUSIONS, (2) envCorruptedPath === corruptedPath equality check ensures the env var value matches the locally-computed path. Ordering is correct — loadAndMigrate consumes env vars before loadEnvironment reads project .env files.
The App.tsx error-handling refactor (split try/catch for copyFileSync/unlinkSync, stderr logging in onContinue) preserves exit semantics and improves observability.
One low-confidence suggestion (terminal-only, not blocking): consider adding a negative test asserting that a mismatched ENV_CORRUPTED_PATH value is rejected by the equality check — the positive path is covered but the new guard branch lacks negative coverage.
— qwen3.7-max via Qwen Code /review
|
@wenshao Thanks for the detailed review and approval! |
|
R13 fixed: 1: pulled 2: added a comment on the needsMigration branch in 3: added negative test for mismatched |
|
@wenshao I’ve addressed the latest feedback and pushed an update. Could you take another look when convenient? |
Maintainer Verification ReportReviewer: wenshao Test ResultsFocused tests (PR-touched files):
Full
Build & Typecheck
Code Review NotesCorrectness observations:
UI behavior:
Potential improvement (non-blocking):
VerdictPASS — Ready to merge. All PR-introduced code passes tests, build/typecheck clean, env var security hardened, no regressions. |
…spawn Co-Authored-By: qwen3.7-max <[email protected]>
|
the R14 fix commit (12f864f), I added a regression test to verify the this commit (b447572) only touches the test file, no code change. @wenshao Could you take another look when convenient? |
|
Hi @wenshao, I’ve pushed a commit to address the R14 comments, which automatically dismissed your previous approval. CI is green now, could you take another look when you have a chance? I’m happy to fix anything that still needs work. |
Co-authored-by: Shaojin Wen <[email protected]>
|
Applied the suggestion, thanks. Could you take another look? |
|
@wenshao Thanks for your approval! If needed, could you help invite another reviewer for this PR?🙏 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM overall. The env-var propagation across relaunchAppInChildProcess() is a clean solution to the parent/child state-loss problem.
A couple of non-blocking follow-ups for later:
- i18n: the
t()calls are passthrough right now — register the locale keys when convenient - Test isolation: the cross-test stdin pollution between
SettingsCorruptedDialog.test.tsxandApp.test.tsxis worth a follow-up fix to avoid flakiness in future CI matrix expansions
Nice work on the E2E coverage 👍 @zzhenyao
|
@yiliang114 Appreciate the approval!🙏 |
|
@tanzhenxin, @pomelo-nwu Hi, is issue #2136 still on the roadmap? I noticed it's currently in the "To Do" column on the project board. I'd like to pick it up.
|
3 participants
Summary
When the user-level
settings.jsoncontains invalid JSON, the CLI silently loads backup settings or enters the initial settings screen. This PR adds a fault-tolerant mechanism: automatic settings recovery + a UI warning dialog.Fixes #4448
Problem Discovery
settings.jsonand starting the CLI caused the file to be silently overwritten without user notificationloadSettings()revealed that on JSON parse failure, the corrupted file is renamed (moved) and recovery from.origbackup is attemptedfs.renameSyncwhich moves the original file — it should usefs.copyFileSyncto preserve a copy for the user to inspectrelaunchAppInChildProcess()forks the child process, and the child re-reads settings.json (already repaired by the parent), causing the corruption marker to be lost.origbackup the dialog shows normally (child also detects the error), with backup the dialog does not appear (parent already fixed the file)Solution
Core Idea
Pass corruption state between parent and child processes via environment variables, avoiding reliance on filesystem state. Skipping
relaunchAppInChildProcess()would affect existing logic, so this approach was not adopted.Changes
1. Recovery Logic (
settings.ts)fs.renameSync→fs.copyFileSync: preserve a copy of the corrupted file (.corruptedsuffix)corruptedPathandwasRecoveredfields toLoadedSettings2. Environment Variable Propagation (
gemini.tsx)settings.corruptedPathis detected, setQWEN_CODE_SETTINGS_CORRUPTED_PATHandQWEN_CODE_SETTINGS_WAS_RECOVEREDloadSettings()and afterstartInteractiveUI()to prevent stale state propagating to child processes3. UI Dialog (
App.tsx+SettingsCorruptedDialog.tsx)<Box marginX={2}>container, consistent with DialogManager's parent in DefaultAppLayoutDesign Decisions
marginLeft={1}, green selection, red title/border)Test Results
npm run preflightTest Files 47 passed (47) Tests 419 passed | 1 skipped (420) Start at 08:10:42 Duration 6.04s (transform 5.27s, setup 0ms, collect 49.98s, tests 2.56s, environment 3.30s, prepare 4.72s)Screenshots
with backup (.orig exists)
without backup, dialog shown
without backup, after selecting "Continue"
TODO
settings.corruptedUI translation