Merge pull request #200 from ModernRelay/feat/no-legacy-config-strict

feat(config): OMNIGRAPH_NO_LEGACY_CONFIG strict mode (RFC-008 stage 4)

Author: aaltshuler

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:?}");
+}

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<OmnigraphConfig> {
     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<OmnigraphConfig> {
     // 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::<OmnigraphConfig>(&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,15 +872,15 @@ 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")
         );
 
         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"),

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

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 }