From 4c50170c773de9a36ee126c551f814a91545656b Mon Sep 17 00:00:00 2001 From: aaltshuler Date: Fri, 12 Jun 2026 00:03:10 +0300 Subject: [PATCH] feat(config): OMNIGRAPH_NO_LEGACY_CONFIG strict mode (RFC-008 stage 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opt-in: with the env set, loading a legacy omnigraph.yaml is a hard error pointing at config migrate — the regression guard for migrated teams (a stray legacy file would otherwise silently outrank operator config during the window) and the rehearsal for stage 5's removal. Strict refuses the FILE, never its absence: flag-less invocations on migrated setups are untouched. Inert unless set. The RFC's stages-1-3-then-4 release gap collapsed honestly: no version boundary was crossed between them, so all four ship in the same release (noted in the RFC). Co-Authored-By: Claude Fable 5 --- .../omnigraph-cli/tests/cli_schema_config.rs | 36 +++++++++ crates/omnigraph-server/src/config.rs | 74 ++++++++++++++----- docs/dev/rfc-008-deprecate-omnigraph-yaml.md | 2 +- docs/user/cli-reference.md | 4 +- 4 files changed, 95 insertions(+), 21 deletions(-) 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 }