Skip to content

Conversation

@afarber
Copy link
Contributor

@afarber afarber commented Dec 24, 2025

TLDR

Address the issue where disableUpdateNag was checked too late and rename all disable* settings to positive enable* naming:

Old New
disableAutoUpdate + disableUpdateNag enableAutoUpdate (consolidated)
disableLoadingPhrases enableLoadingPhrases
disableFuzzySearch enableFuzzySearch
disableCacheControl enableCacheControl

Dive Deeper

  • Bumped SETTINGS_VERSION to 3 with automatic migration that inverts boolean values
  • The update check now happens in gemini.tsx before any network request
  • Old settings files are automatically migrated with a backup created

When a user has both disableAutoUpdate and disableUpdateNag set with different values, the migration follows this policy:

disableAutoUpdate disableUpdateNag enableAutoUpdate
false false true
false true false
true false false
true true false

Reviewer Test Plan

Use node packages/cli/dist/index.js directly (not npm run start, which sets DEV=true and skips update checks).

Test 1: Verify update check is DISABLED

  1. Set {"general": {"enableAutoUpdate": false}} in ~/.qwen/settings.json
  2. Run sudo tcpdump -i en0 host registry.npmjs.org (use your active interface)
  3. In another terminal: node packages/cli/dist/index.js
  4. Confirm: No traffic to registry.npmjs.org

Test 2: Verify update check is ENABLED (default)

  1. Set {"general": {"enableAutoUpdate": true}} in ~/.qwen/settings.json
  2. Run tcpdump as above
  3. Run node packages/cli/dist/index.js
  4. Confirm: Traffic appears to registry.npmjs.org (104.16.x.x)

Test 3: Verify migration

  1. Create legacy settings: {"disableAutoUpdate": true, "disableUpdateNag": false}
  2. Run npm run build && npm run start
  3. Check settings file was migrated to {"general": {"enableAutoUpdate": false}}

Testing Matrix

🍏 🪟 🐧
npm run yes
npx
Docker
Podman - -
Seatbelt - -

Linked issues / bugs

Fixes #1304

@LaZzyMan
Copy link
Collaborator

Thank you for your contribution! However, I need to point out a critical issue with the current implementation.

🚨 Problem: disableAutoUpdate Setting Becomes Ineffective

With your current changes, the disableAutoUpdate setting is checked after checkForUpdates() has already made the network request:

// Current PR implementation
if (!settings.merged.general?.disableUpdateNag) {
  checkForUpdates()  // ← Network request happens here
    .then((info) => {
      handleAutoUpdate(info, ...) {
        // disableAutoUpdate checked here - too late!
        if (settings.merged.general?.disableAutoUpdate) return;
      }
    });
}

This means users who set disableAutoUpdate: true (without setting disableUpdateNag) will still experience network requests to the npm registry on startup, which defeats the purpose of the setting for air-gapped environments.


💡 Our Proposed Solution: Simplify to a Single Setting

After reviewing this issue, we believe there's semantic redundancy and conflict between disableUpdateNag and disableAutoUpdate. We'd like to propose consolidating them into a single disableAutoUpdate setting.

Proposed behavior:

  • When disableAutoUpdate: false (default): Check for updates on startup → Notify user if newer version exists → user confirm -> update
  • When disableAutoUpdate: true: Skip update checks entirely (no network requests)

This approach eliminates confusion between two overlapping settings and provides a clear, single point of control


🎯 We Noticed Your Work on Double Negatives

Speaking of which, we noticed you have an excellent PR in the gemini-cli repository that addresses the confusion caused by double-negative settings:

🔗 google-gemini/gemini-cli#14142

We really appreciate that work! The double-negative pattern (like disableUpdateNag) is indeed confusing and error-prone. Your approach of using positive naming conventions is much clearer.

Would you be interested in applying a similar improvement here? If you could submit a PR to our repository that:

  1. Removes the disableUpdateNag setting
  2. Renames/repurposes disableAutoUpdate to be the single source of truth for update control
  3. Potentially renames it to something more positive (e.g., autoUpdate: true/false or enableAutoUpdate)

We would be very happy to merge it!
Let us know your thoughts!

@afarber
Copy link
Contributor Author

afarber commented Dec 29, 2025

Thank you for reviewing @LaZzyMan

I will try to implement your suggestions in few days

And will also look into backporting my google-gemini/gemini-cli#14142

@afarber afarber force-pushed the 1304-move-disable-update-nag-check branch from a04cc8f to 821266f Compare December 29, 2025 18:43
@afarber afarber changed the title fix(cli): check disableUpdateNag setting before making network requests fix(cli): rename negative settings to positive naming (disable* -> enable*) Dec 29, 2025
@afarber
Copy link
Contributor Author

afarber commented Dec 30, 2025

Hi @LaZzyMan and @xuewenjie123 please review my PR.

Below you can see me successfully testing the changes on Macbook M1 Air -

First I have {"enableAutoUpdate": true} and there is traffic to Cloudflare IP addresses:

Screenshot 2025-12-30 at 08 48 58

Then I have {"enableAutoUpdate": false} and there is no traffic:

Screenshot 2025-12-30 at 08 50 18

@afarber afarber changed the title fix(cli): rename negative settings to positive naming (disable* -> enable*) fix(settings): rename negative settings to positive naming (disable* -> enable*) Dec 30, 2025
@afarber afarber force-pushed the 1304-move-disable-update-nag-check branch from 8f42b8b to 5cf66eb Compare January 5, 2026 17:46
@afarber
Copy link
Contributor Author

afarber commented Jan 5, 2026

Hi @LaZzyMan and @xuewenjie123 please review my PR, I have resolved the conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

While disableUpdateNag is set to true, qwen-code is still checking for updates

3 participants