Verification: surface startup warnings on stderr before TUI render (PR #4461)

Verdict: ❌ FAIL — the PR's mechanical change is correct, but the user-facing fix it promises does not deliver. The corrupted-settings warning that the PR's description (and issue #4448) says will now appear on stderr does not appear in my runtime tests, because at the point the new for-loop runs startupWarnings is empty — the warning isn't propagating from the settings loader to that array in real runs. The new for-loop only delivers warnings that are already in startupWarnings, and the corruption warning isn't among them.

Claim (from PR): Before — TUI shows "Connect a provider" with no visible error. After — stderr shows Settings file ... has invalid JSON and was renamed to ... Your settings have been reset.

Method: Cold start. Branch fix/corrupted-settings-warning (HEAD 92bfe3728, 1 commit ahead of origin/main) into worktree /tmp/pr-4461. npm install produced dist/cli.js. Driven inside tmux -L pr4461 with sandbox HOME=/tmp/pr-4461-home containing a deliberately corrupted ~/.qwen/settings.json ({ not valid JSON }). CLI run from a clean cwd (/tmp/pr-4461-cwd-clean) so workspace settings don't interfere. stdout and stderr captured to separate files.

Steps

1. ❌ Non-interactive run with corrupted user settings. Created /tmp/pr-4461-home/.qwen/settings.json with { not valid JSON }, ran qwen -p "hello".

   • Corruption was detected: post-run the file had been renamed to settings.json.corrupted.1779585599506.
   • stderr (the PR's new channel):

     No auth type is selected. Please configure an auth type (e.g. via settings or `--auth-type`) before running in non-interactive mode.
     

   • stderr did NOT contain the corruption warning. The PR's "After:" claim is unfulfilled. (Auth error fires later in main() via validateNonInteractiveAuth; the PR's for-loop is upstream of that, so it ran before this error, yet no warning came out.)

2. ❌ Interactive TUI run. Same setup, ran qwen (no args). TUI rendered the "Connect a Provider" dialog. stderr (captured separately via 2> redirect) was empty. The TUI's Notifications component would render the warning IF it were in startupWarnings, but the prop arrives empty, so nothing is shown there either.

   • This is what the PR is supposed to fix — and the symptom is unchanged from the pre-PR description.

3. 🔍 Workspace-scope corruption (not user). Put the broken JSON at <cwd>/.qwen/settings.json instead. Same outcome: file renamed to .corrupted.<ts>; stderr only shows the no-auth error.

4. 🔍 Confirmed the PR's for-loop runs and is functional, via independent warning source. The PR's for (const warning of startupWarnings) writeStderrLine(warning) ran when I injected INJECTED_TEST_WARNING_via_warningfile into /tmp/qwen-code-warnings.txt (the path getStartupWarnings() reads from). With that file present, stderr showed:

   INJECTED_TEST_WARNING_via_warningfile
   No auth type is selected. ...
   

   ✅ So the for-loop and writeStderrLine chain work. The bug is purely that the corruption warning never reaches startupWarnings in real runtime.

5. 🔍 Direct runtime measurement. Instrumented dist/cli.js with a single sed insertion right before the new for-loop:

   process.stderr.write("XXX startupWarnings count=" + startupWarnings.length + " items=" + JSON.stringify(startupWarnings) + "\n");
   for (const warning of startupWarnings) { ... }

   Re-ran the corrupted-settings non-interactive scenario. Captured:

   XXX startupWarnings count=0 items=[]
   No auth type is selected. ...
   

   startupWarnings.length === 0 at the for-loop, even though the file rename absolutely confirms the corruption-detection branch ran (file → .corrupted.<ts>).

   Followed by bundle restoration; no leftover instrumentation.

Sample (one frame)

$ ls ~/.qwen/
settings.json                                # corrupted, will be renamed

$ HOME=~ qwen -p "hi"
[stderr]: No auth type is selected. ...      # <-- expected the corruption warning here
[stdout]: (empty)

$ ls ~/.qwen/
settings.json.corrupted.1779585599506        # rename confirms corruption detected

Findings

• ⚠️ The PR doesn't deliver its promised user-facing fix. Steps 1 and 2 reproduce the same "user sees nothing" scenario the PR description targets. The for-loop is mechanically correct (step 4), but the corruption warning isn't in startupWarnings to begin with (step 5). The bug is upstream of this PR's change.

• ⚠️ Where the propagation is breaking (suspected, didn't fully isolate): source-level trace looks fine — loadAndMigrate returns {settings: {}, migrationWarnings: [warningMsg]} from the corruption-rename branch (packages/core/src/config/settings.ts:907–909); allMigrationWarnings aggregates it (settings.ts:1088–1094); LoadedSettings.migrationWarnings stores it (settings.ts:406); getSettingsWarnings(settings) reads from it (settings.ts:335). The bundled chunk's code matches the source verbatim. Yet at runtime startupWarnings.length === 0. Something between loadSettings() returning at gemini.tsx:423 and getSettingsWarnings(settings) being called at gemini.tsx:761 is consuming or replacing the populated array. The existing unit test (settings.test.ts:1845 "should handle JSON parsing errors gracefully by renaming corrupted file") passes because it calls getSettingsWarnings(result) directly on the loadSettings return value — it doesn't exercise the real main() flow where the warning is supposed to propagate. So the unit test is not load-bearing evidence that the PR's claim works end-to-end.

• ⚠️ No .orig backup interference. My test deliberately did not create settings.json.orig, so the recovery branch (if (fs.existsSync(backupPath))) doesn't fire and migrationWarnings: [warningMsg] from the corruption-rename branch is what loadAndMigrate returns. Confirmed by .orig not appearing post-run.

• The independent control in step 4 (injecting via /tmp/qwen-code-warnings.txt) demonstrates the new for-loop works for warnings that DO reach startupWarnings. So this PR is non-zero value — any future fix to the propagation bug will benefit non-interactive users immediately. But shipping this PR alone won't close issue [Request] Improve what happens if settings.json is invalid #4448 from the user's perspective.

• Did not try with .orig backup recovery path (which fires a different "recoveryWarning" message). That code path lives in the same function and the recovery message goes through the same migrationWarnings array, so it's likely subject to the same propagation bug.

• Did not run the unmodified main branch CLI for comparison; the report's logic doesn't require it — the PR claims a difference vs main on stderr, and stderr post-PR is still empty for the targeted scenario.

Recommendation for the maintainer

Don't merge as-is. Either:

(a) Add a direct runtime test that reproduces the user-facing flow (broken ~/.qwen/settings.json, run qwen -p "...", assert stderr contains "invalid JSON"). That test will fail today, which is the actual evidence needed.

(b) Investigate why loadedSettings.migrationWarnings is empty at the point getSettingsWarnings(settings) is called from gemini.tsx:761, even though loadAndMigrate returns {migrationWarnings: [warningMsg]} and allMigrationWarnings includes it. Likely culprits to check: a LoadedSettings re-creation somewhere between line 423 and 745, or a migratedInMemorScopes / migrationWarnings constructor-arg swap (positions 6 and 7).

(c) Land the for-loop on its own (it's harmless and helps the OAuth-expiry and other startup-warning sources that DO populate the array), and open a separate issue for the corrupted-settings propagation bug — which would then justify the merge by acknowledging this PR doesn't close #4448 yet.

Environment: Linux x86_64, Node v22.22.2, npm 10.9.7, tmux 3.5a. HEAD = 92bfe3728. One commit in the PR.

Thank you for the incredibly detailed verification @wenshao — this is a really valuable analysis.

You're right that the for-loop itself is mechanically correct (step 4 proves it), but the corruption warning never reaches startupWarnings at runtime. The propagation break between loadAndMigrate returning the warning and getSettingsWarnings() seeing it is the real bug.

I think option (c) from your recommendation makes the most sense: land the for-loop as-is (it's harmless and already helps OAuth-expiry and other warning sources), update the PR description to acknowledge it doesn't fully close #4448, and open a separate issue for the propagation bug investigation. Let me know if you'd prefer a different approach.

Surface settings.json parse warnings on stderr before TUI renders, so users see diagnostic output instead of it being swallowed by the onboarding flow.

Thanks for the thorough root-cause analysis @wenshao — you're absolutely right that the parent exits at relaunchAppInChildProcess() before reaching the warning loop.

Fix (option 1 as you recommended): Moved the settings warning emission to right after loadSettings() (line ~424), before the relaunch call at line ~580. The parent now emits warnings on stderr before spawning the child. The child reaches the same code path but getSettingsWarnings() returns [] since the parent already renamed the corrupted file — so it's a no-op there.

Regression test added: New test in settings.test.ts verifies that getSettingsWarnings() returns non-empty, human-readable warnings when settings.json has invalid JSON — ensuring the early stderr loop has content to emit.

Pushed as b2994f63a.

Hi @wenshao — this has been APPROVED for a few hours now. Could you merge it when you get a chance? Happy to rebase if needed. Thanks!

I'll re-test and verify whether the fix actually surfaces the corruption warning on stderr in the standard launch flow. Will update the review status afterward.

Local end-to-end test report — PR 4461 verification (for merge reference)

I checked out pr-4461 @ b2994f63a into an isolated git worktree and ran a full real-world verification from a maintainer's perspective: build + unit tests + typecheck + lint + an honest end-to-end smoke that actually corrupts a settings.json and launches the CLI to observe user-visible behavior. Environment and findings below.

Environment

Actual scope of the PR

git diff main...pr-4461 only touches 2 files / +48 / -0:

packages/cli/src/gemini.tsx              +19  (two writeStderrLine for-loops)
packages/cli/src/config/settings.test.ts +29  (1 new regression test)

Two insertion points:

1. ~Line 428: right after loadSettings(), before sandbox / relaunchAppInChildProcess(). Synchronous stderr emit of getSettingsWarnings(settings).
2. ~Line 779: right after startupWarnings is built, before the config.isInteractive() branch and TUI render. Synchronous stderr emit of the full startupWarnings list.

Checks summary

Honest end-to-end smoke (reproducing #4448's path)

Under an isolated HOME=/tmp/pr4461-smoke/home, I hand-crafted a syntactically broken ~/.qwen/settings.json exactly the way the issue describes (unquoted key):

{
  theme: "Dark",
  selectedAuthType: "openai"
}

Then exercised 4 launch modes. All hit the expected code path and the observed behavior matches the PR's claims:

These four scenarios jointly confirm the key claim in the PR description regarding #4448:

Before: TUI shows "Connect a provider" with no visible error.
After: stderr shows: Settings file ... has invalid JSON and was renamed to ... Your settings have been reset.

Real captured stderr (scenario 1, --prompt 'hello'):

Warning: Settings file /tmp/pr4461-smoke/home/.qwen/settings.json has invalid JSON and
was renamed to /tmp/pr4461-smoke/home/.qwen/settings.json.corrupted.1779865812848.
Your settings have been reset. To recover, fix the JSON in
/tmp/pr4461-smoke/home/.qwen/settings.json.corrupted.1779865812848 and rename it back.
No auth type is selected. Please configure an auth type (e.g. via settings or
`--auth-type`) before running in non-interactive mode.

The file was indeed renamed to settings.json.corrupted.1779865812848. The UX gap that "the user has no way of noticing their settings were reset" is closed.

One minor follow-up worth tracking (non-blocking)

The PR emits stderr at two sites and relies on an implicit timing contract — "in the normal path the parent is replaced by the child via relaunchAppInChildProcess() before reaching the second emit site" — to avoid duplicates. On the default qwen launch this contract holds (I observed exactly 1 warning). But when QWEN_CODE_NO_RELAUNCH=true is set (which is precisely the env the parent itself sets for the child it spawns, and also a common developer/test debug switch), a single process runs through both the line 432 early loop and the line 779 pre-TUI loop. Because settings.migrationWarnings is not consumed between the two getSettingsWarnings(settings) calls, the same warning is printed twice. I reproduced this (grep -c 'invalid JSON' = 2):

$ HOME=... QWEN_CODE_NO_RELAUNCH=true node packages/cli/dist/index.js 2>stderr.log
$ grep -c 'invalid JSON' stderr.log
2

End users do not normally set QWEN_CODE_NO_RELAUNCH=true, so this is not a merge blocker. But the child process spawned internally by the PR's own design carries this env (see packages/cli/src/utils/relaunch.ts:49), and the no-duplicate property relies on the timing assumption that "the child's loadSettings() sees an already-renamed world, so migrationWarnings comes back empty." If in the future:

• loadSettings() behavior changes — e.g. it emits a hint when it sees an existing .corrupted.<ts> sibling, or
• some daemon / serve / IDE long-lived scenario calls getSettingsWarnings(settings) twice in the same process,

…the duplicate emit will surface immediately. Suggested minimal dedup:

// At the second for-loop, filter out warnings already emitted by the early site
const earlyEmitted = new Set(settingsWarnings);
for (const w of startupWarnings) {
  if (earlyEmitted.has(w)) continue;
  writeStderrLine(w);
}

Or: keep a single emit site at line 779 and move the first emit into the if (!process.env['SANDBOX']) block, right before relaunchAppInChildProcess() — i.e. only emit early on the branch that is guaranteed to be replaced by a child. Both are single-file, single-hunk changes and don't affect the PR's intent.

Conclusion / merge recommendation

The core problem this PR addresses (#4448: "after settings.json is corrupted the file is silently reset and the user sees the onboarding panel instead of an error") is fully closed under a local repro + a real CLI launch: of the 4 modes, the 3 that the issue directly maps to all met the PR's stated goal (default interactive → 1 stderr line, --prompt → 1 stderr line, --bare → 0 since it doesn't read settings). The new regression test plus the existing 109 settings tests are green, lint / typecheck are clean, the diff is +48 across 2 files, and the surface area of risk is tiny.

Recommend merge, with the following (non-blocking):

1. Nice to have: a dedup between the early and late emit sites so paths like QWEN_CODE_NO_RELAUNCH=true don't double-print the same warning, plus a one-line comment recording the "relaunch timing buys the no-duplicate property" contract so future daemon/serve-style long-process changes don't trip over it.
2. PR is currently mergeStateStatus = BLOCKED, reviewDecision = REVIEW_REQUIRED — needs one reviewer's approval to land.

Hi @wenshao — thanks for the approval! Could this be merged when convenient? Happy to rebase if needed.