Skip to content

fix(config): use read_raw_config() in migrations to prevent expanding defaults (#40821) - #40921

Closed
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:auto-fix/issue-40821
Closed

kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:auto-fix/issue-40821

Conversation

@kyssta-exe

Copy link
Copy Markdown
Contributor

Fixes #40821. The migrate_config() function used load_config() which deep-merges DEFAULT_CONFIG with the user's config, then save_config() wrote the entire ~12 KB expanded defaults to disk, overwriting the user's ~950-byte curated config. Changed all migration steps that call save_config() to use read_raw_config() instead, so only the user's actual config is read and modified. This matches the pattern already used by the v14+ migrations.

@maxmilian maxmilian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, clean fix that targets the actual root cause — the silent full-file expansion came from save_config(load_config()) baking the deep-merged DEFAULT_CONFIG back to disk. I verified the completeness claim: all 7 load_config()save_config() write-back sites inside migrate_config() are converted (v3→4 display/tool_progress, v4→5 timezone, v11→12 custom_providers, the stt step, missing_config backfill, the _config_version bump, and the skill_utils step). No remaining load_config()-then-save_config() path in the function. The custom_providers v11→12 step now reads the user's raw list intact, so the load-bearing local-provider entry survives. 👍

One thing worth confirming (not a blocker — read_raw_config() changes more than just the expansion): switching from the deep-merged view to raw also changes the semantics of each if "X" not in config guard. Under load_config(), DEFAULT_CONFIG always supplied keys like timezone ("timezone": "" at config.py:1797) and display.tool_progress, so those guards were effectively always False — the v4→5 / v3→4 blocks were dead no-ops. With read_raw_config() they now actually fire when the user never wrote the key. That's arguably more correct (the migrations finally run), and check_config_version() already gates on the raw _config_version so each block still runs at most once per real upgrade — so it should be benign. But it's worth a quick pass to confirm none of those previously-dead branches do something surprising on first activation (e.g. branch on a value that used to come from the merged default rather than just on key presence).

The updated test_migrate_adds_discord_channel_prompts_default assertion (channel_prompts no longer expanded into the user file) is the right shape for locking this in. Could be worth one more regression test asserting a user custom_providers entry + comments-adjacent keys survive a full migrate_config() round-trip, since that's the exact #40821 symptom.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Jun 7, 2026
@kshitijk4poor

Copy link
Copy Markdown
Contributor

Merged via #53132. Your fix was chosen as the base — clean 7-line change switching migrations from load_config() to read_raw_config(). Cherry-picked with authorship preserved (rebase-merge). A regression test was added and the existing test was renamed for clarity. Thanks!

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.16.0: first config write after upgrade rewrites config.yaml to expanded defaults, dropping custom_providers (distinct from #17876)

4 participants