fix(cli): validate facts.json/concepts.json enums locally, before push

crates/ordo-cli/src/catalog.rs

@@ -0,0 +1,132 @@
+//! Local validation for `facts.json` / `concepts.json` against the enum
+//! values the platform's catalog endpoints actually accept — mirroring
+//! `NullPolicy` / `FactDataType` in `ordo-platform`'s `models/catalog.rs`.
+//!
+//! Duplicated deliberately, not shared: `ordo-cli` talks to the platform over
+//! HTTP via `ordo-api-client`, not as a Rust dependency on `ordo-platform`, so
+//! there's no existing path to import the real enum types, and pulling one in
+//! just for this would be the wrong direction. This is a small,
+//! independently-maintained mirror of the two enums the platform enforces
+//! strictly (an unrecognized variant fails the server's JSON
+//! deserialization outright, surfacing as an unhelpful round-trip to a 4xx)
+//! — catching it here means a bad value fails locally with a clear message
+//! before any network call.
+//!
+//! Scoped to exactly these two enums, not full schema validation: a missing
+//! *required* field the platform doesn't constrain to an enum (e.g. an empty
+//! `source` on a fact) still only surfaces at push time.
+
+use serde_json::Value;
+
+const VALID_DATA_TYPES: &[&str] = &["string", "number", "boolean", "date", "object"];
+const VALID_NULL_POLICIES: &[&str] = &["error", "default", "skip"];
+
+/// Validate every fact's `data_type` and `null_policy` against the platform's
+/// accepted values. Returns one message per bad field, so multiple failures
+/// in one file are all reported together rather than one-at-a-time.
+pub fn validate_facts(facts: &[Value]) -> Vec<String> {
+    facts
+        .iter()
+        .enumerate()
+        .flat_map(|(i, f)| {
+            let label = entry_label(f, i);
+            let mut errors = check_field(f, "data_type", VALID_DATA_TYPES, &label);
+            errors.extend(check_field(f, "null_policy", VALID_NULL_POLICIES, &label));
+            errors
+        })
+        .collect()
+}
+
+/// Concepts share the same `data_type` enum as facts but have no
+/// `null_policy` field.
+pub fn validate_concepts(concepts: &[Value]) -> Vec<String> {
+    concepts
+        .iter()
+        .enumerate()
+        .flat_map(|(i, c)| {
+            let label = entry_label(c, i);
+            check_field(c, "data_type", VALID_DATA_TYPES, &label)
+        })
+        .collect()
+}
+
+fn entry_label(entry: &Value, index: usize) -> String {
+    entry
+        .get("name")
+        .and_then(Value::as_str)
+        .map(str::to_string)
+        .unwrap_or_else(|| format!("#{}", index + 1))
+}
+
+fn check_field(entry: &Value, field: &str, allowed: &[&str], label: &str) -> Vec<String> {
+    let Some(value) = entry.get(field) else {
+        return vec![format!("{label}: missing \"{field}\"")];
+    };
+    let Some(s) = value.as_str() else {
+        return vec![format!(
+            "{label}: \"{field}\" must be a string, got {value}"
+        )];
+    };
+    if allowed.contains(&s) {
+        Vec::new()
+    } else {
+        vec![format!(
+            "{label}: \"{field}\" is \"{s}\" — must be one of: {}",
+            allowed.join(", ")
+        )]
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use serde_json::json;
+
+    #[test]
+    fn accepts_all_valid_values() {
+        let facts = vec![
+            json!({ "name": "a", "data_type": "string", "null_policy": "error" }),
+            json!({ "name": "b", "data_type": "number", "null_policy": "default" }),
+            json!({ "name": "c", "data_type": "boolean", "null_policy": "skip" }),
+        ];
+        assert!(validate_facts(&facts).is_empty());
+    }
+
+    #[test]
+    fn rejects_legacy_null_policy_aliases() {
+        // The exact incident this guards against: a name that reads like a
+        // plausible synonym of the real enum but was never valid.
+        let facts = vec![json!({
+            "name": "user_score", "data_type": "number", "null_policy": "reject"
+        })];
+        let errors = validate_facts(&facts);
+        assert_eq!(errors.len(), 1);
+        assert!(errors[0].contains("user_score"));
+        assert!(errors[0].contains("null_policy"));
+        assert!(errors[0].contains("\"reject\""));
+        assert!(errors[0].contains("error, default, skip"));
+    }
+
+    #[test]
+    fn rejects_bad_data_type_and_reports_index_when_unnamed() {
+        let facts = vec![json!({ "data_type": "int", "null_policy": "error" })];
+        let errors = validate_facts(&facts);
+        assert_eq!(errors.len(), 1);
+        assert!(errors[0].starts_with("#1:"), "got: {}", errors[0]);
+        assert!(errors[0].contains("data_type"));
+    }
+
+    #[test]
+    fn reports_missing_field() {
+        let facts = vec![json!({ "name": "x", "data_type": "string" })];
+        assert_eq!(validate_facts(&facts), vec!["x: missing \"null_policy\""]);
+    }
+
+    #[test]
+    fn concepts_only_check_data_type() {
+        let concepts = vec![json!({ "name": "risk", "data_type": "notatype" })];
+        let errors = validate_concepts(&concepts);
+        assert_eq!(errors.len(), 1);
+        assert!(errors[0].contains("data_type"));
+    }
+}

crates/ordo-cli/src/main.rs

@@ -2,6 +2,7 @@ use anyhow::Result;
 use clap::{CommandFactory, Parser, Subcommand};
 
 mod api;
+mod catalog;
 mod config;
 mod deployments;
 mod diff;

crates/ordo-cli/src/project.rs

@@ -33,6 +33,23 @@ pub fn ruleset_name(id: &str) -> String {
     s.strip_suffix(".json").unwrap_or(s).to_string()
 }
 
+/// Read a JSON array file as raw `Value`s (not a typed struct — used where the
+/// caller needs every field, including ones a typed deserialize would drop or
+/// reject). `None` when the file doesn't exist; an empty file is `Some(vec![])`.
+pub fn read_json_array(path: &Path) -> Result<Option<Vec<serde_json::Value>>> {
+    if !path.is_file() {
+        return Ok(None);
+    }
+    let text = std::fs::read_to_string(path)
+        .with_context(|| format!("failed to read {}", path.display()))?;
+    if text.trim().is_empty() {
+        return Ok(Some(Vec::new()));
+    }
+    serde_json::from_str(&text)
+        .map(Some)
+        .with_context(|| format!("invalid JSON in {}", path.display()))
+}
+
 /// Project + link configuration, persisted as `ordo.yaml`. The link fields are
 /// populated by `ordo link` (Phase 2) and are safe to commit (ids, not secrets).
 #[derive(Debug, Clone, Serialize, Deserialize)]

crates/ordo-cli/src/push.rs

@@ -65,24 +65,45 @@ pub async fn run(args: PushArgs, json: bool) -> Result<()> {
 
     if !args.rulesets_only && !had_error {
         // 2. Facts + concepts (whole-catalog sync: upsert local, delete server-only).
-        if let Some(facts) = read_array(&project.facts_path())? {
-            let (up, del) = sync_facts(client, proj, &facts).await.map_err(anyerr)?;
-            results.push(("facts.json".into(), format!("{up} upserted, {del} removed")));
+        // Validated locally first — an invalid data_type/null_policy would
+        // otherwise only surface as a 4xx from the server, after every prior
+        // entry already round-tripped over the network.
+        if let Some(facts) = crate::project::read_json_array(&project.facts_path())? {
+            let errors = crate::catalog::validate_facts(&facts);
+            if errors.is_empty() {
+                let (up, del) = sync_facts(client, proj, &facts).await.map_err(anyerr)?;
+                results.push(("facts.json".into(), format!("{up} upserted, {del} removed")));
+            } else {
+                had_error = true;
+                results.push((
+                    "facts.json".into(),
+                    format!("invalid: {}", errors.join("; ")),
+                ));
+            }
         }
-        if let Some(concepts) = read_array(&project.concepts_path())? {
-            let (up, del) = sync_concepts(client, proj, &concepts)
-                .await
-                .map_err(anyerr)?;
-            results.push((
-                "concepts.json".into(),
-                format!("{up} upserted, {del} removed"),
-            ));
+        if let Some(concepts) = crate::project::read_json_array(&project.concepts_path())? {
+            let errors = crate::catalog::validate_concepts(&concepts);
+            if errors.is_empty() {
+                let (up, del) = sync_concepts(client, proj, &concepts)
+                    .await
+                    .map_err(anyerr)?;
+                results.push((
+                    "concepts.json".into(),
+                    format!("{up} upserted, {del} removed"),
+                ));
+            } else {
+                had_error = true;
+                results.push((
+                    "concepts.json".into(),
+                    format!("invalid: {}", errors.join("; ")),
+                ));
+            }
         }
 
         // 3. Tests (per ruleset, keyed by name).
         for name in &names {
             let path = project.tests_path(name);
-            if let Some(tests) = read_array(&path)? {
+            if let Some(tests) = crate::project::read_json_array(&path)? {
                 sync_tests(client, proj, name, &tests).await?;
                 results.push((format!("tests/{name}"), format!("{} synced", tests.len())));
             }
@@ -124,20 +145,6 @@ fn anyerr(e: ApiError) -> anyhow::Error {
     anyhow::anyhow!("{e}")
 }
 
-fn read_array(path: &std::path::Path) -> Result<Option<Vec<Value>>> {
-    if !path.is_file() {
-        return Ok(None);
-    }
-    let text = std::fs::read_to_string(path)
-        .with_context(|| format!("failed to read {}", path.display()))?;
-    if text.trim().is_empty() {
-        return Ok(Some(Vec::new()));
-    }
-    serde_json::from_str(&text)
-        .map(Some)
-        .with_context(|| format!("invalid JSON in {}", path.display()))
-}
-
 fn names_of(items: &[Value]) -> HashSet<String> {
     items
         .iter()

crates/ordo-cli/src/validate.rs

@@ -9,6 +9,7 @@ use clap::Args;
 use ordo_core::prelude::RuleSet;
 use ordo_studio_format::{materialize_concepts, ConvertError};
 use serde::Serialize;
+use std::path::Path;
 
 use crate::project::Project;
 
@@ -34,16 +35,36 @@ pub(crate) struct ValidationError {
 
 pub fn run(args: ValidateArgs, json: bool) -> Result<()> {
     let project = Project::discover(None)?;
+    let validating_whole_project = args.name.is_none();
     let names = match args.name {
         Some(n) => vec![crate::project::ruleset_name(&n)],
         None => project.ruleset_names()?,
     };
     let concepts = project.load_concepts()?;
 
-    let mut reports = Vec::with_capacity(names.len());
+    let mut reports = Vec::with_capacity(names.len() + 2);
     for name in &names {
         reports.push(validate_one(&project, name, &concepts));
     }
+    // Catalog-level (not per-ruleset), but a bad data_type/null_policy here
+    // would otherwise only surface as a 4xx from `ordo push` — same class of
+    // problem `validate_one` already catches for rulesets, extended to the
+    // two files it never touched. Only when validating the whole project —
+    // `ordo validate <one-ruleset>` shouldn't also report on unrelated files.
+    if validating_whole_project {
+        if let Some(report) = validate_catalog_file(&project.facts_path(), "facts.json", |v| {
+            crate::catalog::validate_facts(v)
+        })? {
+            reports.push(report);
+        }
+        if let Some(report) =
+            validate_catalog_file(&project.concepts_path(), "concepts.json", |v| {
+                crate::catalog::validate_concepts(v)
+            })?
+        {
+            reports.push(report);
+        }
+    }
 
     let all_ok = reports.iter().all(|r| r.ok);
 
@@ -161,3 +182,44 @@ pub(crate) fn validate_one(
         errors,
     }
 }
+
+/// Validate a catalog file (`facts.json` / `concepts.json`) with `validator`,
+/// wrapped as a `RulesetReport` under `label` — reuses the exact same
+/// report/print/JSON shape as a ruleset, since the CLI output already prints
+/// `✓/✗ <name>` generically. `None` when the file doesn't exist (consistent
+/// with `ordo push`'s "an absent catalog file is skipped, not an error"). A
+/// read/parse failure becomes a failed report rather than aborting the whole
+/// `ordo validate` run, matching how `validate_one` handles an unreadable
+/// ruleset.
+fn validate_catalog_file(
+    path: &Path,
+    label: &str,
+    validator: impl Fn(&[serde_json::Value]) -> Vec<String>,
+) -> Result<Option<RulesetReport>> {
+    let values = match crate::project::read_json_array(path) {
+        Ok(None) => return Ok(None),
+        Ok(Some(v)) => v,
+        Err(e) => {
+            return Ok(Some(RulesetReport {
+                ruleset: label.to_string(),
+                ok: false,
+                errors: vec![ValidationError {
+                    step_id: None,
+                    message: format!("{e:#}"),
+                }],
+            }))
+        }
+    };
+    let errors = validator(&values)
+        .into_iter()
+        .map(|message| ValidationError {
+            step_id: None,
+            message,
+        })
+        .collect::<Vec<_>>();
+    Ok(Some(RulesetReport {
+        ruleset: label.to_string(),
+        ok: errors.is_empty(),
+        errors,
+    }))
+}