Skip to content

Commit 9ed48fb

Browse files
fix(config): redirect misplaced nested keys to the other config with a scope note (#3473)
1 parent 3b6630b commit 9ed48fb

6 files changed

Lines changed: 246 additions & 13 deletions

File tree

src/commands/config/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,10 +162,10 @@ mod tests {
162162
// `template-append`) so the offending key surfaces nested.
163163
assert_snapshot!(
164164
warn_unknown_keys::<ProjectConfig>("[commit-generation]\ncommand = \"llm\"\n"),
165-
@"[33m▲[39m [33mKey [1mcommit.generation.command[22m belongs in user config (will be ignored)[39m");
165+
@"[33m▲[39m [33mKey [1mcommit.generation.command[22m belongs in user config (will be ignored); to scope it to this repo, add it under [projects.\"<id>\"] in user config[39m");
166166
assert_snapshot!(
167167
warn_unknown_keys::<ProjectConfig>("[commit.generation]\ncommand = \"llm\"\n"),
168-
@"[33m▲[39m [33mKey [1mcommit.generation.command[22m belongs in user config (will be ignored)[39m");
168+
@"[33m▲[39m [33mKey [1mcommit.generation.command[22m belongs in user config (will be ignored); to scope it to this repo, add it under [projects.\"<id>\"] in user config[39m");
169169

170170
// The one project-valid key in the section is unaffected.
171171
assert!(

src/commands/config/show.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -783,7 +783,11 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String {
783783
UnknownWarning::TopLevelWrongConfig {
784784
key,
785785
other_description,
786-
} => cformat!("Key <bold>{key}</> belongs in {other_description} (will be ignored)"),
786+
} => worktrunk::config::with_scope_note(
787+
cformat!("Key <bold>{key}</> belongs in {other_description} (will be ignored)"),
788+
other_description,
789+
key,
790+
),
787791
UnknownWarning::TopLevelDeprecatedWrongConfig {
788792
key,
789793
other_description,
@@ -792,7 +796,11 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String {
792796
UnknownWarning::NestedWrongConfig {
793797
path,
794798
other_description,
795-
} => cformat!("Key <bold>{path}</> belongs in {other_description} (will be ignored)"),
799+
} => worktrunk::config::with_scope_note(
800+
cformat!("Key <bold>{path}</> belongs in {other_description} (will be ignored)"),
801+
other_description,
802+
path,
803+
),
796804
UnknownWarning::NestedUnknown { path } => {
797805
cformat!("Unknown key <bold>{path}</> will be ignored")
798806
}

src/config/deprecation.rs

Lines changed: 149 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1787,6 +1787,29 @@ pub fn nested_key_belongs_in<C: WorktrunkConfig>(path: &str) -> Option<&'static
17871787
.then(C::Other::description)
17881788
}
17891789

1790+
/// Note appended to a "belongs in user config" warning: a key placed in
1791+
/// project config that really lives in user config is usually an attempt to
1792+
/// scope a personal setting to one repo, which the `[projects."<id>"]` table
1793+
/// in user config does directly. Returns `None` for any other destination.
1794+
///
1795+
/// `key` is the misplaced key (`worktree-path`, or a dotted path like
1796+
/// `list.columns`); the note only fires when its top-level segment is a field
1797+
/// of that table (see [`is_user_project_override_key`](crate::config::is_user_project_override_key)).
1798+
/// Root-only user settings — `skip-shell-integration-prompt`,
1799+
/// `skip-commit-generation-prompt` — have no `[projects."<id>"]` form and no
1800+
/// per-repo semantics, so following the note would just produce a fresh
1801+
/// "unknown field"; they get no note.
1802+
///
1803+
/// Keyed off the destination *description* rather than a config-type gate so
1804+
/// both warning formatters (load-time and `config show`) can share it — they
1805+
/// hold only the `other_description` string, not the config type.
1806+
pub fn scope_to_repo_note(other_description: &str, key: &str) -> Option<&'static str> {
1807+
let top_level = key.split('.').next().unwrap_or(key);
1808+
(other_description == crate::config::UserConfig::description()
1809+
&& crate::config::is_user_project_override_key(top_level))
1810+
.then_some(r#"to scope it to this repo, add it under [projects."<id>"] in user config"#)
1811+
}
1812+
17901813
/// Classification of an unknown config key for warning purposes.
17911814
pub enum UnknownKeyKind {
17921815
/// Deprecated key in its correct config type — deprecation system handles it
@@ -1871,8 +1894,12 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) ->
18711894
UnknownWarning::TopLevelWrongConfig {
18721895
key,
18731896
other_description,
1874-
} => cformat!(
1875-
"{label} has key <bold>{key}</> which belongs in {other_description} (will be ignored)"
1897+
} => with_scope_note(
1898+
cformat!(
1899+
"{label} has key <bold>{key}</> which belongs in {other_description} (will be ignored)"
1900+
),
1901+
other_description,
1902+
key,
18761903
),
18771904
UnknownWarning::TopLevelDeprecatedWrongConfig {
18781905
key,
@@ -1884,15 +1911,32 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) ->
18841911
UnknownWarning::NestedWrongConfig {
18851912
path,
18861913
other_description,
1887-
} => cformat!(
1888-
"{label} has key <bold>{path}</> which belongs in {other_description} (will be ignored)"
1914+
} => with_scope_note(
1915+
cformat!(
1916+
"{label} has key <bold>{path}</> which belongs in {other_description} (will be ignored)"
1917+
),
1918+
other_description,
1919+
path,
18891920
),
18901921
UnknownWarning::NestedUnknown { path } => {
18911922
cformat!("{label} has unknown field <bold>{path}</> (will be ignored)")
18921923
}
18931924
}
18941925
}
18951926

1927+
/// Append the project-scoped-user-config note to `message` when the misplaced
1928+
/// `key`'s destination is user config and it's a `[projects."<id>"]` field
1929+
/// (see [`scope_to_repo_note`]). Joined with a semicolon per the house style
1930+
/// for related clauses. The note is plain text so its `[projects."<id>"]`
1931+
/// placeholder isn't parsed as color-print markup. Shared with `config show`
1932+
/// via [`crate::config::with_scope_note`] so the caveat lives in one place.
1933+
pub fn with_scope_note(message: String, other_description: &str, key: &str) -> String {
1934+
match scope_to_repo_note(other_description, key) {
1935+
Some(note) => format!("{message}; {note}"),
1936+
None => message,
1937+
}
1938+
}
1939+
18961940
#[cfg(test)]
18971941
mod tests {
18981942
use super::*;
@@ -4491,6 +4535,107 @@ ff = true
44914535
);
44924536
}
44934537

4538+
#[test]
4539+
fn test_nested_user_only_key_redirects_generally() {
4540+
use crate::config::{ProjectConfig, UnknownWarning, UserConfig, collect_unknown_warnings};
4541+
4542+
// `[list]` is a valid *shared* section (project config accepts `url`),
4543+
// but `columns` / `full` are user-config display settings. Placing them
4544+
// in project config redirects to user config rather than reading
4545+
// "unknown field" — the general "valid in the other config" check, not
4546+
// the hard-coded commit.generation list (#3469).
4547+
let warnings = collect_unknown_warnings::<ProjectConfig>(
4548+
"[list]\ncolumns = [\"branch\"]\nfull = true\n",
4549+
);
4550+
assert!(
4551+
warnings.iter().all(|w| matches!(
4552+
w,
4553+
UnknownWarning::NestedWrongConfig { path, other_description }
4554+
if (path == "list.columns" || path == "list.full")
4555+
&& *other_description == "user config"
4556+
)) && warnings.len() == 2,
4557+
"expected list.columns/list.full → user config, got {warnings:?}"
4558+
);
4559+
4560+
// A key unknown in *both* configs stays "unknown field".
4561+
let warnings = collect_unknown_warnings::<ProjectConfig>("[list]\nnonsense-typo = true\n");
4562+
assert!(
4563+
matches!(
4564+
warnings.as_slice(),
4565+
[UnknownWarning::NestedUnknown { path }] if path == "list.nonsense-typo"
4566+
),
4567+
"expected list.nonsense-typo → unknown, got {warnings:?}"
4568+
);
4569+
4570+
// The reverse direction: `url` is project-only, so it redirects to
4571+
// project config when found in user config — no scope-to-repo note
4572+
// there (that only applies to user-config destinations).
4573+
let warnings = collect_unknown_warnings::<UserConfig>("[list]\nurl = \"x\"\n");
4574+
assert!(
4575+
matches!(
4576+
warnings.as_slice(),
4577+
[UnknownWarning::NestedWrongConfig { path, other_description }]
4578+
if path == "list.url" && *other_description == "project config"
4579+
),
4580+
"expected list.url → project config, got {warnings:?}"
4581+
);
4582+
}
4583+
4584+
#[test]
4585+
fn test_scope_to_repo_note_only_for_user_config() {
4586+
// The note fires for user-config destinations whose top-level key is a
4587+
// `[projects."<id>"]` field, and nothing else.
4588+
assert!(scope_to_repo_note("user config", "list.columns").is_some());
4589+
assert!(scope_to_repo_note("user config", "worktree-path").is_some());
4590+
assert!(scope_to_repo_note("project config", "list.url").is_none());
4591+
4592+
// Root-only user scalars have no `[projects."<id>"]` form, so following
4593+
// the note would just yield a fresh "unknown field" — no note.
4594+
assert!(scope_to_repo_note("user config", "skip-shell-integration-prompt").is_none());
4595+
assert!(scope_to_repo_note("user config", "skip-commit-generation-prompt").is_none());
4596+
4597+
// It reaches the rendered load-warning for a user-config redirect...
4598+
let note = "to scope it to this repo";
4599+
let msg = format_load_warning(
4600+
"Project config",
4601+
&crate::config::UnknownWarning::NestedWrongConfig {
4602+
path: "list.columns".to_string(),
4603+
other_description: "user config",
4604+
},
4605+
);
4606+
assert!(
4607+
msg.contains(note),
4608+
"user-config redirect should carry note: {msg}"
4609+
);
4610+
4611+
// ...but not a project-config redirect.
4612+
let msg = format_load_warning(
4613+
"User config",
4614+
&crate::config::UnknownWarning::NestedWrongConfig {
4615+
path: "list.url".to_string(),
4616+
other_description: "project config",
4617+
},
4618+
);
4619+
assert!(
4620+
!msg.contains(note),
4621+
"project-config redirect should not carry note: {msg}"
4622+
);
4623+
4624+
// ...and not a misplaced root-only user scalar, even though its
4625+
// destination is user config.
4626+
let msg = format_load_warning(
4627+
"Project config",
4628+
&crate::config::UnknownWarning::TopLevelWrongConfig {
4629+
key: "skip-shell-integration-prompt".to_string(),
4630+
other_description: "user config",
4631+
},
4632+
);
4633+
assert!(
4634+
!msg.contains(note),
4635+
"root-only user scalar should not carry note: {msg}"
4636+
);
4637+
}
4638+
44944639
/// Every `DEPRECATION_RULES` row's migration fires on one config, pinning
44954640
/// cross-rule interactions the per-rule tests can't see — in particular
44964641
/// the inserted `[forge]` staying in the mid-file spot where the user

src/config/mod.rs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,19 @@ pub(crate) fn schema_top_level_keys<T: schemars::JsonSchema>() -> Vec<String> {
123123
keys
124124
}
125125

126+
/// Whether `key` is a top-level field of a `[projects."<id>"]` table (the
127+
/// [`UserProjectOverrides`] schema). Used to gate the "scope it to this repo"
128+
/// note: a misplaced user-config key only earns that advice when it can
129+
/// actually be placed under `[projects."<id>"]`. Root-only user settings such
130+
/// as `skip-shell-integration-prompt` are absent here and get no note.
131+
pub fn is_user_project_override_key(key: &str) -> bool {
132+
use std::sync::OnceLock;
133+
static KEYS: OnceLock<Vec<String>> = OnceLock::new();
134+
KEYS.get_or_init(schema_top_level_keys::<UserProjectOverrides>)
135+
.iter()
136+
.any(|k| k == key)
137+
}
138+
126139
// Re-export public types
127140
pub use approvals::{Approvals, approvals_path, require_approvals_path};
128141
pub use commands::{Command, CommandConfig, HookStep, append_aliases};
@@ -142,7 +155,8 @@ pub use deprecation::suppress_warnings;
142155
pub use deprecation::warnings_suppressed;
143156
pub use deprecation::{
144157
DEPRECATED_SECTION_KEYS, DeprecatedSection, UnknownKeyKind, classify_unknown_key,
145-
key_belongs_in, nested_key_belongs_in, warn_unknown_fields,
158+
key_belongs_in, nested_key_belongs_in, scope_to_repo_note, warn_unknown_fields,
159+
with_scope_note,
146160
};
147161
pub use deprecation::{DeprecationKind, Deprecations};
148162
pub use expansion::{

src/config/unknown_tree.rs

Lines changed: 69 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,51 @@ pub enum UnknownWarning {
177177
NestedUnknown { path: String },
178178
}
179179

180+
/// Position within `C::Other`'s unknown tree while walking `C`'s nested
181+
/// unknowns. A nested key that's schema-unknown in `C` but *valid* in
182+
/// `C::Other` (e.g. `list.columns` — a user-config display setting — placed in
183+
/// project config) should redirect there rather than read "unknown field". We
184+
/// answer "is this leaf valid in the other config?" by walking the other
185+
/// config's unknown tree in lockstep: a key valid there never appears in that
186+
/// tree, so its absence is the signal.
187+
#[derive(Clone, Copy)]
188+
enum OtherStatus<'a> {
189+
/// An ancestor section is absent or wholly unknown in `C::Other`, so
190+
/// nothing at or below this point is valid there.
191+
UnknownSection,
192+
/// Within a section that exists in `C::Other`. Carries the corresponding
193+
/// node in the other tree, or `None` once no further unknowns are recorded
194+
/// — i.e. every key at or below here is valid in the other config.
195+
Known(Option<&'a UnknownTree>),
196+
}
197+
198+
impl<'a> OtherStatus<'a> {
199+
/// Whether `key` at the current level is a valid key in `C::Other`.
200+
fn key_is_valid_in_other(self, key: &str) -> bool {
201+
match self {
202+
OtherStatus::UnknownSection => false,
203+
OtherStatus::Known(None) => true,
204+
OtherStatus::Known(Some(node)) => !node.keys.contains(key),
205+
}
206+
}
207+
208+
/// Descend into `key`, returning the status for its children.
209+
fn descend(self, key: &str) -> OtherStatus<'a> {
210+
match self {
211+
OtherStatus::UnknownSection => OtherStatus::UnknownSection,
212+
OtherStatus::Known(None) => OtherStatus::Known(None),
213+
OtherStatus::Known(Some(node)) => {
214+
if node.keys.contains(key) {
215+
// Whole subtree is unknown in the other config too.
216+
OtherStatus::UnknownSection
217+
} else {
218+
OtherStatus::Known(node.nested.get(key))
219+
}
220+
}
221+
}
222+
}
223+
}
224+
180225
/// Collect structured warnings for `raw_contents` under config type `C`.
181226
///
182227
/// Top-level classification reads the *raw* tree (so deprecated top-level
@@ -185,6 +230,14 @@ pub enum UnknownWarning {
185230
/// so patterns the deprecation system already warns about (e.g.,
186231
/// `switch.no-cd`, `merge.no-ff`) don't double-warn here.
187232
///
233+
/// A misplaced *nested* key is redirected to `C::Other` when it's valid there
234+
/// — determined by walking `C::Other`'s own unknown tree for the same content
235+
/// (see the private `OtherStatus` helper). If that other-config analysis is
236+
/// unreliable, the walk falls back to treating nested keys as unknown, so only
237+
/// the hard-coded
238+
/// [`nested_key_belongs_in`](crate::config::nested_key_belongs_in) redirects
239+
/// still fire.
240+
///
188241
/// Returns an empty vec if either analysis is unreliable — the load path
189242
/// surfaces parse/type errors elsewhere.
190243
pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<UnknownWarning> {
@@ -197,6 +250,13 @@ pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<U
197250
UnknownAnalysis::Parsed(t) => t,
198251
UnknownAnalysis::Unreliable(_) => return Vec::new(),
199252
};
253+
// The same content viewed as the *other* config type: a nested key absent
254+
// from this tree is valid there. Unreliable → no generalized redirect.
255+
let other_analysis = compute_unknown_tree::<C::Other>(&migrated);
256+
let other_root = match other_analysis.warn_tree() {
257+
Some(t) => OtherStatus::Known(Some(t)),
258+
None => OtherStatus::UnknownSection,
259+
};
200260

201261
let mut out = Vec::new();
202262
for key in &raw_tree.keys {
@@ -225,19 +285,25 @@ pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<U
225285
if !C::is_valid_key(key) {
226286
continue; // top-level unknowns were classified above against raw
227287
}
228-
walk_nested::<C>(sub, key, &mut out);
288+
walk_nested::<C>(sub, key, other_root.descend(key), &mut out);
229289
}
230290
out
231291
}
232292

233293
fn walk_nested<C: WorktrunkConfig>(
234294
tree: &UnknownTree,
235295
prefix: &str,
296+
other: OtherStatus,
236297
out: &mut Vec<UnknownWarning>,
237298
) {
238299
for key in &tree.keys {
239300
let path = format!("{prefix}.{key}");
240-
out.push(match crate::config::nested_key_belongs_in::<C>(&path) {
301+
// Hard-coded redirects (commit.generation leaves) take precedence and
302+
// fire even when the other-config analysis is unreliable; otherwise
303+
// fall back to the general "valid in the other config" check.
304+
let belongs = crate::config::nested_key_belongs_in::<C>(&path)
305+
.or_else(|| other.key_is_valid_in_other(key).then(C::Other::description));
306+
out.push(match belongs {
241307
Some(other_description) => UnknownWarning::NestedWrongConfig {
242308
path,
243309
other_description,
@@ -250,7 +316,7 @@ fn walk_nested<C: WorktrunkConfig>(
250316
continue;
251317
}
252318
let path = format!("{prefix}.{key}");
253-
walk_nested::<C>(sub, &path, out);
319+
walk_nested::<C>(sub, &path, other.descend(key), out);
254320
}
255321
}
256322

tests/snapshots/integration__integration_tests__config_show__config_show_suggests_user_config_for_commit_generation.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ exit_code: 0
7979
  \ No newline at end of file
8080
  +[commit.generation]
8181
  +command = "claude"
82-
[33m▲[39m [33mKey [1mcommit.generation.command[22m belongs in user config (will be ignored)[39m
82+
[33m▲[39m [33mKey [1mcommit.generation.command[22m belongs in user config (will be ignored); to scope it to this repo, add it under [projects."<id>"] in user config[39m
8383

8484
SHELL INTEGRATION
8585
▲ Shell integration not configured

0 commit comments

Comments
 (0)