diff --git a/crates/omnigraph-cli/tests/cli_schema_config.rs b/crates/omnigraph-cli/tests/cli_schema_config.rs index 3e2a2b9..710c856 100644 --- a/crates/omnigraph-cli/tests/cli_schema_config.rs +++ b/crates/omnigraph-cli/tests/cli_schema_config.rs @@ -610,3 +610,39 @@ fn config_migrate_splits_legacy_config() { assert!(output.status.success(), "{output:?}"); assert!(temp.path().join("cluster.yaml.proposed").exists()); } + +/// RFC-008 stage 4: OMNIGRAPH_NO_LEGACY_CONFIG refuses a present legacy +/// file (pointing at config migrate) but changes nothing on migrated +/// setups with no file. +#[test] +fn strict_mode_refuses_legacy_file_but_not_its_absence() { + let temp = tempdir().unwrap(); + fs::write(temp.path().join("omnigraph.yaml"), "cli:\n actor: a\n").unwrap(); + let output = cli() + .current_dir(temp.path()) + .env("OMNIGRAPH_NO_LEGACY_CONFIG", "1") + .arg("graphs") + .arg("list") + .arg("--json") + .output() + .unwrap(); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("OMNIGRAPH_NO_LEGACY_CONFIG") && stderr.contains("config migrate"), + "{stderr}" + ); + + // Migrated setup (no file): strict mode is a no-op — a config-loading + // command that tolerates empty defaults succeeds. + let clean = tempdir().unwrap(); + let output = cli() + .current_dir(clean.path()) + .env("OMNIGRAPH_NO_LEGACY_CONFIG", "1") + .arg("queries") + .arg("list") + .arg("--json") + .output() + .unwrap(); + assert!(output.status.success(), "{output:?}"); +} diff --git a/crates/omnigraph-server/src/config.rs b/crates/omnigraph-server/src/config.rs index ea7ce30..15b957d 100644 --- a/crates/omnigraph-server/src/config.rs +++ b/crates/omnigraph-server/src/config.rs @@ -531,15 +531,24 @@ pub fn default_config_path() -> PathBuf { /// uses it for the server; RFC-007 §D1 extends it to the CLI). pub const CONFIG_PATH_ENV: &str = "OMNIGRAPH_CONFIG"; +/// RFC-008 stage 4 — opt-in strict mode: when set, loading a legacy +/// `omnigraph.yaml` is a hard error instead of a warning. For teams that +/// finished migrating and want regressions caught (a stray legacy file +/// would otherwise silently outrank operator config during the window). +/// The rehearsal for stage 5's removal. +pub const NO_LEGACY_CONFIG_ENV: &str = "OMNIGRAPH_NO_LEGACY_CONFIG"; + pub fn load_config(config_path: Option<&PathBuf>) -> Result { let env_path = env::var_os(CONFIG_PATH_ENV).map(PathBuf::from); - load_config_in(&env::current_dir()?, config_path, env_path.as_ref()) + let strict = env::var_os(NO_LEGACY_CONFIG_ENV).is_some(); + load_config_in(&env::current_dir()?, config_path, env_path.as_ref(), strict) } fn load_config_in( cwd: &Path, config_path: Option<&PathBuf>, env_path: Option<&PathBuf>, + strict_no_legacy: bool, ) -> Result { // Precedence: explicit --config flag > $OMNIGRAPH_CONFIG > ./omnigraph.yaml. let explicit_path = config_path.or(env_path).cloned(); @@ -549,6 +558,14 @@ fn load_config_in( }); let mut config = if let Some(path) = &config_path { + if strict_no_legacy { + // Strict refuses the FILE, not its absence — flag-less + // invocations on migrated setups keep working. + bail!( + "legacy config '{}' refused: {NO_LEGACY_CONFIG_ENV} is set (RFC-008 strict mode); run `omnigraph config migrate`, then remove the file — or unset the variable", + path.display() + ); + } let text = fs::read_to_string(path)?; warn_yaml_deprecation_once(path, &text); serde_yaml::from_str::(&text)? @@ -665,19 +682,38 @@ mod tests { fs::write(&env_path, "cli:\n actor: act-env\n").unwrap(); // $OMNIGRAPH_CONFIG used when no flag… - let config = load_config_in(temp.path(), None, Some(&env_path)).unwrap(); + let config = load_config_in(temp.path(), None, Some(&env_path), false).unwrap(); assert_eq!(config.cli.actor.as_deref(), Some("act-env")); // …loses to an explicit --config… - let config = load_config_in(temp.path(), Some(&flag_path), Some(&env_path)).unwrap(); + let config = load_config_in(temp.path(), Some(&flag_path), Some(&env_path), false).unwrap(); assert_eq!(config.cli.actor.as_deref(), Some("act-flag")); // …and beats the cwd default file. fs::write(temp.path().join("omnigraph.yaml"), "cli:\n actor: act-cwd\n").unwrap(); - let config = load_config_in(temp.path(), None, Some(&env_path)).unwrap(); + let config = load_config_in(temp.path(), None, Some(&env_path), false).unwrap(); assert_eq!(config.cli.actor.as_deref(), Some("act-env")); } + #[test] + fn strict_mode_refuses_the_file_not_its_absence() { + let temp = tempdir().unwrap(); + // No file: strict mode changes nothing (defaults load). + let config = load_config_in(temp.path(), None, None, true).unwrap(); + assert!(config.cli.actor.is_none()); + + // File present: strict refuses with the migrate pointer. + fs::write(temp.path().join("omnigraph.yaml"), "cli:\n actor: a\n").unwrap(); + let err = load_config_in(temp.path(), None, None, true).unwrap_err(); + let message = err.to_string(); + assert!( + message.contains("OMNIGRAPH_NO_LEGACY_CONFIG") && message.contains("config migrate"), + "{message}" + ); + // Without strict, the same file loads. + assert!(load_config_in(temp.path(), None, None, false).is_ok()); + } + #[test] fn yaml_deprecation_lines_name_present_keys_only() { let lines = super::yaml_deprecation_lines( @@ -717,7 +753,7 @@ policy: {} ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); assert_eq!(config.cli_graph_name(), Some("local")); assert_eq!(config.cli_branch(), "main"); assert_eq!(config.cli_output_format(), ReadOutputFormat::Kv); @@ -752,7 +788,7 @@ policy: {} ) .unwrap(); - let config = load_config_in(&child, None, None).unwrap(); + let config = load_config_in(&child, None, None, false).unwrap(); assert!(config.graphs.is_empty()); } @@ -776,7 +812,7 @@ policy: {} "graphs:\n local:\n uri: ./demo.omni\n", ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); // A known graph passes through unchanged. assert_eq!(config.resolve_graph_selection(Some("local")).unwrap(), Some("local")); @@ -799,7 +835,7 @@ policy: {} "graphs:\n local:\n uri: ./demo.omni\npolicy:\n file: ./top.yaml\n", ) .unwrap(); - let incoherent = load_config_in(temp2.path(), None, None).unwrap(); + let incoherent = load_config_in(temp2.path(), None, None, false).unwrap(); let err = incoherent .resolve_graph_selection(Some("local")) .unwrap_err() @@ -824,7 +860,7 @@ policy: {} server:\n graph: local\ncli:\n graph: prod\n", ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); assert_eq!( config.resolve_policy_tooling_graph_selection().unwrap(), Some("prod") @@ -836,7 +872,7 @@ policy: {} "graphs:\n local:\n uri: ./local.omni\nserver:\n graph: local\n", ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); assert_eq!( config.resolve_policy_tooling_graph_selection().unwrap(), Some("local") @@ -844,7 +880,7 @@ policy: {} let temp = tempdir().unwrap(); fs::write(temp.path().join("omnigraph.yaml"), "policy: {}\n").unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); assert_eq!(config.resolve_policy_tooling_graph_selection().unwrap(), None); let temp = tempdir().unwrap(); @@ -853,7 +889,7 @@ policy: {} "graphs:\n local:\n uri: ./local.omni\nserver:\n graph: ghost\n", ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); let err = config .resolve_policy_tooling_graph_selection() .unwrap_err() @@ -879,7 +915,7 @@ policy: {} ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); let resolved = config.resolve_query_path(Path::new("test.gq")).unwrap(); assert_eq!(resolved, temp.path().join("queries").join("test.gq")); } @@ -896,7 +932,7 @@ policy: {} fs::write(ambient_dir.join("local.gq"), "query ambient { return {} }").unwrap(); let config = - load_config_in(&ambient_dir, Some(&config_dir.join("omnigraph.yaml")), None).unwrap(); + load_config_in(&ambient_dir, Some(&config_dir.join("omnigraph.yaml")), None, false).unwrap(); let resolved = config.resolve_query_path(Path::new("local.gq")).unwrap(); assert_eq!(resolved, config_dir.join("local.gq")); @@ -926,7 +962,7 @@ queries: ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); // Per-graph registry (multi-graph mode). let prod = config.target_query_entries("prod").unwrap(); @@ -967,7 +1003,7 @@ queries: policy:\n file: ./prod.yaml\n bare:\n uri: s3://b/bare\n", ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); // Named graph with its own policy → per-graph (not top-level). assert!( @@ -1003,7 +1039,7 @@ queries: ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); // Additive: no `queries:` anywhere → empty registries everywhere. assert!(config.query_entries().is_empty()); assert!( @@ -1023,7 +1059,7 @@ queries: ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); assert_eq!( config.resolve_policy_file().unwrap(), temp.path().join("policy.yaml") @@ -1046,7 +1082,7 @@ cli: ) .unwrap(); - let config = load_config_in(temp.path(), None, None).unwrap(); + let config = load_config_in(temp.path(), None, None, false).unwrap(); assert_eq!( config.graph_bearer_token_env( Some("https://override.example.com"), diff --git a/docs/dev/rfc-008-deprecate-omnigraph-yaml.md b/docs/dev/rfc-008-deprecate-omnigraph-yaml.md index a59be52..72baaf6 100644 --- a/docs/dev/rfc-008-deprecate-omnigraph-yaml.md +++ b/docs/dev/rfc-008-deprecate-omnigraph-yaml.md @@ -132,7 +132,7 @@ contract), retirement is staged, loud, and tooled: hand-edited anyway (Terraform has no config scaffolder either). New users copy from the cluster quick-start; migrants get a ready-to-review `cluster.yaml` from `config migrate`. -4. **Opt-in strict.** `OMNIGRAPH_NO_LEGACY_CONFIG=1` turns the warning into +4. **Opt-in strict** *(landed — the release gap to stages 1–3 collapsed: no version boundary was crossed between them, so all four ship in the same release)*. `OMNIGRAPH_NO_LEGACY_CONFIG=1` turns the warning into an error — for teams that finished migrating and want regressions caught. 5. **Remove at the next major.** Loading the file becomes an error pointing at `config migrate`. The `OmnigraphConfig` code path, the dual diff --git a/docs/user/cli-reference.md b/docs/user/cli-reference.md index 62e3e0d..b419adf 100644 --- a/docs/user/cli-reference.md +++ b/docs/user/cli-reference.md @@ -109,7 +109,9 @@ operator server use the legacy chain alone. > naming each present key's new home (suppress in CI with > `OMNIGRAPH_SUPPRESS_YAML_DEPRECATION=1`); `omnigraph config migrate` > produces the split. The file keeps working through the deprecation -> window. +> window. Migrated teams can set `OMNIGRAPH_NO_LEGACY_CONFIG=1` to turn +> any legacy-file load into a hard error (regression guard; the file's +> absence is always fine). ```yaml project: { name }