From 147da02627771a49063262ca265ed017154ad093 Mon Sep 17 00:00:00 2001 From: Pama-Lee Date: Wed, 15 Jul 2026 23:27:23 +0800 Subject: [PATCH] fix(cli): validate facts.json/concepts.json enums locally, before push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither ordo validate nor ordo push checked facts.json/concepts.json against anything — push.rs's sync_facts/sync_concepts forwarded them to the platform as opaque JSON. The platform's NullPolicy (error/default/ skip) and FactDataType (string/number/boolean/date/object) enums are strict: an unrecognized variant fails the server's JSON deserialization outright, surfacing as a 4xx only at push time, after every prior catalog entry already round-tripped over the network. Found live: a facts.json using null_policy values (reject, default_false, default_zero) that were never valid, undetected because nothing local checked this field and the pushes had been masked by the save_draft_ruleset 500 (now fixed separately) until that stopped hiding it. Add crates/ordo-cli/src/catalog.rs — a small, independently-maintained mirror of the platform's two enums (ordo-cli talks to the platform over HTTP, not as a Rust dependency on ordo-platform, so there's no path to import the real types) — and wire it into both: - ordo push: validated before sync_facts/sync_concepts is ever called, so an invalid catalog file fails fast locally with a specific per-field message, no network round-trip. - ordo validate: reported as a facts.json/concepts.json entry alongside ruleset reports, reusing the exact same report/print/JSON shape (extends what its own doc comment already promises: catching what the platform would reject, offline). Moved push.rs's private read_array to project.rs as read_json_array so both push and validate can share it. Verified end-to-end through the compiled binary (not just unit-level): push rejects a bad null_policy/data_type without ever attempting a network call (ORDO_API_URL points at a guaranteed-refused port, so a regression that bypasses validation fails the test fast instead of hanging), and ordo validate reports the same. cargo test/clippy/fmt clean. --- crates/ordo-cli/src/catalog.rs | 132 +++++++++++++++++ crates/ordo-cli/src/main.rs | 1 + crates/ordo-cli/src/project.rs | 17 +++ crates/ordo-cli/src/push.rs | 59 ++++---- crates/ordo-cli/src/validate.rs | 64 +++++++- .../ordo-cli/tests/push_catalog_validation.rs | 138 ++++++++++++++++++ 6 files changed, 384 insertions(+), 27 deletions(-) create mode 100644 crates/ordo-cli/src/catalog.rs create mode 100644 crates/ordo-cli/tests/push_catalog_validation.rs diff --git a/crates/ordo-cli/src/catalog.rs b/crates/ordo-cli/src/catalog.rs new file mode 100644 index 00000000..b479af5e --- /dev/null +++ b/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 { + 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 { + 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 { + 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")); + } +} diff --git a/crates/ordo-cli/src/main.rs b/crates/ordo-cli/src/main.rs index 0690762b..9ff50c16 100644 --- a/crates/ordo-cli/src/main.rs +++ b/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; diff --git a/crates/ordo-cli/src/project.rs b/crates/ordo-cli/src/project.rs index b9235187..5993a5fc 100644 --- a/crates/ordo-cli/src/project.rs +++ b/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>> { + 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)] diff --git a/crates/ordo-cli/src/push.rs b/crates/ordo-cli/src/push.rs index a438cbf0..476dbcd6 100644 --- a/crates/ordo-cli/src/push.rs +++ b/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>> { - 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 { items .iter() diff --git a/crates/ordo-cli/src/validate.rs b/crates/ordo-cli/src/validate.rs index 761a133f..abfc3d08 100644 --- a/crates/ordo-cli/src/validate.rs +++ b/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 ` 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 +/// `✓/✗ ` 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, +) -> Result> { + 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::>(); + Ok(Some(RulesetReport { + ruleset: label.to_string(), + ok: errors.is_empty(), + errors, + })) +} diff --git a/crates/ordo-cli/tests/push_catalog_validation.rs b/crates/ordo-cli/tests/push_catalog_validation.rs new file mode 100644 index 00000000..9e208288 --- /dev/null +++ b/crates/ordo-cli/tests/push_catalog_validation.rs @@ -0,0 +1,138 @@ +//! Integration test: `ordo push` rejects an invalid `facts.json`/`concepts.json` +//! locally, before any network call — drives the real compiled binary. +//! +//! `ORDO_API_URL` points at a port nothing listens on (an immediate connection +//! refusal, not a slow DNS/timeout failure) so that if the local validation +//! gate is ever accidentally bypassed, this test fails fast and clearly +//! instead of hanging. + +use std::path::PathBuf; +use std::process::{Command, Output}; + +const BIN: &str = env!("CARGO_BIN_EXE_ordo"); +const UNREACHABLE_API_URL: &str = "http://127.0.0.1:1"; + +fn temp_project(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "ordo-push-catalog-it-{tag}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("ordo.yaml"), + "project: test\norg_id: org1\nproject_id: proj1\n", + ) + .unwrap(); + dir +} + +fn run(dir: &PathBuf, args: &[&str]) -> Output { + Command::new(BIN) + .args(args) + .current_dir(dir) + .env("ORDO_TOKEN", "test-token") + .env("ORDO_API_URL", UNREACHABLE_API_URL) + .output() + .expect("failed to run ordo") +} + +fn stdout(out: &Output) -> String { + String::from_utf8_lossy(&out.stdout).to_string() +} + +#[test] +fn push_rejects_invalid_null_policy_without_touching_the_network() { + let dir = temp_project("null-policy"); + std::fs::write( + dir.join("facts.json"), + r#"[{"name":"user_score","data_type":"number","source":"input","null_policy":"reject"}]"#, + ) + .unwrap(); + + let out = run(&dir, &["push", "--json"]); + assert!( + !out.status.success(), + "push must fail when facts.json is invalid" + ); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).unwrap(); + let facts_row = v["results"] + .as_array() + .unwrap() + .iter() + .find(|r| r["path"] == "facts.json") + .expect("a facts.json result row"); + let status = facts_row["status"].as_str().unwrap(); + assert!(status.starts_with("invalid:"), "got: {status}"); + assert!(status.contains("user_score"), "got: {status}"); + assert!(status.contains("null_policy"), "got: {status}"); + assert!(status.contains("\"reject\""), "got: {status}"); + assert!( + status.contains("error, default, skip"), + "must list the valid values: {status}" + ); +} + +#[test] +fn push_rejects_invalid_data_type_in_concepts() { + let dir = temp_project("concept-data-type"); + std::fs::write( + dir.join("concepts.json"), + r#"[{"name":"risk_band","data_type":"int","expression":"1"}]"#, + ) + .unwrap(); + + let out = run(&dir, &["push", "--json"]); + assert!(!out.status.success()); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).unwrap(); + let row = v["results"] + .as_array() + .unwrap() + .iter() + .find(|r| r["path"] == "concepts.json") + .expect("a concepts.json result row"); + let status = row["status"].as_str().unwrap(); + assert!(status.starts_with("invalid:"), "got: {status}"); + assert!(status.contains("risk_band"), "got: {status}"); + assert!(status.contains("data_type"), "got: {status}"); +} + +#[test] +fn push_with_no_facts_or_concepts_files_has_nothing_to_validate() { + // No rulesets, no facts.json, no concepts.json: push has nothing to do and + // must not error just because the catalog is absent. + let dir = temp_project("empty"); + let out = run(&dir, &["push", "--json"]); + assert!(out.status.success(), "stdout: {}", stdout(&out)); +} + +#[test] +fn ordo_validate_reports_invalid_facts_json_too() { + let dir = temp_project("validate-null-policy"); + std::fs::write( + dir.join("facts.json"), + r#"[{"name":"score","data_type":"number","source":"input","null_policy":"default_zero"}]"#, + ) + .unwrap(); + + let out = Command::new(BIN) + .args(["validate", "--json"]) + .current_dir(&dir) + .output() + .expect("failed to run ordo validate"); + assert!(!out.status.success()); + let v: serde_json::Value = serde_json::from_str(&stdout(&out)).unwrap(); + assert_eq!(v["ok"], false); + let rulesets = v["rulesets"].as_array().unwrap(); + let facts_report = rulesets + .iter() + .find(|r| r["ruleset"] == "facts.json") + .expect("a facts.json report"); + assert_eq!(facts_report["ok"], false); + let message = facts_report["errors"][0]["message"].as_str().unwrap(); + assert!(message.contains("null_policy"), "got: {message}"); + assert!(message.contains("default_zero"), "got: {message}"); +}