diff --git a/src/commands/config/mod.rs b/src/commands/config/mod.rs index 18ad24349..be50e1ba4 100644 --- a/src/commands/config/mod.rs +++ b/src/commands/config/mod.rs @@ -162,10 +162,10 @@ mod tests { // `template-append`) so the offending key surfaces nested. assert_snapshot!( warn_unknown_keys::("[commit-generation]\ncommand = \"llm\"\n"), - @"▲ Key commit.generation.command belongs in user config (will be ignored)"); + @"▲ Key commit.generation.command belongs in user config (will be ignored); to scope it to this repo, add it under [projects.\"\"] in user config"); assert_snapshot!( warn_unknown_keys::("[commit.generation]\ncommand = \"llm\"\n"), - @"▲ Key commit.generation.command belongs in user config (will be ignored)"); + @"▲ Key commit.generation.command belongs in user config (will be ignored); to scope it to this repo, add it under [projects.\"\"] in user config"); // The one project-valid key in the section is unaffected. assert!( diff --git a/src/commands/config/show.rs b/src/commands/config/show.rs index f2f62de1a..831451e06 100644 --- a/src/commands/config/show.rs +++ b/src/commands/config/show.rs @@ -766,7 +766,11 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String { UnknownWarning::TopLevelWrongConfig { key, other_description, - } => cformat!("Key {key} belongs in {other_description} (will be ignored)"), + } => worktrunk::config::with_scope_note( + cformat!("Key {key} belongs in {other_description} (will be ignored)"), + other_description, + key, + ), UnknownWarning::TopLevelDeprecatedWrongConfig { key, other_description, @@ -775,7 +779,11 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String { UnknownWarning::NestedWrongConfig { path, other_description, - } => cformat!("Key {path} belongs in {other_description} (will be ignored)"), + } => worktrunk::config::with_scope_note( + cformat!("Key {path} belongs in {other_description} (will be ignored)"), + other_description, + path, + ), UnknownWarning::NestedUnknown { path } => { cformat!("Unknown key {path} will be ignored") } diff --git a/src/config/deprecation.rs b/src/config/deprecation.rs index 4f76fcc31..8e2ff3d75 100644 --- a/src/config/deprecation.rs +++ b/src/config/deprecation.rs @@ -1787,6 +1787,29 @@ pub fn nested_key_belongs_in(path: &str) -> Option<&'static .then(C::Other::description) } +/// Note appended to a "belongs in user config" warning: a key placed in +/// project config that really lives in user config is usually an attempt to +/// scope a personal setting to one repo, which the `[projects.""]` table +/// in user config does directly. Returns `None` for any other destination. +/// +/// `key` is the misplaced key (`worktree-path`, or a dotted path like +/// `list.columns`); the note only fires when its top-level segment is a field +/// of that table (see [`is_user_project_override_key`](crate::config::is_user_project_override_key)). +/// Root-only user settings — `skip-shell-integration-prompt`, +/// `skip-commit-generation-prompt` — have no `[projects.""]` form and no +/// per-repo semantics, so following the note would just produce a fresh +/// "unknown field"; they get no note. +/// +/// Keyed off the destination *description* rather than a config-type gate so +/// both warning formatters (load-time and `config show`) can share it — they +/// hold only the `other_description` string, not the config type. +pub fn scope_to_repo_note(other_description: &str, key: &str) -> Option<&'static str> { + let top_level = key.split('.').next().unwrap_or(key); + (other_description == crate::config::UserConfig::description() + && crate::config::is_user_project_override_key(top_level)) + .then_some(r#"to scope it to this repo, add it under [projects.""] in user config"#) +} + /// Classification of an unknown config key for warning purposes. pub enum UnknownKeyKind { /// 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) -> UnknownWarning::TopLevelWrongConfig { key, other_description, - } => cformat!( - "{label} has key {key} which belongs in {other_description} (will be ignored)" + } => with_scope_note( + cformat!( + "{label} has key {key} which belongs in {other_description} (will be ignored)" + ), + other_description, + key, ), UnknownWarning::TopLevelDeprecatedWrongConfig { key, @@ -1884,8 +1911,12 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) -> UnknownWarning::NestedWrongConfig { path, other_description, - } => cformat!( - "{label} has key {path} which belongs in {other_description} (will be ignored)" + } => with_scope_note( + cformat!( + "{label} has key {path} which belongs in {other_description} (will be ignored)" + ), + other_description, + path, ), UnknownWarning::NestedUnknown { path } => { cformat!("{label} has unknown field {path} (will be ignored)") @@ -1893,6 +1924,19 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) -> } } +/// Append the project-scoped-user-config note to `message` when the misplaced +/// `key`'s destination is user config and it's a `[projects.""]` field +/// (see [`scope_to_repo_note`]). Joined with a semicolon per the house style +/// for related clauses. The note is plain text so its `[projects.""]` +/// placeholder isn't parsed as color-print markup. Shared with `config show` +/// via [`crate::config::with_scope_note`] so the caveat lives in one place. +pub fn with_scope_note(message: String, other_description: &str, key: &str) -> String { + match scope_to_repo_note(other_description, key) { + Some(note) => format!("{message}; {note}"), + None => message, + } +} + #[cfg(test)] mod tests { use super::*; @@ -4491,6 +4535,107 @@ ff = true ); } + #[test] + fn test_nested_user_only_key_redirects_generally() { + use crate::config::{ProjectConfig, UnknownWarning, UserConfig, collect_unknown_warnings}; + + // `[list]` is a valid *shared* section (project config accepts `url`), + // but `columns` / `full` are user-config display settings. Placing them + // in project config redirects to user config rather than reading + // "unknown field" — the general "valid in the other config" check, not + // the hard-coded commit.generation list (#3469). + let warnings = collect_unknown_warnings::( + "[list]\ncolumns = [\"branch\"]\nfull = true\n", + ); + assert!( + warnings.iter().all(|w| matches!( + w, + UnknownWarning::NestedWrongConfig { path, other_description } + if (path == "list.columns" || path == "list.full") + && *other_description == "user config" + )) && warnings.len() == 2, + "expected list.columns/list.full → user config, got {warnings:?}" + ); + + // A key unknown in *both* configs stays "unknown field". + let warnings = collect_unknown_warnings::("[list]\nnonsense-typo = true\n"); + assert!( + matches!( + warnings.as_slice(), + [UnknownWarning::NestedUnknown { path }] if path == "list.nonsense-typo" + ), + "expected list.nonsense-typo → unknown, got {warnings:?}" + ); + + // The reverse direction: `url` is project-only, so it redirects to + // project config when found in user config — no scope-to-repo note + // there (that only applies to user-config destinations). + let warnings = collect_unknown_warnings::("[list]\nurl = \"x\"\n"); + assert!( + matches!( + warnings.as_slice(), + [UnknownWarning::NestedWrongConfig { path, other_description }] + if path == "list.url" && *other_description == "project config" + ), + "expected list.url → project config, got {warnings:?}" + ); + } + + #[test] + fn test_scope_to_repo_note_only_for_user_config() { + // The note fires for user-config destinations whose top-level key is a + // `[projects.""]` field, and nothing else. + assert!(scope_to_repo_note("user config", "list.columns").is_some()); + assert!(scope_to_repo_note("user config", "worktree-path").is_some()); + assert!(scope_to_repo_note("project config", "list.url").is_none()); + + // Root-only user scalars have no `[projects.""]` form, so following + // the note would just yield a fresh "unknown field" — no note. + assert!(scope_to_repo_note("user config", "skip-shell-integration-prompt").is_none()); + assert!(scope_to_repo_note("user config", "skip-commit-generation-prompt").is_none()); + + // It reaches the rendered load-warning for a user-config redirect... + let note = "to scope it to this repo"; + let msg = format_load_warning( + "Project config", + &crate::config::UnknownWarning::NestedWrongConfig { + path: "list.columns".to_string(), + other_description: "user config", + }, + ); + assert!( + msg.contains(note), + "user-config redirect should carry note: {msg}" + ); + + // ...but not a project-config redirect. + let msg = format_load_warning( + "User config", + &crate::config::UnknownWarning::NestedWrongConfig { + path: "list.url".to_string(), + other_description: "project config", + }, + ); + assert!( + !msg.contains(note), + "project-config redirect should not carry note: {msg}" + ); + + // ...and not a misplaced root-only user scalar, even though its + // destination is user config. + let msg = format_load_warning( + "Project config", + &crate::config::UnknownWarning::TopLevelWrongConfig { + key: "skip-shell-integration-prompt".to_string(), + other_description: "user config", + }, + ); + assert!( + !msg.contains(note), + "root-only user scalar should not carry note: {msg}" + ); + } + /// Every `DEPRECATION_RULES` row's migration fires on one config, pinning /// cross-rule interactions the per-rule tests can't see — in particular /// the inserted `[forge]` staying in the mid-file spot where the user diff --git a/src/config/mod.rs b/src/config/mod.rs index 6c8c45944..3d5de9ef4 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -123,6 +123,19 @@ pub(crate) fn schema_top_level_keys() -> Vec { keys } +/// Whether `key` is a top-level field of a `[projects.""]` table (the +/// [`UserProjectOverrides`] schema). Used to gate the "scope it to this repo" +/// note: a misplaced user-config key only earns that advice when it can +/// actually be placed under `[projects.""]`. Root-only user settings such +/// as `skip-shell-integration-prompt` are absent here and get no note. +pub fn is_user_project_override_key(key: &str) -> bool { + use std::sync::OnceLock; + static KEYS: OnceLock> = OnceLock::new(); + KEYS.get_or_init(schema_top_level_keys::) + .iter() + .any(|k| k == key) +} + // Re-export public types pub use approvals::{Approvals, approvals_path, require_approvals_path}; pub use commands::{Command, CommandConfig, HookStep, append_aliases}; @@ -142,7 +155,8 @@ pub use deprecation::suppress_warnings; pub use deprecation::warnings_suppressed; pub use deprecation::{ DEPRECATED_SECTION_KEYS, DeprecatedSection, UnknownKeyKind, classify_unknown_key, - key_belongs_in, nested_key_belongs_in, warn_unknown_fields, + key_belongs_in, nested_key_belongs_in, scope_to_repo_note, warn_unknown_fields, + with_scope_note, }; pub use deprecation::{DeprecationKind, Deprecations}; pub use expansion::{ diff --git a/src/config/unknown_tree.rs b/src/config/unknown_tree.rs index 1389b8bab..5f3f06ad7 100644 --- a/src/config/unknown_tree.rs +++ b/src/config/unknown_tree.rs @@ -177,6 +177,51 @@ pub enum UnknownWarning { NestedUnknown { path: String }, } +/// Position within `C::Other`'s unknown tree while walking `C`'s nested +/// unknowns. A nested key that's schema-unknown in `C` but *valid* in +/// `C::Other` (e.g. `list.columns` — a user-config display setting — placed in +/// project config) should redirect there rather than read "unknown field". We +/// answer "is this leaf valid in the other config?" by walking the other +/// config's unknown tree in lockstep: a key valid there never appears in that +/// tree, so its absence is the signal. +#[derive(Clone, Copy)] +enum OtherStatus<'a> { + /// An ancestor section is absent or wholly unknown in `C::Other`, so + /// nothing at or below this point is valid there. + UnknownSection, + /// Within a section that exists in `C::Other`. Carries the corresponding + /// node in the other tree, or `None` once no further unknowns are recorded + /// — i.e. every key at or below here is valid in the other config. + Known(Option<&'a UnknownTree>), +} + +impl<'a> OtherStatus<'a> { + /// Whether `key` at the current level is a valid key in `C::Other`. + fn key_is_valid_in_other(self, key: &str) -> bool { + match self { + OtherStatus::UnknownSection => false, + OtherStatus::Known(None) => true, + OtherStatus::Known(Some(node)) => !node.keys.contains(key), + } + } + + /// Descend into `key`, returning the status for its children. + fn descend(self, key: &str) -> OtherStatus<'a> { + match self { + OtherStatus::UnknownSection => OtherStatus::UnknownSection, + OtherStatus::Known(None) => OtherStatus::Known(None), + OtherStatus::Known(Some(node)) => { + if node.keys.contains(key) { + // Whole subtree is unknown in the other config too. + OtherStatus::UnknownSection + } else { + OtherStatus::Known(node.nested.get(key)) + } + } + } + } +} + /// Collect structured warnings for `raw_contents` under config type `C`. /// /// Top-level classification reads the *raw* tree (so deprecated top-level @@ -185,6 +230,14 @@ pub enum UnknownWarning { /// so patterns the deprecation system already warns about (e.g., /// `switch.no-cd`, `merge.no-ff`) don't double-warn here. /// +/// A misplaced *nested* key is redirected to `C::Other` when it's valid there +/// — determined by walking `C::Other`'s own unknown tree for the same content +/// (see the private `OtherStatus` helper). If that other-config analysis is +/// unreliable, the walk falls back to treating nested keys as unknown, so only +/// the hard-coded +/// [`nested_key_belongs_in`](crate::config::nested_key_belongs_in) redirects +/// still fire. +/// /// Returns an empty vec if either analysis is unreliable — the load path /// surfaces parse/type errors elsewhere. pub fn collect_unknown_warnings(raw_contents: &str) -> Vec { @@ -197,6 +250,13 @@ pub fn collect_unknown_warnings(raw_contents: &str) -> Vec t, UnknownAnalysis::Unreliable(_) => return Vec::new(), }; + // The same content viewed as the *other* config type: a nested key absent + // from this tree is valid there. Unreliable → no generalized redirect. + let other_analysis = compute_unknown_tree::(&migrated); + let other_root = match other_analysis.warn_tree() { + Some(t) => OtherStatus::Known(Some(t)), + None => OtherStatus::UnknownSection, + }; let mut out = Vec::new(); for key in &raw_tree.keys { @@ -225,7 +285,7 @@ pub fn collect_unknown_warnings(raw_contents: &str) -> Vec(sub, key, &mut out); + walk_nested::(sub, key, other_root.descend(key), &mut out); } out } @@ -233,11 +293,17 @@ pub fn collect_unknown_warnings(raw_contents: &str) -> Vec( tree: &UnknownTree, prefix: &str, + other: OtherStatus, out: &mut Vec, ) { for key in &tree.keys { let path = format!("{prefix}.{key}"); - out.push(match crate::config::nested_key_belongs_in::(&path) { + // Hard-coded redirects (commit.generation leaves) take precedence and + // fire even when the other-config analysis is unreliable; otherwise + // fall back to the general "valid in the other config" check. + let belongs = crate::config::nested_key_belongs_in::(&path) + .or_else(|| other.key_is_valid_in_other(key).then(C::Other::description)); + out.push(match belongs { Some(other_description) => UnknownWarning::NestedWrongConfig { path, other_description, @@ -250,7 +316,7 @@ fn walk_nested( continue; } let path = format!("{prefix}.{key}"); - walk_nested::(sub, &path, out); + walk_nested::(sub, &path, other.descend(key), out); } } diff --git a/tests/snapshots/integration__integration_tests__config_show__config_show_suggests_user_config_for_commit_generation.snap b/tests/snapshots/integration__integration_tests__config_show__config_show_suggests_user_config_for_commit_generation.snap index 2f8f4d888..a37dc5ee2 100644 --- a/tests/snapshots/integration__integration_tests__config_show__config_show_suggests_user_config_for_commit_generation.snap +++ b/tests/snapshots/integration__integration_tests__config_show__config_show_suggests_user_config_for_commit_generation.snap @@ -79,7 +79,7 @@ exit_code: 0   \ No newline at end of file   +[commit.generation]   +command = "claude" -▲ Key commit.generation.command belongs in user config (will be ignored) +▲ Key commit.generation.command belongs in user config (will be ignored); to scope it to this repo, add it under [projects.""] in user config SHELL INTEGRATION ▲ Shell integration not configured