Merge PR #2022: Reject unparseable conditional-format formulas (#1999)

crates/engine/src/cmd/insert.rs: kept both sides: PR #884's is_string_literal and cf_style_json, and PR #2022's check_cf_* before edit(); 884's kept string literals parse

crates/engine/src/cmd/insert.rs

@@ -1396,6 +1396,49 @@ fn cf_style_json(st: &Json) -> Result<Json> {
     Ok(Json::Object(st))
 }
 
+/// #1999: a formula that doesn't parse must never reach the workbook — Excel refuses to open
+/// files carrying it, even in repair mode. Validated here so the API and the New Rule dialog
+/// (which dispatches this command) share one gate, the way Excel's object model refuses.
+fn check_cf_formula(cmd: &str, what: &str, formula: &str) -> Result<()> {
+    // Strip exactly one leading '=', the way the XLSX writer does: a value like '===A1'
+    // only parses after several strips and would still be saved unparseable. (#1999 review)
+    if gridcraft_formula::parse(formula.strip_prefix('=').unwrap_or(formula)).is_err() {
+        return Err(bad(cmd, format!("invalid {what} formula `{formula}` — it does not parse; Excel would reject the saved file")));
+    }
+    Ok(())
+}
+
+fn check_cf_value(what: &str, v: &gridcraft_model::features::CfValueKind) -> Result<()> {
+    if let gridcraft_model::features::CfValueKind::Formula(f) = v {
+        check_cf_formula("home.conditionalFormat", what, f)?;
+    }
+    Ok(())
+}
+
+fn check_cf_rule(rule: &CfRule) -> Result<()> {
+    const CMD: &str = "home.conditionalFormat";
+    match rule {
+        CfRule::CellIs { a, b, .. } => {
+            check_cf_formula(CMD, "comparison value", a)?;
+            if let Some(b) = b {
+                check_cf_formula(CMD, "second comparison value", b)?;
+            }
+        }
+        CfRule::Expression { formula, .. } => check_cf_formula(CMD, "rule", formula)?,
+        CfRule::DataBar { min, max, .. } => {
+            check_cf_value("data bar minimum", min)?;
+            check_cf_value("data bar maximum", max)?;
+        }
+        CfRule::ColorScale { stops } => {
+            for (kind, _) in stops {
+                check_cf_value("color scale threshold", kind)?;
+            }
+        }
+        _ => {}
+    }
+    Ok(())
+}
+
 fn add_cf(s: &mut Session, p: &Json) -> Result<Json> {
     let ranges = target_ranges(s, p)?;
     let sheet = target_sheet(s, p)?;
@@ -1509,6 +1552,7 @@ fn add_cf(s: &mut Session, p: &Json) -> Result<Json> {
         },
         other => return Err(bad("home.conditionalFormat", format!("unknown rule type `{other}`"))),
     };
+    check_cf_rule(&rule)?;
     edit(s, |cx| {
         let sh = cx.sheet_mut(sheet)?;
         let priority = sh.cond_formats.iter().map(|c| c.priority).max().unwrap_or(0) + 1;

crates/engine/tests/cf_formula_validate.rs

@@ -0,0 +1,61 @@
+//! #1999: a conditional-format rule whose formula doesn't parse must be rejected at
+//! creation (Excel's object model refuses it too), not saved and poison the file.
+
+use gridcraft_engine::Session;
+use serde_json::json;
+
+fn session() -> Session {
+    let mut s = Session::new();
+    s.new_workbook();
+    s
+}
+
+#[test]
+fn invalid_cf_formulas_are_rejected() {
+    for rule in [
+        // exact repros from the issue's measured table
+        json!({"type": "expression", "formula": "=A1>", "preset": "lightRedFill"}),
+        json!({"type": "cellIs", "operator": "greaterThan", "value": "=1+", "preset": "lightRedFill"}),
+        json!({"type": "dataBar", "min": {"type": "formula", "value": "1+"}}),
+        // more garbage that the parser rejects
+        json!({"type": "expression", "formula": "=IF(A1", "preset": "lightRedFill"}),
+        json!({"type": "cellIs", "operator": "equal", "value": "=((A1", "preset": "lightRedFill"}),
+        // multiple leading '=' on a stored-as-typed threshold: the writer strips one,
+        // the rest must still parse (#1999 review)
+        json!({"type": "dataBar", "min": {"type": "formula", "value": "===A1"}}),
+        json!({"type": "dataBar", "max": {"type": "formula", "value": "==1+"}}),
+    ] {
+        let err = session()
+            .execute("home.conditionalFormat", json!({"range": "A1:A5", "rule": rule.clone()}))
+            .expect_err("invalid formula {rule} must be rejected");
+        let msg = err.to_string();
+        assert!(msg.to_lowercase().contains("formula"), "error should mention the formula: {msg}");
+    }
+}
+
+#[test]
+fn valid_cf_formulas_still_work() {
+    let mut s = session();
+    // bare comparison without '=' (the issue: this one saves fine today)
+    s.execute("home.conditionalFormat", json!({"range": "A1:A5", "rule": {"type": "expression", "formula": "A1>5", "preset": "lightRedFill"}}))
+        .unwrap();
+    // leading '=' is accepted and stripped
+    s.execute("home.conditionalFormat", json!({"range": "B1:B5", "rule": {"type": "expression", "formula": "=B1>5", "preset": "lightRedFill"}}))
+        .unwrap();
+    // cellIs numeric and text values are not formulas, must pass untouched
+    s.execute(
+        "home.conditionalFormat",
+        json!({"range": "C1:C5", "rule": {"type": "cellIs", "operator": "greaterThan", "value": "5", "preset": "lightRedFill"}}),
+    )
+    .unwrap();
+    s.execute(
+        "home.conditionalFormat",
+        json!({"range": "D1:D5", "rule": {"type": "cellIs", "operator": "equal", "value": "hello there", "preset": "lightRedFill"}}),
+    )
+    .unwrap();
+    // dataBar formula thresholds that parse
+    s.execute("home.conditionalFormat", json!({"range": "E1:E5", "rule": {"type": "dataBar", "min": {"type": "formula", "value": "MIN(E1:E5)"}, "max": {"type": "formula", "value": "MAX(E1:E5)"}}}))
+        .unwrap();
+    // non-formula rule types are untouched
+    s.execute("home.conditionalFormat", json!({"range": "F1:F5", "rule": {"type": "duplicate", "preset": "lightRedFill"}})).unwrap();
+}