diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 6d6e0f7..1e8c468 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -869,7 +869,7 @@ async fn open_local_db_with_policy(graph: &ResolvedCliGraph) -> Result(cli_as: Option<&'a str>, config: &'a OmnigraphConfig) -> Option<&'a str> { - cli_as.or(config.cli.actor.as_deref()) + cli_as.or(config.defaults.actor.as_deref()) } fn resolve_policy_tests_path(context: &ResolvedPolicyContext) -> PathBuf { @@ -890,7 +890,7 @@ fn resolve_remote_bearer_token( explicit_target: Option<&str>, ) -> Result> { let scoped_env = - config.graph_bearer_token_env(explicit_uri, explicit_target, config.cli_graph_name()); + config.graph_bearer_token_env(explicit_uri, explicit_target, config.default_graph_name()); let mut env_names = Vec::new(); if let Some(name) = scoped_env { env_names.push(name.to_string()); @@ -962,7 +962,7 @@ fn resolve_uri( cli_uri: Option, cli_target: Option<&str>, ) -> Result { - config.resolve_target_uri(cli_uri, cli_target, config.cli_graph_name()) + config.resolve_target_uri(cli_uri, cli_target, config.default_graph_name()) } fn resolve_cli_graph( @@ -975,7 +975,7 @@ fn resolve_cli_graph( } else { cli_target .map(str::to_string) - .or_else(|| config.cli_graph_name().map(str::to_string)) + .or_else(|| config.default_graph_name().map(str::to_string)) }; config.resolve_graph_selection(selected.as_deref())?; let locator = config.resolve_graph(cli_uri.as_deref(), cli_target)?; @@ -1056,7 +1056,7 @@ fn resolve_branch( ) -> String { cli_branch .or(alias_branch) - .or_else(|| config.cli.branch.clone()) + .or_else(|| config.defaults.branch.clone()) .unwrap_or_else(|| default_branch.to_string()) } @@ -1072,7 +1072,7 @@ fn resolve_read_target( Ok(read_target_from_cli( cli_branch .or(alias_branch) - .or_else(|| config.cli.branch.clone()), + .or_else(|| config.defaults.branch.clone()), cli_snapshot, )) } @@ -1520,7 +1520,7 @@ fn resolve_read_format( } else { cli_format .or(alias_format) - .unwrap_or_else(|| config.cli_output_format()) + .unwrap_or_else(|| config.default_output_format()) } } @@ -1583,7 +1583,7 @@ server: graph: local bind: 127.0.0.1:8080 -cli: +defaults: graph: local branch: main output_format: table @@ -1716,7 +1716,7 @@ async fn execute_query_lint( } let has_graph_target = - cli_uri.is_some() || cli_target.is_some() || config.cli_graph_name().is_some(); + cli_uri.is_some() || cli_target.is_some() || config.default_graph_name().is_some(); if !has_graph_target { bail!("query lint requires --schema or a resolvable graph target"); } @@ -1814,7 +1814,7 @@ fn resolve_registry_selection_for_list( ) -> Result> { let selected = target .map(str::to_string) - .or_else(|| config.cli_graph_name().map(str::to_string)); + .or_else(|| config.default_graph_name().map(str::to_string)); if let Some(name) = selected.as_deref() { config.resolve_graph_selection(Some(name))?; return Ok(selected); @@ -2828,7 +2828,7 @@ async fn main() -> Result<()> { || alias_config .and_then(|alias| alias.graph.as_deref()) .is_some() - || config.cli_graph_name().is_some(); + || config.default_graph_name().is_some(); let (legacy_uri, alias_args) = normalize_legacy_alias_uri(legacy_uri, target_available, alias_name, alias_args); let uri = uri.or(legacy_uri); @@ -2914,7 +2914,7 @@ async fn main() -> Result<()> { || alias_config .and_then(|alias| alias.graph.as_deref()) .is_some() - || config.cli_graph_name().is_some(); + || config.default_graph_name().is_some(); let (legacy_uri, alias_args) = normalize_legacy_alias_uri(legacy_uri, target_available, alias_name, alias_args); let uri = uri.or(legacy_uri); @@ -3517,7 +3517,7 @@ policy: } #[test] - fn graph_identity_resolve_cli_graph_named_target_uses_graph_key_not_project_name_or_uri() { + fn graph_identity_resolve_default_graph_named_target_uses_graph_key_not_project_name_or_uri() { let temp = tempdir().unwrap(); let config_path = temp.path().join("omnigraph.yaml"); fs::write( diff --git a/crates/omnigraph-cli/tests/support/mod.rs b/crates/omnigraph-cli/tests/support/mod.rs index bb700fc..ab15d73 100644 --- a/crates/omnigraph-cli/tests/support/mod.rs +++ b/crates/omnigraph-cli/tests/support/mod.rs @@ -123,7 +123,7 @@ version: 1 graphs: local: storage: {} -cli: +defaults: graph: local branch: main query: @@ -145,7 +145,7 @@ servers: graphs: dev: server: dev -cli: +defaults: graph: dev branch: main query: diff --git a/crates/omnigraph-config/src/lib.rs b/crates/omnigraph-config/src/lib.rs index 96c6b07..5c2acd8 100644 --- a/crates/omnigraph-config/src/lib.rs +++ b/crates/omnigraph-config/src/lib.rs @@ -332,8 +332,12 @@ pub struct OmnigraphConfig { pub server: ServerDefaults, #[serde(default)] pub auth: AuthDefaults, - #[serde(default)] - pub cli: CliDefaults, + /// CLI/client defaults (`defaults:`) — RFC-002. The legacy spelling `cli:` + /// is accepted via a serde alias and honored under the legacy schema; under + /// `version: 1` it is rejected at load (see [`legacy_top_level_keys`]). All + /// reads go through the `default_*` accessors. + #[serde(default, alias = "cli")] + pub defaults: CliDefaults, #[serde(default)] pub query: QueryDefaults, #[serde(default)] @@ -353,6 +357,12 @@ pub struct OmnigraphConfig { /// when no `omnigraph.yaml` exists. #[serde(skip)] loaded_from_file: bool, + /// Top-level legacy keys present in the loaded file that v1 renames or + /// removes (see [`legacy_top_level_keys`]). Populated by `load_config_in` + /// from a raw scan; drives the legacy deprecation warnings. Empty under + /// `version: 1` (those keys are rejected at load) and for the built-in default. + #[serde(skip)] + legacy_keys: Vec, } impl Default for OmnigraphConfig { @@ -364,13 +374,14 @@ impl Default for OmnigraphConfig { graphs: BTreeMap::new(), server: ServerDefaults::default(), auth: AuthDefaults::default(), - cli: CliDefaults::default(), + defaults: CliDefaults::default(), query: QueryDefaults::default(), aliases: BTreeMap::new(), policy: PolicySettings::default(), queries: BTreeMap::new(), base_dir: PathBuf::new(), loaded_from_file: false, + legacy_keys: Vec::new(), } } } @@ -380,24 +391,24 @@ impl OmnigraphConfig { &self.base_dir } - pub fn cli_branch(&self) -> &str { - self.cli.branch.as_deref().unwrap_or("main") + pub fn default_branch(&self) -> &str { + self.defaults.branch.as_deref().unwrap_or("main") } - pub fn cli_output_format(&self) -> ReadOutputFormat { - self.cli.output_format.unwrap_or_default() + pub fn default_output_format(&self) -> ReadOutputFormat { + self.defaults.output_format.unwrap_or_default() } pub fn table_max_column_width(&self) -> usize { - self.cli.table_max_column_width.unwrap_or(80) + self.defaults.table_max_column_width.unwrap_or(80) } pub fn table_cell_layout(&self) -> TableCellLayout { - self.cli.table_cell_layout.unwrap_or_default() + self.defaults.table_cell_layout.unwrap_or_default() } - pub fn cli_graph_name(&self) -> Option<&str> { - self.cli.graph.as_deref() + pub fn default_graph_name(&self) -> Option<&str> { + self.defaults.graph.as_deref() } pub fn server_graph_name(&self) -> Option<&str> { @@ -514,7 +525,10 @@ impl OmnigraphConfig { } pub fn resolve_policy_tooling_graph_selection(&self) -> Result> { - self.resolve_graph_selection(self.cli_graph_name().or_else(|| self.server_graph_name())) + self.resolve_graph_selection( + self.default_graph_name() + .or_else(|| self.server_graph_name()), + ) } /// The policy file that applies for a graph selection — the policy @@ -618,14 +632,13 @@ impl OmnigraphConfig { .to_string(), ); } - // Legacy-only notices for keys that `version: 1` rejects at load (so they - // can only reach here under the lenient legacy schema). `project:` has no - // consumer and is removed in v1. - if self.version.is_none() && self.project.name.is_some() { - warnings.push( - "`project:` has no effect and is removed under `version: 1`; delete it." - .to_string(), - ); + // Legacy-only notices for top-level keys that `version: 1` rejects at + // load (so a populated `legacy_keys` only ever occurs under the lenient + // legacy schema). Shares the migration hints with the v1 rejection. + for key in &self.legacy_keys { + if let Some(hint) = legacy_key_migration_hint(key) { + warnings.push(format!("`{key}:` — {hint}")); + } } for (name, entry) in &self.graphs { if entry.storage.is_none() && entry.server.is_none() && !entry.uri.is_empty() { @@ -673,7 +686,7 @@ impl OmnigraphConfig { }); } - let name = name.or_else(|| self.cli_graph_name()).ok_or_else(|| { + let name = name.or_else(|| self.default_graph_name()).ok_or_else(|| { color_eyre::eyre::eyre!("URI must be provided via , --graph, or config") })?; @@ -865,7 +878,7 @@ fn load_config_in(cwd: &Path, config_path: Option<&PathBuf>) -> Result = Vec::new(); let de = serde_yaml::Deserializer::from_str(&text); - let config: OmnigraphConfig = + let mut config: OmnigraphConfig = serde_ignored::deserialize(de, |key| unknown.push(key.to_string()))?; // Strictness is a function of the version, decided here — the one place // the loader holds both the parsed version and the set of ignored fields. @@ -887,7 +900,27 @@ fn load_config_in(cwd: &Path, config_path: Option<&PathBuf>) -> Result {} } - reject_legacy_top_level_keys_under_v1(&text, config.version)?; + // Known-but-legacy top-level keys (renamed/removed by v1) are invisible + // to `serde_ignored` because they stay parseable for the legacy schema, + // so scan the raw text for them: reject under v1, record for the legacy + // deprecation warnings otherwise. + let legacy_keys = legacy_top_level_keys(&text); + if config.version == Some(1) && !legacy_keys.is_empty() { + let offenders = legacy_keys + .iter() + .map(|key| { + format!( + "`{key}:` — {}", + legacy_key_migration_hint(key).unwrap_or("") + ) + }) + .collect::>() + .join("\n "); + bail!( + "invalid key(s) under `version: 1`:\n {offenders}\n(omit `version:` for the legacy lenient schema)" + ); + } + config.legacy_keys = legacy_keys; config } else { OmnigraphConfig::default() @@ -925,34 +958,29 @@ fn absolute_base_dir(cwd: &Path, path: &Path) -> Result { /// silently ignored). The set grows as each rename/removal lands. fn legacy_key_migration_hint(key: &str) -> Option<&'static str> { match key { - "project" => Some("has no effect and is removed under `version: 1`; delete it"), + "project" => Some("remove it; it has no effect under `version: 1`"), + "cli" => Some("rename to `defaults:`"), _ => None, } } -/// Reject any v1-illegal top-level key (see [`legacy_key_migration_hint`]) when -/// `version: 1` is set. Scans the raw top-level mapping rather than the typed -/// config because the offending keys are known struct fields kept for legacy -/// parsing — rejection keys on the *presence* of the key (an empty `policy: {}` -/// under v1 is a silent no-op and must error too), which only a raw scan can see. -fn reject_legacy_top_level_keys_under_v1(text: &str, version: Option) -> Result<()> { - if version != Some(1) { - return Ok(()); - } +/// The top-level keys present in the raw config text that v1 renames or removes +/// (those with a [`legacy_key_migration_hint`]), sorted. Scans the raw mapping +/// rather than the typed config because these keys are honored under the legacy +/// schema (so they stay parseable) — detection keys on the *presence* of the +/// key (an empty `cli: {}`/`policy: {}` under v1 is a silent no-op and must +/// error too), which only a raw scan can see. Drives both the `version: 1` +/// rejection and the legacy deprecation warnings. +fn legacy_top_level_keys(text: &str) -> Vec { let mapping: serde_yaml::Mapping = serde_yaml::from_str(text).unwrap_or_default(); - let mut offenders: Vec = mapping + let mut keys: Vec = mapping .keys() .filter_map(serde_yaml::Value::as_str) - .filter_map(|key| legacy_key_migration_hint(key).map(|hint| format!("`{key}:` — {hint}"))) + .filter(|key| legacy_key_migration_hint(key).is_some()) + .map(str::to_string) .collect(); - if !offenders.is_empty() { - offenders.sort(); - bail!( - "invalid key(s) under `version: 1`:\n {}\n(omit `version:` for the legacy lenient schema)", - offenders.join("\n ") - ); - } - Ok(()) + keys.sort(); + keys } #[cfg(test)] @@ -991,13 +1019,13 @@ policy: {} .unwrap(); let config = load_config_in(temp.path(), None).unwrap(); - assert_eq!(config.cli_graph_name(), Some("local")); - assert_eq!(config.cli_branch(), "main"); - assert_eq!(config.cli_output_format(), ReadOutputFormat::Kv); + assert_eq!(config.default_graph_name(), Some("local")); + assert_eq!(config.default_branch(), "main"); + assert_eq!(config.default_output_format(), ReadOutputFormat::Kv); assert_eq!(config.table_max_column_width(), 40); assert_eq!(config.table_cell_layout(), TableCellLayout::Wrap); assert_eq!( - config.graph_bearer_token_env(None, None, config.cli_graph_name()), + config.graph_bearer_token_env(None, None, config.default_graph_name()), Some("DEMO_TOKEN") ); assert_eq!( @@ -1007,7 +1035,7 @@ policy: {} assert_eq!( PathBuf::from( config - .resolve_target_uri(None, None, config.cli_graph_name()) + .resolve_target_uri(None, None, config.default_graph_name()) .unwrap() ), temp.path().join("./demo.omni") @@ -1328,7 +1356,7 @@ cli: config.graph_bearer_token_env( Some("https://override.example.com"), None, - config.cli_graph_name() + config.default_graph_name() ), None ); @@ -1336,7 +1364,7 @@ cli: config.graph_bearer_token_env( Some("https://override.example.com"), Some("demo"), - config.cli_graph_name() + config.default_graph_name() ), Some("DEMO_TOKEN") ); @@ -1347,15 +1375,15 @@ cli: let temp = tempdir().unwrap(); fs::write( temp.path().join("omnigraph.yaml"), - "version: 1\ngraphs:\n local:\n uri: ./demo.omni\ncli:\n graph: local\n", + "version: 1\ngraphs:\n local:\n uri: ./demo.omni\ndefaults:\n graph: local\n", ) .unwrap(); let config = load_config_in(temp.path(), None).unwrap(); - assert_eq!(config.cli_graph_name(), Some("local")); + assert_eq!(config.default_graph_name(), Some("local")); assert_eq!( PathBuf::from( config - .resolve_target_uri(None, None, config.cli_graph_name()) + .resolve_target_uri(None, None, config.default_graph_name()) .unwrap() ), temp.path().join("./demo.omni") @@ -1390,18 +1418,18 @@ cli: "graphs:\n local:\n uri: ./demo.omni\n\ future_top_level_key: whatever\ncli:\n graph: local\n", ); - assert_eq!(config.cli_graph_name(), Some("local")); + assert_eq!(config.default_graph_name(), Some("local")); } #[test] fn version_one_rejects_unknown_nested_field() { // `version: 1` is strict at ALL depths via serde_ignored, not only at the // structs that carry their own `deny_unknown_fields`: a typo inside a - // nested block such as `cli:` must error, naming the offending key + // nested block such as `defaults:` must error, naming the offending key // (honored-or-rejected, RFC-002 §3). let err = load_yaml_err( "version: 1\ngraphs:\n local:\n uri: ./demo.omni\n\ - cli:\n graph: local\n outout_format: kv\n", + defaults:\n graph: local\n outout_format: kv\n", ); assert!( err.contains("outout_format") || err.contains("unknown config field"), @@ -1409,6 +1437,52 @@ cli: ); } + #[test] + fn version_one_rejects_legacy_cli_key() { + // `cli:` is renamed to `defaults:` under v1; writing it is a hard error + // that points at the new spelling (a known struct field via the `cli` + // alias, so only the raw key scan catches it). + let err = load_yaml_err( + "version: 1\ngraphs:\n local:\n storage: ./demo.omni\ncli:\n graph: local\n", + ); + assert!( + err.contains("cli") && err.contains("rename to `defaults:`"), + "v1 must reject `cli:` and point at `defaults:`: {err}" + ); + } + + #[test] + fn version_one_honors_defaults_block() { + let config = load_yaml( + "version: 1\ngraphs:\n local:\n storage: ./demo.omni\n\ + defaults:\n graph: local\n branch: dev\n output_format: kv\n", + ); + assert_eq!(config.default_graph_name(), Some("local")); + assert_eq!(config.default_branch(), "dev"); + assert_eq!(config.default_output_format(), ReadOutputFormat::Kv); + assert!( + config.deprecation_warnings().is_empty(), + "clean v1 `defaults:` config must not warn: {:?}", + config.deprecation_warnings() + ); + } + + #[test] + fn legacy_cli_key_honored_via_alias_and_warned() { + // No `version:` ⇒ the legacy `cli:` spelling is accepted (serde alias of + // `defaults`) and still honored, but flagged for migration. + let config = load_yaml("graphs:\n local:\n uri: ./demo.omni\ncli:\n graph: local\n"); + assert_eq!(config.default_graph_name(), Some("local")); + assert!( + config + .deprecation_warnings() + .iter() + .any(|w| w.contains("cli") && w.contains("defaults")), + "legacy `cli:` must warn to migrate: {:?}", + config.deprecation_warnings() + ); + } + #[test] fn version_one_rejects_legacy_project_key() { // `version: 1` removes the `project:` block (no consumer). Writing it diff --git a/omnigraph.example.yaml b/omnigraph.example.yaml index f964573..b43fb00 100644 --- a/omnigraph.example.yaml +++ b/omnigraph.example.yaml @@ -11,7 +11,7 @@ graphs: server: prod bearer_token_env: OMNIGRAPH_BEARER_TOKEN -cli: +defaults: graph: local branch: main