Skip to content

fix(config): common config merge overwrites provider-specific values#1682

Open
3351163616 wants to merge 3 commits intofarion1231:mainfrom
3351163616:fix/common-config-merge-order
Open

fix(config): common config merge overwrites provider-specific values#1682
3351163616 wants to merge 3 commits intofarion1231:mainfrom
3351163616:fix/common-config-merge-order

Conversation

@3351163616
Copy link
Copy Markdown

Problem

When editing a Claude provider's config (e.g., changing ENABLE_LSP_TOOLS from "1" to "0") and saving, the ~/.claude/settings.json file was not updated — the old value persisted silently.

Root Cause

In apply_common_config_to_settings (src-tauri/src/services/provider/live.rs), common config was merged ON TOP of provider settings via json_deep_merge(provider, common_config). This caused any leaf value present in the common config snippet to always overwrite the corresponding provider-specific value, even when the user explicitly changed it in the editor.

The save flow:

  1. normalize_provider_common_config_for_storage — strips exact-match common config fields from provider settings (works correctly, different values are preserved)
  2. Save to DB — provider-specific override is stored
  3. write_live_with_common_configapply_common_config_to_settingsmerges common config ON TOP, overwriting the user's edit back to the snippet's value
  4. Write to disk — file appears unchanged

Fix

Reversed the merge order to json_deep_merge(common_config, provider) so that:

  • Common config fills in absent fields (default behavior preserved)
  • Provider-specific values override common config (user edits preserved)

Applied the same fix for all three config formats:

  • Claude (JSON deep merge)
  • Codex (TOML table merge)
  • Gemini (JSON env merge)

Testing

  • All 7 existing + new unit tests pass (cargo test --lib -- live::tests)
  • pnpm typecheck passes
  • pnpm test:unit — 183/185 tests pass; the 2 failures in tests/integration/App.test.tsx are pre-existing (confirmed by running on unmodified main)
  • Added 2 regression tests:
    • claude_provider_override_wins_over_common_config — verifies provider value takes precedence
    • claude_common_config_fills_absent_fields — verifies common config still fills missing fields

…take precedence

When common config was enabled, apply_common_config_to_settings merged
the common config snippet ON TOP of provider settings, silently
overwriting user edits to fields present in the snippet (e.g. changing
ENABLE_LSP_TOOLS from "1" to "0" in the editor had no effect on disk).

Reversed merge order for Claude (JSON), Codex (TOML), and Gemini (env)
so that provider-specific values override common config defaults while
absent fields are still filled from the snippet.

Added regression tests for both override and fill-in behaviors.
Copy link
Copy Markdown
Owner

@farion1231 farion1231 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Could you please check whether the following issue still exists?

[P1] Array-valued common config still loses shared entries on save

This fixes the scalar/object overwrite bug, but array-valued common config still does not round-trip correctly. The apply path here still relies on json_deep_merge / merge_toml_item, and both helpers treat arrays (and other non-object values) as full replacements. In contrast, remove_common_config_from_settings already strips common-array entries element by element. That means a provider value like ['tool1', 'tool2'] with a common snippet ['tool1'] is persisted as ['tool2'], and this apply step reconstructs only ['tool2'] instead of restoring ['tool1', 'tool2']. As a result, Claude, Codex, and Gemini can still silently drop shared array entries after save.

Mirror the backend merge-order fix on the frontend side:
- Reverse deepMerge argument order in updateCommonConfigSnippet and
  updateTomlCommonConfigSnippet so provider values override common config
  defaults rather than being overwritten by them
- Replace isSubset (value equality) with hasMatchingShape (key existence)
  in hasCommonConfigSnippet and hasTomlCommonConfigSnippet so the toggle
  stays checked when the user edits a field that also exists in the snippet
- Fix applySnippetToEnv in useGeminiCommonConfig to apply snippet first
  then let provider env values override, consistent with other app types

Add regression tests covering JSON, TOML, and Gemini env precedence.
The remove step strips common config arrays element-by-element, but
the apply step was replacing arrays wholesale via json_deep_merge /
merge_toml_item. This caused array-valued common config entries
(e.g. allowedTools) to lose shared elements after a save round-trip.

Add Array match arms to json_deep_merge and merge_toml_item that
append only items not already present, using the same subset
predicates as the remove path for symmetry.

Added round-trip, dedup, and scalar-regression tests for both
Claude (JSON) and Codex (TOML) arrays.
@3351163616
Copy link
Copy Markdown
Author

Thanks for the review! The array round-trip issue you identified has been fixed in the latest commit (e2ac1c5).

Changes made:

  1. json_deep_merge — Added a (Value::Array, Value::Array) branch that uses set-union semantics: source array items not already present in target are appended. Membership is checked via json_is_subset, the same predicate used by json_remove_array_items in the remove path, ensuring symmetric behavior.

  2. merge_toml_item — Added the same array set-union branch for TOML arrays, using toml_value_is_subset for consistency with toml_remove_array_items.

  3. 4 new tests added:

    • claude_common_config_array_roundtrip — verifies ["tool2"] + snippet ["tool1"] → apply produces ["tool1", "tool2"] → remove restores ["tool2"]
    • claude_common_config_array_deduplicates — verifies ["tool1", "tool2"] + snippet ["tool1"] does not duplicate
    • codex_common_config_array_roundtrip — TOML equivalent of the above
    • claude_common_config_scalar_still_overrides_with_array_union — regression guard ensuring scalar override behavior is unchanged

All 11 Rust tests pass (cargo test --lib -- live::tests).

Additionally, commit 5e213ac aligns the frontend merge order and detection logic with the backend fix (provider overrides take precedence, detection uses key-shape matching instead of value equality).

Copy link
Copy Markdown
Owner

@farion1231 farion1231 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, I still have three review concerns:

P2 Shape-only detection can misclassify legacy/imported Claude and Codex providers as opted in to common config. The new hasMatchingShape check only verifies that matching keys exist, not that the snippet was actually applied before. Because the form still uses initialEnabled ?? inferredHasCommon, this only affects providers that do not already have an explicit commonConfigEnabled flag. If such a provider is opened and saved, the UI state can be persisted as commonConfigEnabled: true, which changes future behavior.

P2 Gemini has the same issue. Its new detection logic now checks only for key presence, so a legacy/imported Gemini provider without explicit commonConfigEnabled can also be treated as opted in after an edit/save cycle.

P2 Frontend array semantics still do not match the backend. The Rust side now uses set-union semantics for arrays during apply and element-wise removal during strip, but the frontend helpers still replace arrays wholesale and only remove them on exact match. That means the editor preview can still diverge from the actual live config for array-valued common config.

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.

2 participants