Skip to content

fix(config): migrate deprecated keys passed via --config-set - #3152

Merged
max-sixty merged 1 commit into
mainfrom
config-set-deprecation-migration
Jun 22, 2026
Merged

max-sixty merged 1 commit into
mainfrom
config-set-deprecation-migration

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

The gap

The global --config-set <toml> flag (inline config override, added in #3138, shipped in v0.61.0) bypassed the config deprecation-migration layer. A deprecated key passed via --config-set fell through as an unknown field and was silently dropped — UserConfig has no deny_unknown_fields, and the --config-set layer never ran the unknown-field warning — so the override had no effect and no signal. The same key in a config file is rewritten to its canonical form by migrate_content() before serde parses it, so it works there.

For example, before this change wt --config-set 'merge.no-ff = true' … left merge.ff at its default (the deprecated no-ff was ignored), whereas [merge]\nno-ff = true in a config file correctly resolved to merge.ff = false.

The fix

Route each --config-set fragment through the same migrate_content() config files use — structural and silent rewrites only, no UpdateOnly — before parsing and deep-merging it (apply_cli_overrides in src/config/user/mod.rs). This is the same shape the test-only load_from_str already uses: migrate, then parse. deprecation.rs is untouched.

Two decisions worth calling out:

  • Migrate each fragment, not the merged document. Because migrate_negated_bool keeps a pre-existing canonical key when both forms are present, migrating the merged table would let a lower layer's merge.ff win over a --config-set 'merge.no-ff = …' override. Migrating each fragment in isolation and then deep-merging preserves the documented layer precedence (config files → WORKTRUNK_* env → --config-set).
  • The migration is silent (uses migrate_content, not check_and_migrate). The deprecation warning invariant is file-scoped — "a warning fires exactly when wt config update would change the file" — and an inline override has no file to materialize, so a "run wt config update" hint would point at a file the key isn't in. Deprecated keys via --config-set previously vanished silently; they now take effect silently.

Malformed fragments are unaffected: migrate_content returns invalid TOML unchanged, so the existing parse-error path still drops the whole --config-set layer with an attributed LoadError::CliOverride warning, preserving the lower layers.

Tests

  • Unit (src/config/user/tests.rs): test_cli_override_migrates_deprecated_keys (merge.no-ff → ff = false, switch.no-cd → cd = false, no warnings) and test_cli_override_deprecated_key_wins_over_lower_canonical (precedence).
  • Integration (tests/integration_tests/switch.rs): test_switch_config_set_migrates_deprecated_no_cd — end-to-end proof via the cd directive (the migrated switch.cd = false suppresses the "Cannot change directory" line). This is the bug-catcher: reverting the one-line fix reintroduces the line and fails the snapshot.
  • Integration (tests/integration_tests/list_config.rs): test_list_config_cli_override_deprecated_key_silently_migrated — guards the "layer accepted, no warning" contract among the other --config-set tests. (A wt list snapshot can't observe a merge key, so the behavioral proof lives in the switch test.)

Known adjacent gap (out of scope)

The WORKTRUNK_* env-var layer still does not migrate deprecated keys: WORKTRUNK__MERGE__NO_FF=true lands as merge.no-ff and is silently dropped, the same defect this PR fixes for --config-set. That path is a different shape (per-var typed/string probing against the file table, not a TOML fragment) and can't reuse the one-line migrate_content insertion, so it's left for a follow-up rather than forced into this change. The inline-table form (merge = { no-ff = true }) is likewise not migrated — a pre-existing migrate_content limitation that affects config files too (documented at deprecation.rs:500-504); the dotted form the --config-set help text shows works.

This was written by Claude Code on behalf of max

A deprecated config key passed via the global `--config-set <toml>` flag
bypassed the deprecation-migration layer: it fell through as an unknown field
and was silently ignored, while the same key in a config file is rewritten to
its canonical form before serde parses it.

Route each `--config-set` fragment through the same `migrate_content()` config
files use (structural and silent rewrites, no UpdateOnly) before parsing and
merging, so e.g. `merge.no-ff` canonicalizes to `merge.ff` and takes effect.
Migrating each fragment in isolation — not the merged document — keeps layer
precedence intact: a deprecated override still wins over a lower layer's
canonical key. The rewrite is silent: an inline override has no file to
materialize, so the file-scoped "run wt config update" warning would have
nothing to act on.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@max-sixty
max-sixty merged commit c20b19c into main Jun 22, 2026
37 checks passed
@max-sixty
max-sixty deleted the config-set-deprecation-migration branch June 22, 2026 01:43
max-sixty added a commit that referenced this pull request Jun 22, 2026
…3158)

## The gap

#3152 routed the `--config-set` layer through the deprecation-migration
layer, but a deprecated config key could still slip through unmigrated
in two other places. Both are now closed with the same mechanism
(`migrate_content`).

## Env-var layer

`migrate_env_overlay()` runs the assembled `WORKTRUNK_*` overlay through
the same `migrate_content()` config files and `--config-set` use, before
the deep-merge. So `WORKTRUNK__MERGE__NO_FF=true` resolves to the
deprecated key `merge.no-ff` and now takes effect as `merge.ff=false`,
instead of falling through as an unknown field and being silently
ignored. Migrating before the merge (not after) keeps the env value
winning over a lower layer's canonical key, matching the layer
precedence.

One behavior edge, now documented on the function and pinned by a test:
canonicalization surfaces a type mismatch the deprecated name hid.
`WORKTRUNK__COMMIT_GENERATION__COMMAND=42` migrates to
`commit.generation.command = 42`, which fails to deserialize (the field
is a String), so the whole env layer is dropped with an attributed
`LoadError::Env` warning and file config is preserved — the same
contract a type-mismatched value in a *canonical* env var already
follows. Previously the unknown key was silently ignored and other env
vars survived.

## Inline-table form

`migrate_negated_bool_doc` resolved its section with `as_table_mut()`,
which returns `None` for an inline `merge = { no-ff = true }`, so the
inline form was never migrated (and surfaced as a double unknown-field
warning instead of a deprecation warning). It now branches on standard
vs inline table, fixing config files, `--config-set`, and env at once —
the inline shape is preserved (`merge = { ff = false }`).

`remove_switch_picker_timeout_in` had the same
`as_table_mut()`-on-`switch` limitation, so it's given matching
inline-`switch` handling and now every `switch` sub-key rule is
inline-aware. With that, the `DEPRECATION_RULES` ordering comment is
corrected: the `[select]` → `[switch.picker]` rule's inline-to-standard
`switch` conversion is no longer relied on by any rule; only its
`timeout-ms` move remains load-bearing.

## Tests

- Unit: `test_env_overlay_migrates_deprecated_key` (full `EnvVar` →
resolve → migrate path), `test_migrate_negated_bool_inline_table`.
- Integration: `test_switch_env_var_migrates_deprecated_no_cd`
(cd-directive bug-catcher — reverting the fix re-adds the "Cannot change
directory" line),
`test_list_config_env_deprecated_type_mismatch_drops_layer` (the
documented whole-layer-drop edge).
- Inline cases added to the `test_warning_fires_iff_update_changes`
battery (negated-bool inline, inline-`switch` `timeout-ms`).

> _This was written by Claude Code on behalf of max_

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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