Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/commands/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ mod tests {
// `template-append`) so the offending key surfaces nested.
assert_snapshot!(
warn_unknown_keys::<ProjectConfig>("[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.\"<id>\"] in user config");
assert_snapshot!(
warn_unknown_keys::<ProjectConfig>("[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.\"<id>\"] in user config");

// The one project-valid key in the section is unaffected.
assert!(
Expand Down
12 changes: 10 additions & 2 deletions src/commands/config/show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,11 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String {
UnknownWarning::TopLevelWrongConfig {
key,
other_description,
} => cformat!("Key <bold>{key}</> belongs in {other_description} (will be ignored)"),
} => worktrunk::config::with_scope_note(
cformat!("Key <bold>{key}</> belongs in {other_description} (will be ignored)"),
other_description,
key,
),
UnknownWarning::TopLevelDeprecatedWrongConfig {
key,
other_description,
Expand All @@ -775,7 +779,11 @@ fn format_show_warning(warning: &worktrunk::config::UnknownWarning) -> String {
UnknownWarning::NestedWrongConfig {
path,
other_description,
} => cformat!("Key <bold>{path}</> belongs in {other_description} (will be ignored)"),
} => worktrunk::config::with_scope_note(
cformat!("Key <bold>{path}</> belongs in {other_description} (will be ignored)"),
other_description,
path,
),
UnknownWarning::NestedUnknown { path } => {
cformat!("Unknown key <bold>{path}</> will be ignored")
}
Expand Down
153 changes: 149 additions & 4 deletions src/config/deprecation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,29 @@ pub fn nested_key_belongs_in<C: WorktrunkConfig>(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."<id>"]` 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."<id>"]` 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."<id>"] 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
Expand Down Expand Up @@ -1871,8 +1894,12 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) ->
UnknownWarning::TopLevelWrongConfig {
key,
other_description,
} => cformat!(
"{label} has key <bold>{key}</> which belongs in {other_description} (will be ignored)"
} => with_scope_note(
cformat!(
"{label} has key <bold>{key}</> which belongs in {other_description} (will be ignored)"
),
other_description,
key,
),
UnknownWarning::TopLevelDeprecatedWrongConfig {
key,
Expand All @@ -1884,15 +1911,32 @@ fn format_load_warning(label: &str, warning: &crate::config::UnknownWarning) ->
UnknownWarning::NestedWrongConfig {
path,
other_description,
} => cformat!(
"{label} has key <bold>{path}</> which belongs in {other_description} (will be ignored)"
} => with_scope_note(
cformat!(
"{label} has key <bold>{path}</> which belongs in {other_description} (will be ignored)"
),
other_description,
path,
),
UnknownWarning::NestedUnknown { path } => {
cformat!("{label} has unknown field <bold>{path}</> (will be ignored)")
}
}
}

/// Append the project-scoped-user-config note to `message` when the misplaced
/// `key`'s destination is user config and it's a `[projects."<id>"]` 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."<id>"]`
/// 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::*;
Expand Down Expand Up @@ -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::<ProjectConfig>(
"[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::<ProjectConfig>("[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::<UserConfig>("[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."<id>"]` 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."<id>"]` 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
Expand Down
16 changes: 15 additions & 1 deletion src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,19 @@ pub(crate) fn schema_top_level_keys<T: schemars::JsonSchema>() -> Vec<String> {
keys
}

/// Whether `key` is a top-level field of a `[projects."<id>"]` 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."<id>"]`. 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<Vec<String>> = OnceLock::new();
KEYS.get_or_init(schema_top_level_keys::<UserProjectOverrides>)
.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};
Expand All @@ -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::{
Expand Down
72 changes: 69 additions & 3 deletions src/config/unknown_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<C: WorktrunkConfig>(raw_contents: &str) -> Vec<UnknownWarning> {
Expand All @@ -197,6 +250,13 @@ pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<U
UnknownAnalysis::Parsed(t) => 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::<C::Other>(&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 {
Expand Down Expand Up @@ -225,19 +285,25 @@ pub fn collect_unknown_warnings<C: WorktrunkConfig>(raw_contents: &str) -> Vec<U
if !C::is_valid_key(key) {
continue; // top-level unknowns were classified above against raw
}
walk_nested::<C>(sub, key, &mut out);
walk_nested::<C>(sub, key, other_root.descend(key), &mut out);
}
out
}

fn walk_nested<C: WorktrunkConfig>(
tree: &UnknownTree,
prefix: &str,
other: OtherStatus,
out: &mut Vec<UnknownWarning>,
) {
for key in &tree.keys {
let path = format!("{prefix}.{key}");
out.push(match crate::config::nested_key_belongs_in::<C>(&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::<C>(&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,
Expand All @@ -250,7 +316,7 @@ fn walk_nested<C: WorktrunkConfig>(
continue;
}
let path = format!("{prefix}.{key}");
walk_nested::<C>(sub, &path, out);
walk_nested::<C>(sub, &path, other.descend(key), out);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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."<id>"] in user config

SHELL INTEGRATION
▲ Shell integration not configured
Expand Down
Loading