diff --git a/cli/tests/ui/config_schema_additional_properties_schema.json5 b/cli/tests/ui/config_schema_additional_properties_schema.json5 new file mode 100644 index 00000000..ae6240b1 --- /dev/null +++ b/cli/tests/ui/config_schema_additional_properties_schema.json5 @@ -0,0 +1,10 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + properties: { + token: { type: "string" }, + }, + additionalProperties: { type: "string" }, + }, +} diff --git a/cli/tests/ui/config_schema_additional_properties_schema.stderr b/cli/tests/ui/config_schema_additional_properties_schema.stderr new file mode 100644 index 00000000..f0cf9ac9 --- /dev/null +++ b/cli/tests/ui/config_schema_additional_properties_schema.stderr @@ -0,0 +1,12 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at additionalProperties: + `additionalProperties` must be a boolean in Amber config_schema + ╭─[/tests/ui/config_schema_additional_properties_schema.json5:8:27] + 7 │ }, + 8 │ additionalProperties: { type: "string" }, + · ─────────┬──────── + · ╰── invalid config definition here + 9 │ }, + ╰──── diff --git a/cli/tests/ui/config_schema_invalid_property_name.json5 b/cli/tests/ui/config_schema_invalid_property_name.json5 new file mode 100644 index 00000000..4a81ed79 --- /dev/null +++ b/cli/tests/ui/config_schema_invalid_property_name.json5 @@ -0,0 +1,9 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + properties: { + agent_HF_TOKEN: { type: "string", secret: true }, + }, + }, +} diff --git a/cli/tests/ui/config_schema_invalid_property_name.stderr b/cli/tests/ui/config_schema_invalid_property_name.stderr new file mode 100644 index 00000000..69d4a56d --- /dev/null +++ b/cli/tests/ui/config_schema_invalid_property_name.stderr @@ -0,0 +1,15 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at properties.agent_HF_TOKEN: config property + name `agent_HF_TOKEN` is invalid; config property names must start with + a lowercase ASCII letter and contain only lowercase ASCII letters, + digits, and underscores; double underscores are not allowed. Uppercase + letters are not allowed; use `agent_hf_token` instead + ╭─[/tests/ui/config_schema_invalid_property_name.json5:6:7] + 5 │ properties: { + 6 │ agent_HF_TOKEN: { type: "string", secret: true }, + · ───────┬────── + · ╰── invalid config definition here + 7 │ }, + ╰──── diff --git a/cli/tests/ui/config_schema_invalid_required_name.json5 b/cli/tests/ui/config_schema_invalid_required_name.json5 new file mode 100644 index 00000000..cfcc4d90 --- /dev/null +++ b/cli/tests/ui/config_schema_invalid_required_name.json5 @@ -0,0 +1,10 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + properties: { + token: { type: "string" }, + }, + required: ["HF_TOKEN"], + }, +} diff --git a/cli/tests/ui/config_schema_invalid_required_name.stderr b/cli/tests/ui/config_schema_invalid_required_name.stderr new file mode 100644 index 00000000..e3ec99bc --- /dev/null +++ b/cli/tests/ui/config_schema_invalid_required_name.stderr @@ -0,0 +1,15 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at required[0]: required config property name + `HF_TOKEN` is invalid; config property names must start with a lowercase + ASCII letter and contain only lowercase ASCII letters, digits, and + underscores; double underscores are not allowed. Uppercase letters are + not allowed; use `hf_token` instead + ╭─[/tests/ui/config_schema_invalid_required_name.json5:8:16] + 7 │ }, + 8 │ required: ["HF_TOKEN"], + · ─────┬──── + · ╰── invalid config definition here + 9 │ }, + ╰──── diff --git a/cli/tests/ui/config_schema_invalid_type_keyword.json5 b/cli/tests/ui/config_schema_invalid_type_keyword.json5 new file mode 100644 index 00000000..56cd0b78 --- /dev/null +++ b/cli/tests/ui/config_schema_invalid_type_keyword.json5 @@ -0,0 +1,11 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + properties: { + token: { + type: 42, + }, + }, + }, +} diff --git a/cli/tests/ui/config_schema_invalid_type_keyword.stderr b/cli/tests/ui/config_schema_invalid_type_keyword.stderr new file mode 100644 index 00000000..821521da --- /dev/null +++ b/cli/tests/ui/config_schema_invalid_type_keyword.stderr @@ -0,0 +1,12 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at properties.token.type: 42 is not valid + under any of the schemas listed in the 'anyOf' keyword + ╭─[/tests/ui/config_schema_invalid_type_keyword.json5:7:15] + 6 │ token: { + 7 │ type: 42, + · ─┬ + · ╰── invalid config definition here + 8 │ }, + ╰──── diff --git a/cli/tests/ui/config_schema_non_local_ref.json5 b/cli/tests/ui/config_schema_non_local_ref.json5 new file mode 100644 index 00000000..b7986ac8 --- /dev/null +++ b/cli/tests/ui/config_schema_non_local_ref.json5 @@ -0,0 +1,12 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + $defs: { + token: { $anchor: "token", type: "string" }, + }, + properties: { + token: { $ref: "#token" }, + }, + }, +} diff --git a/cli/tests/ui/config_schema_non_local_ref.stderr b/cli/tests/ui/config_schema_non_local_ref.stderr new file mode 100644 index 00000000..f2ed4a3e --- /dev/null +++ b/cli/tests/ui/config_schema_non_local_ref.stderr @@ -0,0 +1,12 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at properties.token.$ref: `$ref` must be local + to the same config_schema (`#` or `#/...`) + ╭─[/tests/ui/config_schema_non_local_ref.json5:9:22] + 8 │ properties: { + 9 │ token: { $ref: "#token" }, + · ────┬─── + · ╰── invalid config definition here + 10 │ }, + ╰──── diff --git a/cli/tests/ui/config_schema_properties_wrong_type.json5 b/cli/tests/ui/config_schema_properties_wrong_type.json5 new file mode 100644 index 00000000..feffc5cb --- /dev/null +++ b/cli/tests/ui/config_schema_properties_wrong_type.json5 @@ -0,0 +1,7 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + properties: [], + }, +} diff --git a/cli/tests/ui/config_schema_properties_wrong_type.stderr b/cli/tests/ui/config_schema_properties_wrong_type.stderr new file mode 100644 index 00000000..de036595 --- /dev/null +++ b/cli/tests/ui/config_schema_properties_wrong_type.stderr @@ -0,0 +1,12 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at properties: `properties` must be an object + in Amber config_schema, got array + ╭─[/tests/ui/config_schema_properties_wrong_type.json5:5:17] + 4 │ type: "object", + 5 │ properties: [], + · ─┬ + · ╰── invalid config definition here + 6 │ }, + ╰──── diff --git a/cli/tests/ui/config_schema_unknown_keyword.json5 b/cli/tests/ui/config_schema_unknown_keyword.json5 new file mode 100644 index 00000000..3f3276fc --- /dev/null +++ b/cli/tests/ui/config_schema_unknown_keyword.json5 @@ -0,0 +1,7 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + uiLabel: "Root config", + }, +} diff --git a/cli/tests/ui/config_schema_unknown_keyword.status b/cli/tests/ui/config_schema_unknown_keyword.status new file mode 100644 index 00000000..2e9ba477 --- /dev/null +++ b/cli/tests/ui/config_schema_unknown_keyword.status @@ -0,0 +1 @@ +success diff --git a/cli/tests/ui/config_schema_unsupported_keyword.json5 b/cli/tests/ui/config_schema_unsupported_keyword.json5 new file mode 100644 index 00000000..b6418a99 --- /dev/null +++ b/cli/tests/ui/config_schema_unsupported_keyword.json5 @@ -0,0 +1,9 @@ +{ + manifest_version: "0.1.0", + config_schema: { + type: "object", + patternProperties: { + ".*": { type: "string" }, + }, + }, +} diff --git a/cli/tests/ui/config_schema_unsupported_keyword.stderr b/cli/tests/ui/config_schema_unsupported_keyword.stderr new file mode 100644 index 00000000..8236a70d --- /dev/null +++ b/cli/tests/ui/config_schema_unsupported_keyword.stderr @@ -0,0 +1,12 @@ +Error: manifest::invalid_config_schema + + × check failed + ╰─▶ invalid config definition at patternProperties: unsupported + config_schema keyword `patternProperties` + ╭─[/tests/ui/config_schema_unsupported_keyword.json5:5:5] + 4 │ type: "object", + 5 │ patternProperties: { + · ────────┬──────── + · ╰── invalid config definition here + 6 │ ".*": { type: "string" }, + ╰──── diff --git a/compiler/config/src/lib.rs b/compiler/config/src/lib.rs index 8acff070..c0b0b3f6 100644 --- a/compiler/config/src/lib.rs +++ b/compiler/config/src/lib.rs @@ -1,6 +1,7 @@ mod env; mod error; mod node; +mod profile; mod schema; mod template; @@ -10,6 +11,7 @@ pub use env::{ }; pub use error::{ConfigError, Result}; pub use node::{ConfigNode, RootConfigTemplate, compose_config_template}; +pub use profile::{ConfigSchemaProfileError, validate_config_schema_profile}; pub use schema::{ SchemaLeaf, SchemaLookup, SchemaPresence, SchemaWalkResult, apply_schema_defaults, apply_schema_defaults_to_node, canonical_json, collect_leaf_paths, collect_schema_leaves, diff --git a/compiler/config/src/profile.rs b/compiler/config/src/profile.rs new file mode 100644 index 00000000..9e7c8487 --- /dev/null +++ b/compiler/config/src/profile.rs @@ -0,0 +1,600 @@ +use serde_json::Value; + +use crate::is_valid_config_key; + +const CONFIG_KEY_RULE: &str = "must start with a lowercase ASCII letter and contain only \ + lowercase ASCII letters, digits, and underscores; double \ + underscores are not allowed"; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ConfigSchemaProfileError { + pub path: String, + pub message: String, + pub pointer: Option, + pub key: Option, +} + +impl ConfigSchemaProfileError { + pub fn from_json_schema_validation(error: &jsonschema::ValidationError<'_>) -> Self { + let pointer = error.instance_path().as_str().to_string(); + Self { + path: display_path(path_from_json_pointer(&pointer).as_str()), + message: error.to_string(), + pointer: Some(pointer), + key: None, + } + } + + fn at_value(pointer: impl Into, path: impl Into, message: String) -> Self { + Self { + path: display_path(path.into().as_str()), + message, + pointer: Some(pointer.into()), + key: None, + } + } + + fn at_key( + object_pointer: impl Into, + object_path: impl Into, + key: &str, + message: String, + ) -> Self { + let object_path = object_path.into(); + Self { + path: display_path(path_child(&object_path, key).as_str()), + message, + pointer: Some(object_pointer.into()), + key: Some(key.to_string()), + } + } + + fn at_root(message: String) -> Self { + Self { + path: "".to_string(), + message, + pointer: Some(String::new()), + key: None, + } + } +} + +fn display_path(path: &str) -> String { + if path.is_empty() { + "".to_string() + } else { + path.to_string() + } +} + +fn path_child(path: &str, segment: &str) -> String { + if path.is_empty() { + segment.to_string() + } else { + format!("{path}.{segment}") + } +} + +fn path_index(path: &str, index: usize) -> String { + if path.is_empty() { + format!("[{index}]") + } else { + format!("{path}[{index}]") + } +} + +fn pointer_child(pointer: &str, segment: &str) -> String { + let escaped = escape_json_pointer_segment(segment); + if pointer.is_empty() { + format!("/{escaped}") + } else { + format!("{pointer}/{escaped}") + } +} + +fn pointer_index(pointer: &str, index: usize) -> String { + if pointer.is_empty() { + format!("/{index}") + } else { + format!("{pointer}/{index}") + } +} + +fn escape_json_pointer_segment(segment: &str) -> String { + segment.replace('~', "~0").replace('/', "~1") +} + +fn unescape_json_pointer_segment(segment: &str) -> String { + segment.replace("~1", "/").replace("~0", "~") +} + +fn path_from_json_pointer(pointer: &str) -> String { + let mut path = String::new(); + for segment in pointer.strip_prefix('/').unwrap_or(pointer).split('/') { + if segment.is_empty() { + continue; + } + let segment = unescape_json_pointer_segment(segment); + path = match segment.parse::() { + Ok(index) => path_index(&path, index), + Err(_) => path_child(&path, &segment), + }; + } + path +} + +fn type_includes_object(schema: &Value) -> bool { + match schema.get("type") { + Some(Value::String(ty)) => ty == "object", + Some(Value::Array(types)) => types.iter().any(|ty| ty.as_str() == Some("object")), + _ => false, + } +} + +fn invalid_config_key_message(kind: &str, key: &str) -> String { + let mut message = format!("{kind} `{key}` is invalid; config property names {CONFIG_KEY_RULE}"); + if key.chars().any(char::is_uppercase) { + message.push_str(". Uppercase letters are not allowed"); + } + let lower = key.to_ascii_lowercase(); + if lower != key && is_valid_config_key(&lower) { + message.push_str("; use `"); + message.push_str(&lower); + message.push_str("` instead"); + } + message +} + +fn is_draft_2020_12_schema(uri: &str) -> bool { + matches!( + uri, + "https://json-schema.org/draft/2020-12/schema" + | "https://json-schema.org/draft/2020-12/schema#" + | "http://json-schema.org/draft/2020-12/schema" + | "http://json-schema.org/draft/2020-12/schema#" + ) +} + +fn check_object_type_for_keyword( + schema: &Value, + pointer: &str, + path: &str, + keyword: &str, +) -> Result<(), ConfigSchemaProfileError> { + if type_includes_object(schema) { + return Ok(()); + } + + let message = format!( + "schema objects with `{keyword}` must declare `type: \"object\"` or include \"object\" in \ + their `type` array" + ); + if schema.get("type").is_some() { + Err(ConfigSchemaProfileError::at_value( + pointer_child(pointer, "type"), + path_child(path, "type"), + message, + )) + } else { + Err(ConfigSchemaProfileError::at_value(pointer, path, message)) + } +} + +fn unsupported_keyword_message(keyword: &str) -> String { + format!("unsupported config_schema keyword `{keyword}`") +} + +fn value_kind(value: &Value) -> &'static str { + match value { + Value::Null => "null", + Value::Bool(_) => "boolean", + Value::Number(_) => "number", + Value::String(_) => "string", + Value::Array(_) => "array", + Value::Object(_) => "object", + } +} + +fn keyword_type_error( + pointer: &str, + path: &str, + keyword: &str, + expected: &str, + value: &Value, +) -> ConfigSchemaProfileError { + ConfigSchemaProfileError::at_value( + pointer_child(pointer, keyword), + path_child(path, keyword), + format!( + "`{keyword}` must be {expected} in Amber config_schema, got {}", + value_kind(value) + ), + ) +} + +pub fn validate_config_schema_profile(schema: &Value) -> Result<(), ConfigSchemaProfileError> { + if !schema.is_object() { + return Err(ConfigSchemaProfileError::at_root( + "root config_schema must be a JSON Schema object with `type: \"object\"`".to_string(), + )); + } + if !type_includes_object(schema) { + return Err(ConfigSchemaProfileError::at_value( + schema + .get("type") + .map_or_else(String::new, |_| pointer_child("", "type")), + schema + .get("type") + .map_or_else(String::new, |_| path_child("", "type")), + "root config_schema must declare `type: \"object\"` or include \"object\" in its \ + `type` array" + .to_string(), + )); + } + walk_schema(schema, "", "") +} + +fn walk_schema(schema: &Value, pointer: &str, path: &str) -> Result<(), ConfigSchemaProfileError> { + let Some(obj) = schema.as_object() else { + return Ok(()); + }; + + if let Some(schema_uri) = obj.get("$schema") { + let Some(schema_uri) = schema_uri.as_str() else { + return Err(keyword_type_error( + pointer, path, "$schema", "a string", schema_uri, + )); + }; + if !is_draft_2020_12_schema(schema_uri) { + return Err(ConfigSchemaProfileError::at_value( + pointer_child(pointer, "$schema"), + path_child(path, "$schema"), + "`$schema` must be Draft 2020-12 for Amber config_schema".to_string(), + )); + } + } + + if let Some(reference) = obj.get("$ref") { + let Some(reference) = reference.as_str() else { + return Err(keyword_type_error( + pointer, path, "$ref", "a string", reference, + )); + }; + if reference != "#" && !reference.starts_with("#/") { + return Err(ConfigSchemaProfileError::at_value( + pointer_child(pointer, "$ref"), + path_child(path, "$ref"), + "`$ref` must be local to the same config_schema (`#` or `#/...`)".to_string(), + )); + } + } + + for keyword in [ + "anyOf", + "oneOf", + "not", + "if", + "then", + "else", + "patternProperties", + "propertyNames", + "dependentSchemas", + "dependentRequired", + "unevaluatedProperties", + "unevaluatedItems", + "$dynamicRef", + "$recursiveRef", + ] { + if obj.contains_key(keyword) { + return Err(ConfigSchemaProfileError::at_key( + pointer, + path, + keyword, + unsupported_keyword_message(keyword), + )); + } + } + + if let Some(secret) = obj.get("secret") + && !secret.is_boolean() + { + return Err(ConfigSchemaProfileError::at_value( + pointer_child(pointer, "secret"), + path_child(path, "secret"), + "`secret` must be a boolean annotation".to_string(), + )); + } + + if let Some(additional) = obj.get("additionalProperties") + && !additional.is_boolean() + { + return Err(ConfigSchemaProfileError::at_value( + pointer_child(pointer, "additionalProperties"), + path_child(path, "additionalProperties"), + "`additionalProperties` must be a boolean in Amber config_schema".to_string(), + )); + } + + if let Some(properties) = obj.get("properties") { + check_object_type_for_keyword(schema, pointer, path, "properties")?; + let Some(properties) = properties.as_object() else { + return Err(keyword_type_error( + pointer, + path, + "properties", + "an object", + properties, + )); + }; + let properties_pointer = pointer_child(pointer, "properties"); + let properties_path = path_child(path, "properties"); + for (key, child_schema) in properties { + if !is_valid_config_key(key) { + return Err(ConfigSchemaProfileError::at_key( + &properties_pointer, + &properties_path, + key, + invalid_config_key_message("config property name", key), + )); + } + walk_schema( + child_schema, + &pointer_child(&properties_pointer, key), + &path_child(&properties_path, key), + )?; + } + } + + if let Some(required) = obj.get("required") { + check_object_type_for_keyword(schema, pointer, path, "required")?; + let Some(required) = required.as_array() else { + return Err(keyword_type_error( + pointer, path, "required", "an array", required, + )); + }; + for (idx, required_key) in required.iter().enumerate() { + let Some(required_key) = required_key.as_str() else { + return Err(ConfigSchemaProfileError::at_value( + pointer_index(&pointer_child(pointer, "required"), idx), + path_index(&path_child(path, "required"), idx), + format!( + "`required` entries must be strings in Amber config_schema, got {}", + value_kind(required_key) + ), + )); + }; + if !is_valid_config_key(required_key) { + return Err(ConfigSchemaProfileError::at_value( + pointer_index(&pointer_child(pointer, "required"), idx), + path_index(&path_child(path, "required"), idx), + invalid_config_key_message("required config property name", required_key), + )); + } + } + } + + for keyword in ["$defs", "definitions"] { + let Some(defs) = obj.get(keyword) else { + continue; + }; + let Some(defs) = defs.as_object() else { + return Err(keyword_type_error( + pointer, + path, + keyword, + "an object", + defs, + )); + }; + let defs_pointer = pointer_child(pointer, keyword); + let defs_path = path_child(path, keyword); + for (name, child_schema) in defs { + walk_schema( + child_schema, + &pointer_child(&defs_pointer, name), + &path_child(&defs_path, name), + )?; + } + } + + if let Some(all_of) = obj.get("allOf") { + let Some(all_of) = all_of.as_array() else { + return Err(keyword_type_error( + pointer, path, "allOf", "an array", all_of, + )); + }; + let all_of_pointer = pointer_child(pointer, "allOf"); + let all_of_path = path_child(path, "allOf"); + for (idx, child_schema) in all_of.iter().enumerate() { + walk_schema( + child_schema, + &pointer_index(&all_of_pointer, idx), + &path_index(&all_of_path, idx), + )?; + } + } + + if let Some(items) = obj.get("items") { + walk_schema( + items, + &pointer_child(pointer, "items"), + &path_child(path, "items"), + )?; + } + + if let Some(contains) = obj.get("contains") { + walk_schema( + contains, + &pointer_child(pointer, "contains"), + &path_child(path, "contains"), + )?; + } + + if let Some(prefix_items) = obj.get("prefixItems") { + let Some(prefix_items) = prefix_items.as_array() else { + return Err(keyword_type_error( + pointer, + path, + "prefixItems", + "an array", + prefix_items, + )); + }; + let prefix_pointer = pointer_child(pointer, "prefixItems"); + let prefix_path = path_child(path, "prefixItems"); + for (idx, child_schema) in prefix_items.iter().enumerate() { + walk_schema( + child_schema, + &pointer_index(&prefix_pointer, idx), + &path_index(&prefix_path, idx), + )?; + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + fn profile_error(schema: Value) -> ConfigSchemaProfileError { + validate_config_schema_profile(&schema).unwrap_err() + } + + #[test] + fn rejects_pattern_properties() { + let err = profile_error(json!({ + "type": "object", + "patternProperties": { + ".*": { "type": "string" } + } + })); + + assert_eq!(err.path, "patternProperties"); + assert_eq!( + err.message, + "unsupported config_schema keyword `patternProperties`" + ); + assert_eq!(err.pointer.as_deref(), Some("")); + assert_eq!(err.key.as_deref(), Some("patternProperties")); + } + + #[test] + fn rejects_uppercase_property_name() { + let err = profile_error(json!({ + "type": "object", + "properties": { + "agent_HF_TOKEN": { "type": "string", "secret": true } + } + })); + + assert_eq!(err.path, "properties.agent_HF_TOKEN"); + assert!(err.message.contains("Uppercase letters are not allowed")); + assert!(err.message.contains("use `agent_hf_token` instead")); + assert_eq!(err.pointer.as_deref(), Some("/properties")); + assert_eq!(err.key.as_deref(), Some("agent_HF_TOKEN")); + } + + #[test] + fn rejects_uppercase_required_name() { + let err = profile_error(json!({ + "type": "object", + "properties": { + "token": { "type": "string" } + }, + "required": ["HF_TOKEN"] + })); + + assert_eq!(err.path, "required[0]"); + assert!(err.message.contains("required config property name")); + assert!(err.message.contains("Uppercase letters are not allowed")); + assert_eq!(err.pointer.as_deref(), Some("/required/0")); + assert_eq!(err.key, None); + } + + #[test] + fn accepts_secret_and_x_annotations() { + validate_config_schema_profile(&json!({ + "type": "object", + "properties": { + "token": { "type": "string", "secret": true }, + "model": { "type": "string", "x-example-hide": true } + } + })) + .expect("supported profile should pass"); + } + + #[test] + fn rejects_non_2020_12_draft() { + let err = profile_error(json!({ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "x": { "type": "string" } + } + })); + + assert_eq!(err.path, "$schema"); + assert_eq!( + err.message, + "`$schema` must be Draft 2020-12 for Amber config_schema" + ); + assert_eq!(err.pointer.as_deref(), Some("/$schema")); + } + + #[test] + fn rejects_non_pointer_ref() { + let err = profile_error(json!({ + "type": "object", + "$defs": { + "x": { "$anchor": "x", "type": "string" } + }, + "properties": { + "prop": { "$ref": "#x" } + } + })); + + assert_eq!(err.path, "properties.prop.$ref"); + assert_eq!( + err.message, + "`$ref` must be local to the same config_schema (`#` or `#/...`)" + ); + assert_eq!(err.pointer.as_deref(), Some("/properties/prop/$ref")); + } + + #[test] + fn accepts_custom_annotation_keywords() { + validate_config_schema_profile(&json!({ + "type": "object", + "uiLabel": "Root config", + "properties": { + "token": { + "type": "string", + "uiLabel": "Token" + } + } + })) + .expect("custom annotation keywords should pass through the profile"); + } + + #[test] + fn maps_json_schema_validation_error_to_config_schema_path() { + let schema = json!({ + "type": "object", + "properties": { + "token": { + "type": 42 + } + } + }); + let schema_error = jsonschema::validator_for(&schema).unwrap_err(); + let err = ConfigSchemaProfileError::from_json_schema_validation(&schema_error); + + assert_eq!(err.path, "properties.token.type"); + assert_eq!(err.pointer.as_deref(), Some("/properties/token/type")); + assert!(err.message.contains("42")); + } +} diff --git a/compiler/manifest/src/config_schema_profile.rs b/compiler/manifest/src/config_schema_profile.rs deleted file mode 100644 index d84c5f98..00000000 --- a/compiler/manifest/src/config_schema_profile.rs +++ /dev/null @@ -1,283 +0,0 @@ -use std::sync::OnceLock; - -use jsonschema::Validator; -use serde_json::Value; - -const PROFILE_ID: &str = "https://amber-protocol.org/json-schema/config-schema-profile"; - -fn profile_meta_schema() -> Value { - // This is a JSON Schema meta-schema (Draft 2020-12) used to validate the `config_schema` - // objects that Amber accepts in manifests. - // - // It intentionally restricts authors to a deterministic subset that Amber tooling supports - // (static config ref validation, leaf enumeration, env-var mapping, etc.). - // - // Note: Avoid internal `$ref`s to `#/$defs/...` here. The `jsonschema` crate's reference - // resolution can treat in-document refs as relative to a referenced meta-schema in some - // cases, which makes `#/$defs/...` unexpectedly point at the wrong document. - let key_name = serde_json::json!({ - "type": "string", - "pattern": "^(?!.*__)[a-z][a-z0-9_]*$", - }); - let type_includes_object = serde_json::json!({ - "anyOf": [ - { "const": "object" }, - { - "type": "array", - "contains": { "const": "object" }, - }, - ], - }); - - let meta_schema = serde_json::json!({ - "$dynamicAnchor": "meta", - "anyOf": [ - { "type": "boolean" }, - { - "$ref": "https://json-schema.org/draft/2020-12/schema", - "unevaluatedProperties": false, - "properties": { - "$schema": { - "type": "string", - "pattern": "^https?://json-schema.org/draft/2020-12/schema#?$", - }, - "$ref": { - "type": "string", - "pattern": "^#($|/.*)$", - }, - "properties": { - "type": "object", - "propertyNames": key_name.clone(), - }, - "required": { - "type": "array", - "items": key_name.clone(), - }, - "additionalProperties": { "type": "boolean" }, - "secret": { "type": "boolean" }, - - "anyOf": false, - "oneOf": false, - "not": false, - "if": false, - "then": false, - "else": false, - "patternProperties": false, - "propertyNames": false, - "dependentSchemas": false, - "dependentRequired": false, - "unevaluatedProperties": false, - "unevaluatedItems": false, - "$dynamicRef": false, - "$recursiveRef": false, - }, - "patternProperties": { - "^x-": true, - }, - "allOf": [ - { - "if": { "required": ["properties"] }, - "then": { - "required": ["type"], - "properties": { "type": type_includes_object.clone() }, - }, - }, - { - "if": { "required": ["required"] }, - "then": { - "required": ["type"], - "properties": { "type": type_includes_object.clone() }, - }, - }, - ], - }, - ], - }); - - let root_schema = serde_json::json!({ - "type": "object", - "required": ["type"], - "properties": { - "type": type_includes_object, - }, - }); - - serde_json::json!({ - "$schema": "https://json-schema.org/draft/2020-12/schema", - "$id": PROFILE_ID, - "title": "Amber config_schema profile", - "description": "Restricts JSON Schema to the subset supported by Amber for `config_schema`.", - "allOf": [meta_schema, root_schema], - }) -} - -fn profile_validator() -> Result<&'static Validator, String> { - static VALIDATOR: OnceLock> = OnceLock::new(); - let validator = VALIDATOR.get_or_init(|| { - jsonschema::validator_for(&profile_meta_schema()).map_err(|err| { - format!("internal error: failed to build Amber config_schema profile validator: {err}") - }) - }); - - match validator { - Ok(validator) => Ok(validator), - Err(err) => Err(err.clone()), - } -} - -pub fn validate(schema: &Value) -> Result<(), String> { - let v = profile_validator()?; - - let mut errs = v.iter_errors(schema); - let Some(first) = errs.next() else { - return Ok(()); - }; - - let at = first.instance_path().to_string(); - let mut msgs = vec![first.to_string()]; - msgs.extend(errs.take(7).map(|e| e.to_string())); - - let at = if at.is_empty() { "" } else { at.as_str() }; - Err(format!( - "does not match Amber config_schema profile at {at}: {}", - msgs.join("; ") - )) -} - -#[cfg(test)] -mod tests { - use crate::{Error, Manifest}; - - #[test] - fn config_schema_profile_rejects_pattern_properties() { - let input = r#" - { - manifest_version: "0.1.0", - config_schema: { - type: "object", - patternProperties: { - ".*": { type: "string" }, - }, - }, - } - "#; - let err = input.parse::().unwrap_err(); - match err { - Error::Json5Path(diag) => { - assert!( - diag.to_string() - .contains("does not match Amber config_schema profile") - ); - } - other => panic!("expected Json5Path error, got: {other}"), - } - } - - #[test] - fn config_schema_profile_accepts_secret_annotation() { - let input = r#" - { - manifest_version: "0.1.0", - config_schema: { - type: "object", - properties: { - token: { type: "string", secret: true }, - group: { - type: "object", - secret: true, - properties: { - url: { type: "string" }, - }, - }, - }, - }, - } - "#; - let _ = input - .parse::() - .expect("schema with secret annotations should parse"); - } - - #[test] - fn config_schema_profile_accepts_x_annotations() { - let input = r#" - { - manifest_version: "0.1.0", - config_schema: { - type: "object", - properties: { - my_config: { - type: "string", - "x-example-hide": true, - }, - nested: { - type: "object", - "x-example-section": "advanced", - properties: { - enabled: { - type: "boolean", - "x-example-label": "Enable advanced mode", - }, - }, - }, - }, - }, - } - "#; - let _ = input - .parse::() - .expect("schema with x-* annotations should parse"); - } - - #[test] - fn config_schema_profile_rejects_non_2020_12_draft() { - let input = r#" - { - manifest_version: "0.1.0", - config_schema: { - $schema: "http://json-schema.org/draft-07/schema#", - type: "object", - properties: { x: { type: "string" } }, - }, - } - "#; - let err = input.parse::().unwrap_err(); - match err { - Error::Json5Path(diag) => { - assert!( - diag.to_string() - .contains("does not match Amber config_schema profile") - ); - } - other => panic!("expected Json5Path error, got: {other}"), - } - } - - #[test] - fn config_schema_profile_rejects_non_pointer_ref() { - let input = r##" - { - manifest_version: "0.1.0", - config_schema: { - type: "object", - $defs: { - x: { $anchor: "x", type: "string" }, - }, - properties: { - prop: { $ref: "#x" }, - }, - }, - } - "##; - let err = input.parse::().unwrap_err(); - match err { - Error::Json5Path(diag) => { - assert!( - diag.to_string() - .contains("does not match Amber config_schema profile") - ); - } - other => panic!("expected Json5Path error, got: {other}"), - } - } -} diff --git a/compiler/manifest/src/document.rs b/compiler/manifest/src/document.rs index 2bd1d70b..181cd7e0 100644 --- a/compiler/manifest/src/document.rs +++ b/compiler/manifest/src/document.rs @@ -2,6 +2,7 @@ use std::{path::PathBuf, sync::Arc}; +use amber_json5::spans::span_for_object_key; use miette::{Diagnostic, LabeledSpan, NamedSource, SourceCode, SourceSpan}; use thiserror::Error; use url::Url; @@ -255,10 +256,9 @@ fn labels_for_manifest_error( ManifestError::UnsupportedMountSource { mount } => { labels_for_mount_source(spans, mount, "reserved mount source") } - ManifestError::InvalidConfigSchema(_) => vec![primary( - span_or_default(spans.config_schema), - Some("invalid config definition here".to_string()), - )], + ManifestError::InvalidConfigSchema { pointer, key, .. } => { + labels_for_config_schema_error(source, spans, pointer.as_deref(), key.as_deref()) + } ManifestError::UnknownEnvironmentExtends { name, .. } => { labels_for_unknown_environment_extends(spans, name) } @@ -440,6 +440,45 @@ fn labels_for_mount_path(spans: &ManifestSpans, path: &str, label: &str) -> Vec< ) } +fn labels_for_config_schema_error( + source: &str, + spans: &ManifestSpans, + pointer: Option<&str>, + key: Option<&str>, +) -> Vec { + let Some(schema_span) = spans.config_schema else { + return vec![primary( + span_or_default(None), + Some("invalid config definition here".to_string()), + )]; + }; + + let span = match (pointer, key) { + (Some(pointer), Some(key)) => crate::span_for_json_pointer(source, schema_span, pointer) + .and_then(|object| span_for_object_key(source, object, key)) + .or_else(|| { + crate::span_for_json_pointer(source, schema_span, &json_pointer_child(pointer, key)) + }), + (Some(pointer), None) => crate::span_for_json_pointer(source, schema_span, pointer), + (None, _) => None, + } + .unwrap_or(schema_span); + + vec![primary( + span, + Some("invalid config definition here".to_string()), + )] +} + +fn json_pointer_child(pointer: &str, key: &str) -> String { + let key = key.replace('~', "~0").replace('/', "~1"); + if pointer.is_empty() { + format!("/{key}") + } else { + format!("{pointer}/{key}") + } +} + fn labels_for_mount_source(spans: &ManifestSpans, source: &str, label: &str) -> Vec { labels_for_mount( spans, @@ -932,6 +971,32 @@ mod tests { assert_eq!(label.len(), "\"#b\"".len()); } + #[test] + fn invalid_config_schema_property_name_points_to_key() { + let source = r#" + { + manifest_version: "0.1.0", + config_schema: { + type: "object", + properties: { + agent_HF_TOKEN: { type: "string", secret: true }, + }, + }, + } + "#; + + let err = ParsedManifest::parse_named("test", Arc::from(source)).unwrap_err(); + assert!(matches!(err.kind, crate::Error::InvalidConfigSchema { .. })); + + let labels: Vec<_> = err.labels().unwrap().collect(); + assert_eq!(labels.len(), 1); + + let label = &labels[0]; + let offset = source.find("agent_HF_TOKEN").unwrap(); + assert_eq!(label.offset(), offset); + assert_eq!(label.len(), "agent_HF_TOKEN".len()); + } + #[test] fn binding_missing_capability_points_to_slot_value() { let source = r##" diff --git a/compiler/manifest/src/error.rs b/compiler/manifest/src/error.rs index 0678e509..5f8f9f41 100644 --- a/compiler/manifest/src/error.rs +++ b/compiler/manifest/src/error.rs @@ -266,9 +266,14 @@ pub enum Error { #[diagnostic(code(manifest::unsupported_mount_source))] UnsupportedMountSource { mount: String }, - #[error("invalid config definition: {0}")] + #[error("invalid config definition at {path}: {message}")] #[diagnostic(code(manifest::invalid_config_schema))] - InvalidConfigSchema(String), + InvalidConfigSchema { + path: String, + message: String, + pointer: Option, + key: Option, + }, #[error("unsupported manifest version `{version}` (supported: {supported_req})")] #[diagnostic(code(manifest::unsupported_version))] diff --git a/compiler/manifest/src/lib.rs b/compiler/manifest/src/lib.rs index 39b4aa92..d23e72cc 100644 --- a/compiler/manifest/src/lib.rs +++ b/compiler/manifest/src/lib.rs @@ -1,6 +1,5 @@ #![allow(unused_assignments)] -mod config_schema_profile; mod document; pub mod framework; pub mod lint; diff --git a/compiler/manifest/src/schema.rs b/compiler/manifest/src/schema.rs index f7e49ba7..ef3d2c6a 100644 --- a/compiler/manifest/src/schema.rs +++ b/compiler/manifest/src/schema.rs @@ -18,7 +18,6 @@ use serde_with::{ }; use crate::{ - config_schema_profile, error::Error, interpolation::{ InlineStringSpec, InterpolatedString, InterpolationSource, ProgramEntrypoint, @@ -825,8 +824,23 @@ pub struct ConfigSchema(pub Value); impl ConfigSchema { pub(crate) fn validate_value(value: &Value) -> Result<(), Error> { - jsonschema::validator_for(value).map_err(|e| Error::InvalidConfigSchema(e.to_string()))?; - config_schema_profile::validate(value).map_err(Error::InvalidConfigSchema)?; + amber_config::validate_config_schema_profile(value).map_err(|e| { + Error::InvalidConfigSchema { + path: e.path, + message: e.message, + pointer: e.pointer, + key: e.key, + } + })?; + jsonschema::validator_for(value).map_err(|e| { + let e = amber_config::ConfigSchemaProfileError::from_json_schema_validation(&e); + Error::InvalidConfigSchema { + path: e.path, + message: e.message, + pointer: e.pointer, + key: e.key, + } + })?; Ok(()) } @@ -850,7 +864,9 @@ impl<'de> Deserialize<'de> for ConfigSchema { D: Deserializer<'de>, { let value = Value::deserialize(deserializer)?; - ConfigSchema::new(value).map_err(serde::de::Error::custom) + // Raw manifests validate config_schema after deserialization so diagnostics can use + // manifest-level source spans instead of pointing at the whole field. + Ok(ConfigSchema(value)) } } diff --git a/compiler/manifest/tests/builder.rs b/compiler/manifest/tests/builder.rs index 29282d45..7382cd8b 100644 --- a/compiler/manifest/tests/builder.rs +++ b/compiler/manifest/tests/builder.rs @@ -78,7 +78,7 @@ fn manifest_builder_rejects_invalid_config_schema() { .build() .unwrap_err(); - assert!(matches!(err, Error::InvalidConfigSchema(_))); + assert!(matches!(err, Error::InvalidConfigSchema { .. })); } #[test]