From b045d0c5e95bb70ca602f734d13a49f6b1f57de7 Mon Sep 17 00:00:00 2001
From: "Dhanji R. Prasanna" <d@wideplay.com>
Date: Sat, 7 Feb 2026 13:24:41 +1100
Subject: [PATCH] fix: reject write_envelope with empty facts

The write_envelope tool was silently accepting YAML without a 'facts:'
top-level key. serde would ignore unknown fields and default the facts
HashMap to empty, causing the predicate pipeline to always see no facts.

Now validates that envelope.facts is non-empty after deserialization,
returning a clear error with an example of the correct format.

Adds 6 tests covering valid/invalid/boundary deserialization cases.
---
 crates/g3-core/src/tools/envelope.rs   | 16 +++++
 crates/g3-core/src/tools/invariants.rs | 83 ++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)

diff --git a/crates/g3-core/src/tools/envelope.rs b/crates/g3-core/src/tools/envelope.rs
index 5033923..a500ff1 100644
--- a/crates/g3-core/src/tools/envelope.rs
+++ b/crates/g3-core/src/tools/envelope.rs
@@ -57,6 +57,22 @@ pub async fn execute_write_envelope<W: UiWriter>(
         Err(e) => return Ok(format!("❌ Invalid envelope YAML: {}", e)),
     };
 
+    // Validate that facts is non-empty. This catches the common mistake where
+    // the agent sends a raw YAML map without the required `facts:` top-level key.
+    // serde silently ignores unknown fields and defaults `facts` to an empty HashMap,
+    // so we must check explicitly.
+    if envelope.facts.is_empty() {
+        return Ok(
+            "❌ Envelope has empty facts. The YAML must contain a non-empty `facts` top-level key. Example:\n\n\
+             ```yaml\n\
+             facts:\n\
+             \x20 my_feature:\n\
+             \x20   capabilities: [feature_a, feature_b]\n\
+             \x20   file: \"src/my_feature.rs\"\n\
+             ```".to_string()
+        );
+    }
+
     // Write the envelope to disk
     if let Err(e) = write_envelope(session_id, &envelope) {
         return Ok(format!("❌ Failed to write envelope: {}", e));
diff --git a/crates/g3-core/src/tools/invariants.rs b/crates/g3-core/src/tools/invariants.rs
index 107db03..c6de19a 100644
--- a/crates/g3-core/src/tools/invariants.rs
+++ b/crates/g3-core/src/tools/invariants.rs
@@ -1363,4 +1363,87 @@ mod tests {
         assert!(output.contains("**breaking_changes**:"));
         assert!(output.contains("_null_"));
     }
+
+    // ========================================================================
+    // ActionEnvelope Deserialization Validation Tests
+    // ========================================================================
+
+    #[test]
+    fn test_envelope_deser_with_facts_key() {
+        // Valid: YAML with facts: top-level key
+        let yaml = r#"
+facts:
+  csv_importer:
+    capabilities:
+      - handle_headers
+      - handle_tsv
+    file: src/import/csv.rs
+"#;
+        let envelope: ActionEnvelope = serde_yaml::from_str(yaml).unwrap();
+        assert!(!envelope.facts.is_empty());
+        assert!(envelope.facts.contains_key("csv_importer"));
+    }
+
+    #[test]
+    fn test_envelope_deser_without_facts_key_is_empty() {
+        // Bug scenario: YAML without facts: wrapper silently produces empty facts
+        let yaml = r#"
+csv_importer:
+  capabilities:
+    - handle_headers
+    - handle_tsv
+  file: src/import/csv.rs
+"#;
+        let envelope: ActionEnvelope = serde_yaml::from_str(yaml).unwrap();
+        // serde silently ignores unknown fields, facts defaults to empty
+        assert!(envelope.facts.is_empty(), "Expected empty facts when 'facts:' key is missing");
+    }
+
+    #[test]
+    fn test_envelope_deser_empty_facts_is_empty() {
+        let yaml = "facts: {}";
+        let envelope: ActionEnvelope = serde_yaml::from_str(yaml).unwrap();
+        assert!(envelope.facts.is_empty());
+    }
+
+    #[test]
+    fn test_envelope_deser_facts_with_null_values() {
+        let yaml = r#"
+facts:
+  breaking_changes: null
+  csv_importer:
+    file: src/import/csv.rs
+"#;
+        let envelope: ActionEnvelope = serde_yaml::from_str(yaml).unwrap();
+        assert_eq!(envelope.facts.len(), 2);
+        assert!(envelope.facts.contains_key("breaking_changes"));
+        assert_eq!(envelope.facts.get("breaking_changes").unwrap(), &YamlValue::Null);
+        assert!(envelope.facts.contains_key("csv_importer"));
+    }
+
+    #[test]
+    fn test_envelope_deser_facts_single_key() {
+        let yaml = r#"
+facts:
+  my_feature:
+    file: src/my_feature.rs
+"#;
+        let envelope: ActionEnvelope = serde_yaml::from_str(yaml).unwrap();
+        assert_eq!(envelope.facts.len(), 1);
+        assert!(envelope.facts.contains_key("my_feature"));
+    }
+
+    #[test]
+    fn test_envelope_roundtrip_preserves_null_facts() {
+        let mut envelope = ActionEnvelope::new();
+        envelope.add_fact("breaking_changes", YamlValue::Null);
+        envelope.add_fact("feature", YamlValue::String("done".to_string()));
+
+        let yaml = serde_yaml::to_string(&envelope).unwrap();
+        let parsed: ActionEnvelope = serde_yaml::from_str(&yaml).unwrap();
+
+        assert_eq!(parsed.facts.len(), 2);
+        assert_eq!(parsed.facts.get("breaking_changes").unwrap(), &YamlValue::Null);
+        assert_eq!(parsed.facts.get("feature").unwrap(), &YamlValue::String("done".to_string()));
+    }
 }