fix(config): rank env vars and --config-set above project entries - #3790
fix(config): rank env vars and --config-set above project entries#3790worktrunk-bot wants to merge 8 commits into
Conversation
Layer and specificity were separate steps: `load_with_warnings` flattened system config, user config, `WORKTRUNK_*` env vars and `--config-set` into one document, and the accessors then resolved specificity on the result, so a `[projects."<id>"]` entry beat the global key whichever layer set it. `WORKTRUNK_WORKTREE_PATH` therefore could not override a project's `worktree-path` (#3788), and neither could a global `--config-set`. Load now applies the two invocation layers at both scopes: whatever they set is dropped from every project entry, so the global key they also set answers for it. Held back are keys the layer restates under `projects."<name>"` (the highest layer *and* the most specific key) and hooks/aliases, whose project-scoped values append to the global ones rather than replacing them — both already run, so nothing is outranked there.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Two gaps in the new step 5, both reproduced against 392d478 with throwaway tests in src/config/user/tests.rs; details inline.
The one that matters: an invocation override of a [list.custom-columns.<Name>] key strands that column's required template inside the project entry, the merged document stops deserializing, and finalize takes its Err arm and returns UserConfig::default(). Step 5 runs after both existing probe points, so nothing rolls it back — the user loses their entire config for that invocation, not just the project entry.
Smaller: step.copy-ignored.exclude composes exactly the way hooks and aliases do, but isn't held back, so --config-set 'step.copy-ignored.exclude=[…]' — the flagship example in the --config-set docs section — silently stops a project's patterns applying.
The rest reads well: the layer-vs-specificity framing, the exclusive_sibling reasoning and its now-exhaustive pair coverage, the control assertion that keeps the end-to-end test from passing on project entries having stopped applying, and the precedence table.
Three of the four sources it ranks are not environment variables, so `### Precedence` sat under the wrong parent. It becomes a `##` section of its own, after the two invocation layers it ranks above have been introduced. The table's `Loses to` column restated the row order, so the ranking is a numbered list saying what the order means: `worktree-path` comes from the first source that sets it. The lead names the principle behind the order rather than the internal layer/specificity vocabulary, and hooks and aliases follow from it instead of standing as an exception. `--config-set` and the env-var intro each stated their own rank; both now point at the one section that holds it. In the loader, the function docstring no longer restates the module docstring's two axes.
worktrunk-bot
left a comment
There was a problem hiding this comment.
The docs restructure reads well — a standalone Precedence section that both the env-var and [projects] sections point at is a better home than a subsection of Environment variables, and the numbered list is easier to scan than the old table.
One thing got lost in the move: the ordered list is framed as exhaustive ("comes from the first of these that sets it"), but a project-scoped --config-set beats a global --config-set for the same key, and the replacement sentence describes scoping rather than ranking. That's the one case apply_invocation_layer_over_projects special-cases (the restated argument to drop_overridden_keys), so it's worth keeping the ranking claim the old text carried. Inline suggestion below; it changes src/cli/mod.rs, so the mirrors and the help_config_long snapshot need regenerating with it.
Review found two gaps in the step that ranks the invocation layers above `[projects."…"]` specificity. `[list.custom-columns]` merges per whole column, and `ListColumnConfig::template` is required, so removing one overridden leaf both left the project's column winning the merge anyway and stranded a column that no longer deserializes — `finalize` would then answer with `UserConfig::default()`, costing the user their whole config rather than one project entry. `is_atomic_section` names the table whose children go as a unit, alongside `exclusive_sibling`'s pairs. Both are enumerations, so the step now degrades as a unit behind them: the removals land on a candidate that is kept only if it still deserializes and validates, the same all-or-nothing guarantee the env and `--config-set` layers already have. `deserialize_and_validate` is that shared probe. `step.copy-ignored.exclude` unions the project's patterns onto the global ones (`CopyIgnoredConfig::merged_with`), exactly as hooks and aliases do, so it joins them in the held-back set — otherwise `--config-set 'step.copy-ignored.exclude=…'`, the example the docs use for the flag, silently stopped a project's patterns applying. The set is now a path list, cached in a `OnceLock` the way `is_user_project_override_key` caches its schema lookup, and an empty overlay returns before building it at all.
The numbered ranking reads as complete, so a reader who sets both a global and a project-scoped `--config-set` for one key takes the global one as the winner. The project-scoped one wins — it is both the highest layer and the most specific key, which is the `restated` branch in `drop_overridden_keys`. "Applies to that project alone" states the scope but not the rank the pre-move text carried.
worktrunk-bot
left a comment
There was a problem hiding this comment.
is_atomic_section and the candidate/backstop shape land what the earlier threads asked for, and I re-derived the "only required leaf" claim rather than taking the reply for it: scanning UserProjectOverrides, ListColumnConfig::template is indeed the sole non-Option, non-default leaf reachable under a project entry — ProjectForgeConfig, CopyIgnoredConfig, CommitGenerationConfig, ListConfig's scalars and the rest are all optional or defaulted — so the enumeration is complete for today's schema. CopyIgnoredConfig::merged_with unions through StepConfig::merge_with/merge_optional, so step.copy-ignored.exclude belongs on the composing list exactly as hooks and aliases do.
One correction, inline: the Err arm's comment claims it is unreachable, and it isn't — not through a partial removal, but through a candidate that was already invalid when the pass started.
`step.copy-ignored.exclude` made the held-back set a nested path, and a path list needs a walker with an empty-path case that nothing reaches. Asking `is_compose_only(section, key)` inside the walk instead puts the rule alongside `exclusive_sibling` and `is_atomic_section`, which already answer per section, and leaves every arm reachable from a test. The projects table is now taken with `let … else` after the clone rather than probed twice — the `contains_key` guard made the `if let` beneath it dead.
worktrunk-bot
left a comment
There was a problem hiding this comment.
The predicate reads better than the path list — is_compose_only sits next to exclusive_sibling and is_atomic_section and is asked the same question by the same walk, and deleting remove_path takes the empty-parent pruning with it, which only ever existed to make the path list behave.
One regression rode along, inline below: the !merged_table.contains_key("projects") half of the entry guard became the let Some(projects) = … else — which now sits after both overlay.clone() and merged_table.clone().
A second consequence of the same move, worth a decision rather than a fix: global.is_empty() used to be evaluated after the compose-only paths were pruned out of global, so an overlay of nothing but composing keys (--config-set 'aliases.ship = …', the case test_invocation_layer_leaves_composing_keys_alone exercises) returned before the candidate clone. A predicate can't be applied to global up front the way a path list could, so those invocations now clone the merged table and run a full deserialize_and_validate for a pass that provably removed nothing. That's likely the right trade for the simpler shape, but it isn't in the commit message, and it widens which invocations reach the Err arm below.
The `contains_key("projects")` half of the entry guard turned into the
`let … else` below it, which sits after both clones — so a config with env
vars or `--config-set` in play and no `[projects]` table paid for them on
every load before bailing. The `let … else` stays as the `get_mut` and the
non-table backstop.
The discard arm's comment claimed it was unreachable. It isn't: step 3's env
probe deserializes without validating, so a semantically-invalid env value
reaches this pass in an already-invalid candidate and lands there on the
pre-existing failure rather than on anything the removals did. Behavior is
right either way — `finalize` reports it — but the comment named a dead arm
that isn't. `test_invocation_layer_keeps_an_already_invalid_candidate_untouched`
pins it, with a control showing the same overlay does remove the project's key
when it validates.
An overlay of nothing but composing keys still clones and probes: the path
list could be pruned out of `global` before the emptiness check, a predicate
asked per section can't. That is the cost of the simpler shape, on
invocations that already set an env var or `--config-set`.
Fixes #3788.
Layer and specificity were separate steps.
load_with_warningsflattened system config → user config →WORKTRUNK_*env vars →--config-setinto one document, and the accessors then resolved specificity on that document, so a[projects."<id>"]entry answered for the global key of the same name whichever layer set it.WORKTRUNK_WORKTREE_PATHcould therefore not override a project'sworktree-path, and a global--config-sethit the same wall.Per @max-sixty in the issue thread — "env vars should indeed take precedence over the user project config, we should fix this throughout" — the two invocation layers now cross the axes: they're typed for one run, so they outrank a project entry as well as the global key. Load applies them at both scopes (
apply_invocation_layer_over_projects, the last step beforefinalize): whatever the layer set is dropped from every project entry, leaving the global key it also set to answer for it.Two kinds of key are held back:
projects."<name>"—--config-set 'projects."github.com/owner/repo".worktree-path = …'is both the highest layer and the most specific key, so it still wins over the same layer's global key.step.copy-ignored.exclude— whose project-scoped values append to the global ones rather than replacing them. Both already apply, so an env-set hook was never outranked, and dropping the project's copy would silently stop it running. Hook names come fromHooksConfig's schema, so a new hook can't be forgotten.Two sections have to go as a unit rather than leaf by leaf.
[commit.generation]'s mutually exclusive pairs:templateandtemplate-fileclear one another inmerge_withand are rejected together byvalidate, so overriding either has to displace both at project scope — otherwise the project's partner would still win the merge.exclusive_siblingnames those pairs. And[list.custom-columns], whichListConfig::merge_withextends per whole column, so a partial removal leaves the project's column replacing the global one anyway — andListColumnConfig::templateis required, so it can also strand a column that no longer deserializes.is_atomic_sectionnames that table.Both are enumerations, so the pass degrades as a unit behind them: the removals land on a candidate, kept only if it still deserializes and validates. That is the guarantee the env and
--config-setlayers already have, and without it the next required field would answer a stranded leaf withUserConfig::default()— costing the user their whole config for that invocation rather than one project entry's precedence.The precedence table now reads:
worktree-path--config-set 'worktree-path = …'WORKTRUNK_WORKTREE_PATH--config-set[projects."github.com/owner/repo"]in a config fileworktree-pathin a config fileDocs
The help text had no precedence section at all — the gap that made this read as a bug — so this adds one under Environment variables, plus a pointer from User project-specific settings. That supersedes #3789, which documented the old behavior; I'll close it in favour of this.
Testing
Nine unit tests in
src/config/user/tests.rscover the table-level rule (both layers, pattern entries, restated project-scoped overrides, untouched sibling keys, composing keys, the exclusive pair, the atomic custom column, a rolled-back layer, and the no-override no-op), andtest_switch_create_invocation_layers_outrank_project_worktree_pathproves it end-to-end — a real process is the only thing that readsWORKTRUNK_WORKTREE_PATHoff the environment. That test keeps a control showing the project entry still beats the config file's own global key, so it can't pass by project entries having stopped applying.The reproduction from the issue now lands where it says it should:
Local suite
cargo test --lib --binsandcargo test --test integrationare green apart fromtest_copy_ignored_preserves_file_executable_permissions, which fails in this sandbox because its umask is0002(file created0664, test expects0644) — unrelated to this change and not reproducible on a0022runner.cargo fmt --checkandcargo clippy --all-targets --all-featuresare clean.