Repository navigation
fix(config): skip a rejected env override in every command, not just wt list - #4358
worktrunk-bot wants to merge 2 commits into
Conversation
The help added in #4348 says invalid environment overrides are ignored with a warning while the valid ones apply. That holds for `wt list` and other tolerant loads, but `wt switch`, `wt merge`, `wt remove`, and the `wt step` commands load config strictly and exit 1 on the same override, as `test_eval_invalid_env_override_keeps_one_fatal_diagnosis` pins. Also refresh the `LoadError::Env` docstring: since #4348 it is pushed once per rejected setting, and `vars` lists only that setting's vars.
|
why would we have different intended behavior?! |
worktrunk-bot
left a comment
There was a problem hiding this comment.
@max-sixty, it shouldn't differ. The original sentence describes the intended design, and the strict commands contradict it. This PR's docs rewording documented a bug as behavior, so I'm replacing it with a loader fix.
Startup prewarm already loads the user config tolerantly and prints ▲ Ignoring env var overrides: …. The strict commands then reload through UserConfig::load(), and load_with_cause turns the same rejection into ✗ Failed to load config. So they say the override is ignored and then exit 1 because of it. The wt step eval test I cited as evidence of intent pins that contradictory output. It doesn't show a design choice.
The follow-up push makes load_with_cause skip LoadError::Env, keeps file, --config-set, and validation failures fatal, and restores the original doc sentence. The eval test now asserts that the command succeeds and prints the warning once.
One consequence needs your call before merge. A mistyped override such as WORKTRUNK__MERGE__REMOVE=flase or WORKTRUNK__REMOVE__DELETE_BRANCH=nope used to stop wt merge or wt remove. Now the command warns and falls back to the file or default value, which can mean removing the worktree or deleting an integrated branch. That matches the documented contract and wt list, but it loosens what gates a removal, so I'm leaving it to you.
Commands that load config through UserConfig::load() (wt switch, merge, remove, for-each, the wt step commands) treated a rejected WORKTRUNK_* override as fatal, after the startup prewarm had already warned that it was being ignored. load_with_cause now skips LoadError::Env, so every command falls back to the file layers for that setting and still applies the other overrides, as documented and as wt list already does. File, --config-set, and validation failures stay fatal. Reverts this branch's docs rewording, which described the strict path as intended.
wt config --helpsays "Invalid environment overrides are ignored with a warning; other valid overrides still apply." Only tolerant loads such aswt listbehaved that way.wt switch,wt merge,wt remove,wt for-each, and thewt stepcommands reload config throughUserConfig::load(), andload_with_causetreated a rejected override as fatal. These commands printed▲ Ignoring env var overrides: …during the startup prewarm and then exited 1 with✗ Failed to load configfor the same variable.load_with_causenow skipsLoadError::Env. Every command falls back to the file layers for the rejected setting and keeps the other overrides, as the docs say. User-file parse errors,--config-setfailures, and validation failures stay fatal for these commands.With
WORKTRUNK__LIST__BRANCHES=nope,wt step eval '{{ branch }}'now prints the one warning and outputsmain(exit 0).test_eval_skips_invalid_env_overridereplaces the test that pinned the old exit 1.A mistyped override such as
WORKTRUNK__MERGE__REMOVE=flaseorWORKTRUNK__REMOVE__DELETE_BRANCH=nopeno longer stopswt mergeorwt remove. The command warns and uses the file or default value instead.The
LoadErrordocstrings are updated too. Since #4348,LoadError::Envis emitted once per rejected setting, and strict loading no longer treats it as fatal.