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.
This commit is contained in:
@@ -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));
|
||||
|
||||
@@ -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()));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user