diff --git a/crates/hydra-codegen/src/lib.rs b/crates/hydra-codegen/src/lib.rs index cd3aa42..c317c26 100644 --- a/crates/hydra-codegen/src/lib.rs +++ b/crates/hydra-codegen/src/lib.rs @@ -164,12 +164,7 @@ fn generate_cli(definition: &ApiDefinition) -> String { if index > 0 { out.push_str(", "); } - out.push_str(&rust_string_literal(¶meter.name)); - out.push_str(": args."); - out.push_str(¶meter.name); - if matches!(parameter.ty, hydra_core::ParameterType::String) || !parameter.required { - out.push_str(".clone()"); - } + push_parameters_json_entry(&mut out, parameter); } out.push_str("}),\n"); } @@ -184,20 +179,29 @@ fn generate_cli(definition: &ApiDefinition) -> String { out.push_str("Args {\n"); for parameter in &operation.parameters { push_doc_comment(&mut out, " ", ¶meter.description); - if parameter.location != ParameterLocation::Path { - out.push_str(" #[arg(long)]\n"); - } - out.push_str(" pub "); - out.push_str(¶meter.name); - out.push_str(": "); - if parameter.required { - out.push_str(parameter.ty.rust_type()); - } else { - out.push_str("Option<"); - out.push_str(parameter.ty.rust_type()); - out.push('>'); + emit_cli_field( + &mut out, + ¶meter.name, + parameter.ty.rust_type(), + parameter.location == ParameterLocation::Path, + parameter.required, + parameter.cli.as_ref(), + None, + ); + if let Some(cli) = ¶meter.cli { + for companion in &cli.companions { + push_doc_comment(&mut out, " ", &companion.description); + emit_cli_field( + &mut out, + &companion.field, + "String", + false, + false, + None, + Some(&companion.flag), + ); + } } - out.push_str(",\n"); } out.push_str("}\n\n"); } @@ -205,10 +209,99 @@ fn generate_cli(definition: &ApiDefinition) -> String { out } +/// Emit one clap field for a generated CLI args struct. +/// +/// Path-location parameters stay positional (no attribute), preserving +/// the pre-existing contract. Companion fields are always optional +/// repeatable strings with an explicitly declared flag name. Parameter +/// fields with a CLI representation override follow it: an explicit flag +/// name when declared, and `multiple` → `Option>` with +/// `ArgAction::Append`. When no flag is declared, clap derives the long +/// flag from the field name (`snake_case` → kebab-case). +fn emit_cli_field( + out: &mut String, + field: &str, + rust_type: &str, + positional: bool, + required: bool, + cli: Option<&hydra_core::CliOverride>, + companion_flag: Option<&str>, +) { + let mut field_type = if required && companion_flag.is_none() && !cli.is_some_and(|c| c.multiple) + { + rust_type.to_owned() + } else { + format!("Option<{rust_type}>") + }; + let attribute = if let Some(flag) = companion_flag { + field_type = "Option>".to_string(); + format!(" #[arg(long = \"{flag}\", action = clap::ArgAction::Append)]\n") + } else if let Some(cli) = cli { + if cli.multiple { + field_type = "Option>".to_string(); + format!( + " #[arg(long = \"{}\", action = clap::ArgAction::Append, required = {required})]\n", + cli.effective_flag(field) + ) + } else if let Some(flag) = &cli.flag { + // An explicitly declared flag name is emitted verbatim. + format!(" #[arg(long = \"{flag}\")]\n") + } else { + " #[arg(long)]\n".to_owned() + } + } else if positional { + String::new() + } else { + " #[arg(long)]\n".to_owned() + }; + out.push_str(&attribute); + out.push_str(" pub "); + out.push_str(field); + out.push_str(": "); + out.push_str(&field_type); + out.push_str(",\n"); +} + fn cli_operations(definition: &ApiDefinition) -> impl Iterator { definition.operations.iter().filter(|o| o.generates_cli()) } +/// Emit one `"wire_name": args.field` entry of a generated +/// `parameters_json()` match arm. +/// +/// CLI representation overrides transform the CLI input back into the +/// wire shape: repeatable flags become arrays (defaulting to empty), +/// companions ride alongside as sibling keys. +fn push_parameters_json_entry(out: &mut String, parameter: &Parameter) { + out.push_str(&rust_string_literal(¶meter.name)); + out.push_str(": "); + out.push_str("args."); + out.push_str(¶meter.name); + match ¶meter.cli { + Some(cli) => { + if cli.multiple { + // `required = true` multiple flags are enforced by clap at + // parse time; unwrap_or_default() covers the optional case. + out.push_str(".clone().unwrap_or_default()"); + } else { + out.push_str(".clone()"); + } + for companion in &cli.companions { + out.push_str(", "); + out.push_str(&rust_string_literal(&companion.field)); + out.push_str(": args."); + out.push_str(&companion.field); + out.push_str(".clone()"); + } + } + None => { + if matches!(parameter.ty, hydra_core::ParameterType::String) || !parameter.required { + out.push_str(".clone()"); + } + } + } +} + // ── HTTP surface ─────────────────────────────────────────────────────────── /// Which axum imports the generated HTTP module needs, derived from what @@ -577,13 +670,29 @@ fn generate_mcp(definition: &ApiDefinition) -> String { if parameter.required { required.push(parameter.name.clone()); } - properties.insert( - parameter.name.clone(), + let property = if parameter.ty == hydra_core::ParameterType::Json { + // Declared JSON Schema subtree, verbatim. The parameter + // description is merged in only when the subtree does + // not carry its own `description` — a schema-level + // description wins, preserving verbatim embedding as + // the primary contract. + let mut subtree = parameter + .schema + .clone() + .unwrap_or_else(|| json!({"type": "object"})); + if let Some(object) = subtree.as_object_mut() { + object + .entry("description".to_owned()) + .or_insert_with(|| json!(parameter.description)); + } + subtree + } else { json!({ "type": parameter.ty.json_schema_type(), "description": parameter.description, - }), - ); + }) + }; + properties.insert(parameter.name.clone(), property); } json!({ "name": operation.name, diff --git a/crates/hydra-codegen/tests/codegen.rs b/crates/hydra-codegen/tests/codegen.rs index d9a2c6f..0fba9c2 100644 --- a/crates/hydra-codegen/tests/codegen.rs +++ b/crates/hydra-codegen/tests/codegen.rs @@ -20,6 +20,8 @@ fn sample_definition() -> ApiDefinition { ty: hydra_core::ParameterType::U32, required: false, location: ParameterLocation::Query, + schema: None, + cli: None, }], delivery: Delivery::Unary, surfaces: None, @@ -238,6 +240,8 @@ fn raw_request_operation_with_path_parameter_extracts_typed_path() { ty: hydra_core::ParameterType::String, required: true, location: ParameterLocation::Path, + schema: None, + cli: None, }], delivery: Delivery::Unary, surfaces: Some(vec![hydra_core::Surface::Http]), @@ -309,6 +313,8 @@ fn rejects_raw_request_with_sse_or_body_params() { ty: hydra_core::ParameterType::String, required: true, location: ParameterLocation::Body, + schema: None, + cli: None, }); assert!(hydra_core::validate::validate_definition(&definition).is_err()); definition.operations[0].parameters.pop(); @@ -334,3 +340,322 @@ fn committed_example_artifacts_are_current() { panic!("{e}"); } } + +// ── json parameters + CLI representation overrides (COD-411) ─────────────── + +fn attachments_parameter() -> Parameter { + Parameter { + name: "attachments".into(), + description: "Attachments to send.".into(), + ty: hydra_core::ParameterType::Json, + required: false, + location: ParameterLocation::Body, + schema: Some(serde_json::json!({ + "type": "array", + "items": { + "type": "object", + "oneOf": [ + { + "type": "object", + "properties": { + "mime_type": {"type": "string"}, + "filename": {"type": "string"}, + "data_base64": {"type": "string"} + }, + "required": ["mime_type", "data_base64"], + "additionalProperties": false + }, + { + "type": "object", + "properties": { + "stored_id": {"type": "string"} + }, + "required": ["stored_id"], + "additionalProperties": false + } + ] + } + })), + cli: Some(hydra_core::CliOverride { + flag: Some("attach".into()), + multiple: true, + companions: vec![hydra_core::CliCompanion { + flag: "attach-mime".into(), + field: "attach_mime".into(), + description: "MIME type override for the corresponding --attach.".into(), + }], + }), + } +} + +fn definition_with_attachments() -> ApiDefinition { + let mut definition = sample_definition(); + definition.operations[0] + .parameters + .push(attachments_parameter()); + definition +} + +#[test] +fn json_parameter_cli_override_generates_repeatable_and_companion_flags() { + let artifacts = generate_all(&definition_with_attachments(), &GenerateConfig::default()); + // Repeatable --attach flag with Append action + assert!( + artifacts.cli_rs.contains( + "#[arg(long = \"attach\", action = clap::ArgAction::Append, required = false)]" + ) + ); + // Companion --attach-mime flag, also repeatable, declared flag emitted + assert!(artifacts.cli_rs.contains( + "#[arg(long = \"attach-mime\", action = clap::ArgAction::Append)]\n pub attach_mime: Option>" + )); + // parameters_json maps the repeatable flag back to the wire name and + // carries the companion alongside + assert!( + artifacts + .cli_rs + .contains("\"attachments\": args.attachments.clone().unwrap_or_default()") + ); + assert!( + artifacts + .cli_rs + .contains("\"attach_mime\": args.attach_mime.clone()") + ); +} + +#[test] +fn json_parameter_schema_flows_into_mcp_input_schema() { + let artifacts = generate_all(&definition_with_attachments(), &GenerateConfig::default()); + let parsed: serde_json::Value = serde_json::from_str(&artifacts.mcp_json).unwrap(); + let schema = &parsed["tools"][0]["inputSchema"]["properties"]["attachments"]; + assert_eq!(schema["type"], "array"); + assert!(schema["items"]["oneOf"].is_array()); + // The declared schema is embedded verbatim with the description merged in + assert_eq!(schema["description"], "Attachments to send."); +} + +#[test] +fn scalar_definitions_unchanged_by_feature() { + // Definitions that don't use json params or cli overrides must produce + // byte-identical CLI output (the pre-feature generator). + let artifacts = generate_all(&sample_definition(), &GenerateConfig::default()); + assert!( + artifacts + .cli_rs + .contains("#[arg(long)]\n pub limit: Option,") + ); + assert!(!artifacts.cli_rs.contains("ArgAction::Append")); + assert!(!artifacts.cli_rs.contains("long = \"")); +} + +#[test] +fn cli_flag_override_without_multiple_keeps_scalar_shape() { + let mut definition = sample_definition(); + definition.operations[0].parameters[0].cli = Some(hydra_core::CliOverride { + flag: Some("max-items".into()), + multiple: false, + companions: vec![], + }); + let artifacts = generate_all(&definition, &GenerateConfig::default()); + assert!(artifacts.cli_rs.contains("#[arg(long = \"max-items\")]")); + assert!(artifacts.cli_rs.contains("pub limit: Option,")); + // parameters_json keeps the wire name `limit` + assert!(artifacts.cli_rs.contains("\"limit\": args.limit.clone()")); +} + +#[test] +fn rejects_json_parameter_without_schema() { + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.schema = None; + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_json_parameter_on_non_body_location() { + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.location = ParameterLocation::Query; + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_schema_on_scalar_parameter() { + let mut definition = sample_definition(); + definition.operations[0].parameters[0].schema = Some(serde_json::json!({"type": "integer"})); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_json_parameter_without_cli_block_on_cli_operation() { + let mut definition = definition_with_attachments(); + definition.operations[0].parameters[1].cli = None; + assert!(hydra_core::validate::validate_definition(&definition).is_err()); + // ...but a json param is fine without cli on an operation that does not + // generate the CLI surface + definition.operations[0].surfaces = + Some(vec![hydra_core::Surface::Http, hydra_core::Surface::Mcp]); + assert!(hydra_core::validate::validate_definition(&definition).is_ok()); +} + +#[test] +fn rejects_cli_block_on_non_cli_operation() { + let mut definition = definition_with_attachments(); + definition.operations[0].surfaces = + Some(vec![hydra_core::Surface::Http, hydra_core::Surface::Mcp]); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_multiple_on_scalar_parameter() { + let mut definition = sample_definition(); + definition.operations[0].parameters[0].cli = Some(hydra_core::CliOverride { + flag: None, + multiple: true, + companions: vec![], + }); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_colliding_cli_flags() { + let mut definition = definition_with_attachments(); + // The companion flag `attach-mime` collides with... nothing yet; make the + // parameter's own flag collide with the companion. + let parameter = &mut definition.operations[0].parameters[1]; + parameter.cli = Some(hydra_core::CliOverride { + flag: Some("attach-mime".into()), + multiple: true, + companions: vec![hydra_core::CliCompanion { + flag: "attach-mime".into(), + field: "attach_mime".into(), + description: "Colliding companion.".into(), + }], + }); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_non_kebab_flag_and_invalid_companion_field() { + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.cli = Some(hydra_core::CliOverride { + flag: Some("Attach_Path".into()), + multiple: true, + companions: vec![], + }); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); + + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.cli = Some(hydra_core::CliOverride { + flag: None, + multiple: true, + companions: vec![hydra_core::CliCompanion { + flag: "attach-mime".into(), + field: "attach-mime".into(), // not snake_case + description: "Bad field.".into(), + }], + }); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +// ── review-panel regression tests (round 2) ──────────────────────────────── + +#[test] +fn companion_flag_name_is_emitted_not_derived() { + // A companion whose flag diverges from the kebab derivation of its + // field must emit the declared flag verbatim. + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.cli = Some(hydra_core::CliOverride { + flag: Some("attach".into()), + multiple: true, + companions: vec![hydra_core::CliCompanion { + flag: "mime-override".into(), + field: "attach_mime".into(), + description: "MIME override.".into(), + }], + }); + let artifacts = generate_all(&definition, &GenerateConfig::default()); + assert!(artifacts.cli_rs.contains( + "#[arg(long = \"mime-override\", action = clap::ArgAction::Append)]\n pub attach_mime: Option>" + )); +} + +#[test] +fn rejects_override_flag_colliding_with_default_parameter_flag() { + // A default-shaped parameter's derived flag must collide with an + // explicit override on another parameter. + let mut definition = definition_with_attachments(); + // attachments has cli.flag = attach; rename the scalar `limit` + // parameter's flag to `attach` — collision via the derived default? + // No: limit's default flag is `limit`. Set attachments' flag to + // `limit` instead. + let parameter = &mut definition.operations[0].parameters[1]; + parameter.cli = Some(hydra_core::CliOverride { + flag: Some("limit".into()), + multiple: true, + companions: vec![], + }); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_companion_field_colliding_with_parameter_field() { + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.cli = Some(hydra_core::CliOverride { + flag: Some("attach".into()), + multiple: true, + companions: vec![hydra_core::CliCompanion { + flag: "limit-override".into(), + field: "limit".into(), // collides with the `limit` parameter + description: "Bad.".into(), + }], + }); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn rejects_cli_block_on_path_parameter() { + let mut definition = definition_with_attachments(); + // Make the scalar limit parameter a path param with a cli block. + definition.operations[0].parameters[0].location = ParameterLocation::Path; + definition.operations[0].parameters[0].cli = Some(hydra_core::CliOverride { + flag: Some("max".into()), + multiple: false, + companions: vec![], + }); + // Path param needs matching placeholder; adjust the path. + definition.operations[0].path = "/items/{limit}".into(); + assert!(hydra_core::validate::validate_definition(&definition).is_err()); +} + +#[test] +fn cli_block_without_flag_uses_default_long_attribute() { + // cli: present, flag omitted: clap derives the flag from the field + // name (kebab), so the plain #[arg(long)] attribute is emitted — + // identical to the default shape. + let mut definition = sample_definition(); + definition.operations[0].parameters[0].cli = Some(hydra_core::CliOverride { + flag: None, + multiple: false, + companions: vec![], + }); + let artifacts = generate_all(&definition, &GenerateConfig::default()); + assert!( + artifacts + .cli_rs + .contains("#[arg(long)]\n pub limit: Option,") + ); +} + +#[test] +fn required_multiple_flag_carries_required_true() { + let mut definition = definition_with_attachments(); + let parameter = &mut definition.operations[0].parameters[1]; + parameter.required = true; + let artifacts = generate_all(&definition, &GenerateConfig::default()); + assert!(artifacts.cli_rs.contains("required = true")); +} diff --git a/crates/hydra-core/src/lib.rs b/crates/hydra-core/src/lib.rs index 9aaddbb..b937845 100644 --- a/crates/hydra-core/src/lib.rs +++ b/crates/hydra-core/src/lib.rs @@ -201,6 +201,60 @@ pub struct Parameter { pub required: bool, /// Where this parameter appears in HTTP requests. pub location: ParameterLocation, + /// JSON Schema describing the wire shape of a `json` parameter. + /// + /// Declared explicitly — never inferred — and embedded verbatim (plus + /// the parameter description) into generated MCP tool input schemas. + /// Required for `json` parameters, forbidden for scalar ones. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub schema: Option, + /// Explicit CLI representation override. Declared — never inferred — + /// for parameters whose CLI shape differs from the default single + /// `--name` flag. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub cli: Option, +} + +/// Explicit CLI representation for a parameter. +/// +/// The default CLI shape is one `--name` flag bound to the parameter. +/// When that is wrong (repeatable flags, companion flags that only make +/// sense on the CLI), the definition declares it here. Everything is +/// explicit: flag names, field names, and descriptions. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub struct CliOverride { + /// Kebab-case flag name override. Defaults to the parameter name. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub flag: Option, + /// Repeatable flag → `Vec` clap field. `json` parameters only. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub multiple: bool, + /// Additional CLI-only repeatable string flags (e.g. `--attach-mime`). + /// Emitted as `Option>` clap fields; never part of the + /// HTTP or MCP surface. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub companions: Vec, +} + +/// A CLI-only companion flag paired with a parameter's own CLI flags. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub struct CliCompanion { + /// Kebab-case flag name, e.g. `attach-mime`. + pub flag: String, + /// Stable `snake_case` struct field name, e.g. `attach_mime`. + pub field: String, + /// Human-readable description for CLI help. + pub description: String, +} + +impl CliOverride { + /// Effective flag name: the declared override or the parameter name. + #[must_use] + pub fn effective_flag(&self, parameter_name: &str) -> String { + self.flag + .clone() + .unwrap_or_else(|| parameter_name.to_owned()) + } } /// Parameter type supported by the first-generation codegen contract. @@ -213,6 +267,12 @@ pub enum ParameterType { U32, /// Boolean flag. Bool, + /// Arbitrary JSON value on the body location. Requires a declared + /// `schema`, which flows into generated MCP tool input schemas; the + /// HTTP body already arrives as an untyped JSON value, so the + /// runtime owns validation. `json` parameters on CLI-generating + /// operations must declare a `cli:` representation override. + Json, } impl ParameterType { @@ -223,6 +283,7 @@ impl ParameterType { Self::String => "String", Self::U32 => "u32", Self::Bool => "bool", + Self::Json => "serde_json::Value", } } @@ -233,6 +294,7 @@ impl ParameterType { Self::String => "string", Self::U32 => "integer", Self::Bool => "boolean", + Self::Json => "object", } } } diff --git a/crates/hydra-core/src/validate.rs b/crates/hydra-core/src/validate.rs index bce5448..327214a 100644 --- a/crates/hydra-core/src/validate.rs +++ b/crates/hydra-core/src/validate.rs @@ -10,7 +10,7 @@ use anyhow::Result; use crate::{ ApiDefinition, GENERATED_HTTP_RESERVED_NAMES, HttpMethod, Operation, ParameterLocation, - RUST_KEYWORDS, Surface, + ParameterType, RUST_KEYWORDS, Surface, }; /// Validate semantic constraints that YAML parsing alone cannot enforce. @@ -53,6 +53,7 @@ pub fn validate_definition(definition: &ApiDefinition) -> Result<()> { validate_operation_parameters(operation)?; validate_operation_surfaces(operation)?; validate_operation_raw_request(operation)?; + validate_operation_cli_overrides(operation)?; } // CLI command overrides must not collide with any generated subcommand. @@ -124,6 +125,142 @@ fn validate_operation_parameters(operation: &Operation) -> Result<()> { Ok(()) } +/// Validate json parameters, declared schemas, and CLI representation +/// overrides for one operation. Everything is declared explicitly — this +/// layer exists so a generated surface can never guess a parameter's CLI +/// shape or MCP schema. +fn validate_operation_cli_overrides(operation: &Operation) -> Result<()> { + // Collision namespaces across the whole operation: CLI struct field + // names and effective long flags. Every CLI-visible parameter and + // companion registers here — including parameters using the default + // CLI shape — so explicit overrides cannot collide with defaults. + let mut cli_fields: std::collections::BTreeSet = std::collections::BTreeSet::new(); + let mut cli_flags: std::collections::BTreeSet = std::collections::BTreeSet::new(); + + for parameter in &operation.parameters { + let label = format!("{}.{}", operation.name, parameter.name); + validate_parameter_schema(parameter, &label)?; + + // CLI registration only matters when the CLI surface is generated. + if !operation.generates_cli() { + anyhow::ensure!( + parameter.cli.is_none(), + "parameter {label} declares a `cli` representation but operation {} \ + does not generate the CLI surface", + operation.name + ); + continue; + } + + if let Some(cli) = ¶meter.cli { + anyhow::ensure!( + parameter.location != ParameterLocation::Path, + "parameter {label} declares a `cli` representation but is a path \ + parameter; path parameters are positional and keep their default \ + CLI shape" + ); + if let Some(flag) = &cli.flag { + anyhow::ensure!( + is_kebab_case(flag), + "parameter {label} declares cli flag {flag:?} which is not kebab-case" + ); + } + anyhow::ensure!( + !cli.multiple || parameter.ty == ParameterType::Json, + "parameter {label} declares cli multiple: true but is not type json; \ + repeatable flags are json-parameter-only" + ); + } else { + // No CLI override: a json parameter would fall back to an + // inferred CLI shape (a single Value flag), which is forbidden. + anyhow::ensure!( + parameter.ty != ParameterType::Json, + "parameter {label} is type json on a CLI-generating operation \ + but declares no `cli` representation; the CLI shape must be \ + declared explicitly (flag/multiple/companions)" + ); + } + + // Register the parameter's CLI field name and, for non-positional + // parameters, its effective long flag (declared override or the + // kebab-case derivation clap applies to the field name). + anyhow::ensure!( + cli_fields.insert(parameter.name.clone()), + "operation {} declares colliding CLI fields: {}", + operation.name, + parameter.name + ); + if parameter.location != ParameterLocation::Path { + let effective_flag = parameter + .cli + .as_ref() + .and_then(|cli| cli.flag.clone()) + .unwrap_or_else(|| parameter.name.replace('_', "-")); + anyhow::ensure!( + cli_flags.insert(effective_flag.clone()), + "operation {} declares colliding CLI flags: {effective_flag}", + operation.name + ); + } + + if let Some(cli) = ¶meter.cli { + for companion in &cli.companions { + anyhow::ensure!( + is_kebab_case(&companion.flag), + "parameter {label} companion declares flag {:?} which is not kebab-case", + companion.flag + ); + anyhow::ensure!( + crate::validate::is_valid_identifier(&companion.field), + "parameter {label} companion declares field {:?} which is not a \ + Rust-safe snake_case identifier", + companion.field + ); + anyhow::ensure!( + cli_flags.insert(companion.flag.clone()), + "operation {} declares colliding CLI flags: {}", + operation.name, + companion.flag + ); + anyhow::ensure!( + cli_fields.insert(companion.field.clone()), + "operation {} declares colliding CLI fields: {} (companion field)", + operation.name, + companion.field + ); + } + } + } + Ok(()) +} + +/// Validate the json/schema pairing for one parameter: json parameters +/// are body-only and must declare an object schema; schemas are +/// json-parameter-only. +fn validate_parameter_schema(parameter: &crate::Parameter, label: &str) -> Result<()> { + if parameter.ty == ParameterType::Json { + anyhow::ensure!( + parameter.location == ParameterLocation::Body, + "parameter {label} is type json but not located in the body; \ + json parameters are body-only" + ); + anyhow::ensure!( + parameter + .schema + .as_ref() + .is_some_and(serde_json::Value::is_object), + "parameter {label} is type json but declares no object `schema`; \ + json parameters must declare their JSON Schema explicitly" + ); + } + anyhow::ensure!( + parameter.ty == ParameterType::Json || parameter.schema.is_none(), + "parameter {label} declares a `schema` but is not type json; \ + schemas are json-parameter-only" + ); + Ok(()) +} + /// Validate the raw-request escape hatch: it is an HTTP-surface-only /// feature, so raw operations must not appear on CLI or MCP, must not /// stream, and must not declare body-location parameters (the raw bytes diff --git a/examples/notes/api/operations.yaml b/examples/notes/api/operations.yaml index 5bb45e7..a29ecf7 100644 --- a/examples/notes/api/operations.yaml +++ b/examples/notes/api/operations.yaml @@ -77,3 +77,52 @@ operations: parameters: [] surfaces: [http] raw_request: true + - name: annotate_note + description: Append an annotation to a note, optionally carrying attachments. + method: POST + path: /notes/{note_id}/annotate + read: false + output_type: Value + parameters: + - name: note_id + description: Note ID to annotate. + type: string + required: true + location: path + - name: body + description: Annotation text. + type: string + required: true + location: body + - name: attachments + description: Attachments to attach; each is inline bytes or a stored reference. + type: json + required: false + location: body + schema: + type: array + items: + oneOf: + - type: object + properties: + mime_type: + type: string + filename: + type: string + data_base64: + type: string + required: [mime_type, data_base64] + additionalProperties: false + - type: object + properties: + stored_id: + type: string + required: [stored_id] + additionalProperties: false + cli: + flag: attach + multiple: true + companions: + - flag: attach-mime + field: attach_mime + description: MIME type for the corresponding local-path --attach value. diff --git a/examples/notes/generated/cli.rs b/examples/notes/generated/cli.rs index 62d1f4c..1209d7c 100644 --- a/examples/notes/generated/cli.rs +++ b/examples/notes/generated/cli.rs @@ -16,6 +16,8 @@ pub enum GeneratedCommand { DeleteNote(DeleteNoteArgs), /// Internal compaction job; exposed on CLI only for operators. CompactNotes(CompactNotesArgs), + /// Append an annotation to a note, optionally carrying attachments. + AnnotateNote(AnnotateNoteArgs), } impl GeneratedCommand { @@ -26,6 +28,7 @@ impl GeneratedCommand { Self::CreateNote(_) => "create_note", Self::DeleteNote(_) => "delete_note", Self::CompactNotes(_) => "compact_notes", + Self::AnnotateNote(_) => "annotate_note", } } @@ -36,6 +39,7 @@ impl GeneratedCommand { Self::CreateNote(args) => serde_json::json!({"title": args.title.clone(), "body": args.body.clone()}), Self::DeleteNote(args) => serde_json::json!({"note_id": args.note_id.clone()}), Self::CompactNotes(_args) => serde_json::json!({}), + Self::AnnotateNote(args) => serde_json::json!({"note_id": args.note_id.clone(), "body": args.body.clone(), "attachments": args.attachments.clone().unwrap_or_default(), "attach_mime": args.attach_mime.clone()}), } } } @@ -73,3 +77,18 @@ pub struct DeleteNoteArgs { pub struct CompactNotesArgs { } +#[derive(Debug, Clone, Serialize, Deserialize, Args)] +pub struct AnnotateNoteArgs { + /// Note ID to annotate. + pub note_id: String, + /// Annotation text. + #[arg(long)] + pub body: String, + /// Attachments to attach; each is inline bytes or a stored reference. + #[arg(long = "attach", action = clap::ArgAction::Append, required = false)] + pub attachments: Option>, + /// MIME type for the corresponding local-path --attach value. + #[arg(long = "attach-mime", action = clap::ArgAction::Append)] + pub attach_mime: Option>, +} + diff --git a/examples/notes/generated/http.rs b/examples/notes/generated/http.rs index a2ba99d..07e44af 100644 --- a/examples/notes/generated/http.rs +++ b/examples/notes/generated/http.rs @@ -45,6 +45,7 @@ pub const GENERATED_ROUTES: &[GeneratedRoute] = &[ GeneratedRoute { name: "delete_note", method: "POST", path: "/notes/{note_id}/delete" }, GeneratedRoute { name: "note_stats", method: "GET", path: "/stats" }, GeneratedRoute { name: "echo_raw", method: "POST", path: "/hooks/echo" }, + GeneratedRoute { name: "annotate_note", method: "POST", path: "/notes/{note_id}/annotate" }, ]; pub fn generated_router() -> Router { @@ -55,6 +56,7 @@ pub fn generated_router() -> Router { .route("/notes/{note_id}/delete", post(delete_note)) .route("/stats", get(note_stats)) .route("/hooks/echo", post(echo_raw)) + .route("/notes/{note_id}/annotate", post(annotate_note)) } async fn list_notes( @@ -162,3 +164,20 @@ async fn echo_raw( ) .await } + +async fn annotate_note( + State(state): State, + Path(path): Path>, + Json(body): Json, +) -> Response { + crate::execute_operation_http( + &state, + "annotate_note", + GeneratedOperationInput { + path, + query: BTreeMap::new(), + body, + }, + ) + .await +} diff --git a/examples/notes/generated/mcp.json b/examples/notes/generated/mcp.json index 89a1dbf..4b62eb4 100644 --- a/examples/notes/generated/mcp.json +++ b/examples/notes/generated/mcp.json @@ -1,5 +1,10 @@ { "locations": { + "annotate_note": { + "attachments": "body", + "body": "body", + "note_id": "path" + }, "create_note": { "body": "body", "title": "body" @@ -96,6 +101,67 @@ "type": "object" }, "name": "note_stats" + }, + { + "description": "Append an annotation to a note, optionally carrying attachments.", + "inputSchema": { + "additionalProperties": false, + "properties": { + "attachments": { + "description": "Attachments to attach; each is inline bytes or a stored reference.", + "items": { + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "data_base64": { + "type": "string" + }, + "filename": { + "type": "string" + }, + "mime_type": { + "type": "string" + } + }, + "required": [ + "mime_type", + "data_base64" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "stored_id": { + "type": "string" + } + }, + "required": [ + "stored_id" + ], + "type": "object" + } + ] + }, + "type": "array" + }, + "body": { + "description": "Annotation text.", + "type": "string" + }, + "note_id": { + "description": "Note ID to annotate.", + "type": "string" + } + }, + "required": [ + "note_id", + "body" + ], + "type": "object" + }, + "name": "annotate_note" } ] } diff --git a/examples/notes/src/lib.rs b/examples/notes/src/lib.rs index 7b8209a..ddf571b 100644 --- a/examples/notes/src/lib.rs +++ b/examples/notes/src/lib.rs @@ -156,6 +156,36 @@ pub async fn execute_operation( } Ok(Value::Null) } + "annotate_note" => { + #[derive(Deserialize)] + struct AnnotateArgs { + body: String, + #[serde(default)] + attachments: Value, + } + let args: AnnotateArgs = serde_json::from_value(input.body.clone()) + .map_err(|e| bad_request(&format!("invalid body: {e}")))?; + let id = path_str(&input, "note_id")?; + let note = state + .notes + .lock() + .expect("notes lock") + .iter_mut() + .find(|n| n.id == id) + .map(|note| { + note.body.push_str("\n[annotation] "); + note.body.push_str(&args.body); + note.clone() + }) + .ok_or_else(|| not_found(&format!("note not found: {id}")))?; + // Echo the decoded attachment union back, content-free summary + // only — proving the wire shape survives all three surfaces. + let attachments = summarize_attachments(&args.attachments)?; + Ok(serde_json::json!({ + "note": note, + "attachments": attachments, + })) + } "note_stats" => { let out_stats = { let notes = state.notes.lock().expect("notes lock"); @@ -206,6 +236,55 @@ fn not_found(message: &str) -> OperationError { } } +/// Validate the closed attachment union on the body and return a +/// content-free summary: each item must be exactly inline +/// (`mime_type` + `data_base64`, optional `filename`) or stored +/// (`stored_id`); unknown or mixed fields are rejected. +fn summarize_attachments(attachments: &Value) -> Result, OperationError> { + let Some(items) = attachments.as_array() else { + // Absent (null) or empty — no attachments to validate. + if attachments.is_null() { + return Ok(Vec::new()); + } + return Err(bad_request("attachments must be an array")); + }; + let mut summaries = Vec::with_capacity(items.len()); + for item in items { + let Some(object) = item.as_object() else { + return Err(bad_request("each attachment must be an object")); + }; + let has_inline = object.contains_key("mime_type") || object.contains_key("data_base64"); + let has_stored = object.contains_key("stored_id"); + if has_stored && has_inline { + return Err(bad_request("attachment mixes inline and stored fields")); + } + let summary = if has_stored { + if object.len() != 1 { + return Err(bad_request("stored attachment must carry only stored_id")); + } + serde_json::json!({ "kind": "stored", "stored_id": object["stored_id"] }) + } else if has_inline { + if !object.contains_key("mime_type") || !object.contains_key("data_base64") { + return Err(bad_request( + "inline attachment requires mime_type and data_base64", + )); + } + for key in object.keys() { + if !matches!(key.as_str(), "mime_type" | "data_base64" | "filename") { + return Err(bad_request(&format!( + "unknown inline attachment field: {key}" + ))); + } + } + serde_json::json!({ "kind": "inline", "mime_type": object["mime_type"] }) + } else { + return Err(bad_request("attachment must be inline or stored")); + }; + summaries.push(summary); + } + Ok(summaries) +} + fn bad_request(message: &str) -> OperationError { OperationError { status: StatusCode::BAD_REQUEST, diff --git a/examples/notes/tests/surfaces.rs b/examples/notes/tests/surfaces.rs index cb5fb80..f2d4f9f 100644 --- a/examples/notes/tests/surfaces.rs +++ b/examples/notes/tests/surfaces.rs @@ -212,3 +212,163 @@ fn raw_request_route_absent_from_cli_and_mcp() { assert!(!cli_rs.contains("echo_raw") && !cli_rs.contains("EchoRaw")); assert!(!mcp_json.contains("echo_raw")); } + +// ── annotate_note: json parameters + CLI representation (COD-411) ────────── + +#[tokio::test] +async fn annotate_note_accepts_valid_inline_and_stored_unions_over_http() { + let state = AppState::with_fixtures(); + let res = http( + &state, + Request::post("/notes/n1/annotate") + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&json!({ + "body": "see attachment", + "attachments": [ + {"mime_type": "image/png", "filename": "a.png", "data_base64": "aGk="}, + {"stored_id": "11111111-1111-1111-1111-111111111111"} + ] + })) + .unwrap(), + )) + .unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::OK); + let body = body_json(res).await; + assert_eq!( + body["attachments"], + json!([ + {"kind": "inline", "mime_type": "image/png"}, + {"kind": "stored", "stored_id": "11111111-1111-1111-1111-111111111111"} + ]) + ); + assert!( + body["note"]["body"] + .as_str() + .unwrap() + .contains("[annotation] see attachment") + ); +} + +#[tokio::test] +async fn annotate_note_rejects_malformed_unions_with_400() { + let state = AppState::with_fixtures(); + for bad in [ + // mixed inline + stored + json!([{"mime_type": "image/png", "data_base64": "aGk=", "stored_id": "x"}]), + // inline missing data_base64 + json!([{"mime_type": "image/png"}]), + // stored with extra field + json!([{"stored_id": "x", "filename": "y"}]), + // unknown field + json!([{"mime_type": "image/png", "data_base64": "aGk=", "url": "https://x"}]), + // neither variant + json!([{"filename": "only.png"}]), + ] { + let res = http( + &state, + Request::post("/notes/n1/annotate") + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&json!({"body": "b", "attachments": bad})).unwrap(), + )) + .unwrap(), + ) + .await; + assert_eq!( + res.status(), + StatusCode::BAD_REQUEST, + "union {bad} must 400" + ); + } +} + +#[tokio::test] +async fn annotate_note_without_attachments_is_text_only() { + let state = AppState::with_fixtures(); + let res = http( + &state, + Request::post("/notes/n1/annotate") + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&json!({"body": "plain"})).unwrap(), + )) + .unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::OK); + let body = body_json(res).await; + assert_eq!(body["attachments"], json!([])); +} + +#[test] +fn generated_cli_parses_repeatable_attach_flags_into_wire_shape() { + use clap::Parser; + + #[derive(Parser)] + struct Cli { + #[command(subcommand)] + command: notes_example::generated_cli::GeneratedCommand, + } + + let cli = Cli::try_parse_from([ + "notes", + "annotate-note", + "n1", + "--body", + "see attachments", + "--attach", + "/tmp/a.png", + "--attach", + "iris://attachment/11111111-1111-1111-1111-111111111111", + "--attach-mime", + "image/png", + ]) + .expect("repeatable flags parse"); + let notes_example::generated_cli::GeneratedCommand::AnnotateNote(args) = cli.command else { + panic!("expected annotate-note subcommand"); + }; + assert_eq!( + args.attachments, + Some(vec![ + "/tmp/a.png".to_string(), + "iris://attachment/11111111-1111-1111-1111-111111111111".to_string(), + ]) + ); + assert_eq!(args.attach_mime, Some(vec!["image/png".to_string()])); + // parameters_json maps CLI shape back to the wire shape + let params = + notes_example::generated_cli::GeneratedCommand::AnnotateNote(args).parameters_json(); + assert_eq!( + params["attachments"], + json!([ + "/tmp/a.png", + "iris://attachment/11111111-1111-1111-1111-111111111111" + ]) + ); + assert_eq!(params["attach_mime"], json!(["image/png"])); + // body + note_id flow through unchanged + assert_eq!(params["body"], json!("see attachments")); + assert_eq!(params["note_id"], json!("n1")); +} + +#[test] +fn generated_mcp_tool_schema_carries_declared_union() { + let mcp: Value = serde_json::from_str(include_str!("../generated/mcp.json")).unwrap(); + let tool = mcp["tools"] + .as_array() + .unwrap() + .iter() + .find(|t| t["name"] == json!("annotate_note")) + .expect("annotate_note tool present"); + let attachments = &tool["inputSchema"]["properties"]["attachments"]; + assert_eq!(attachments["type"], json!("array")); + let one_of = attachments["items"]["oneOf"].as_array().unwrap(); + assert_eq!(one_of.len(), 2); + assert_eq!(one_of[0]["required"], json!(["mime_type", "data_base64"])); + assert_eq!(one_of[1]["required"], json!(["stored_id"])); + assert_eq!(one_of[0]["additionalProperties"], json!(false)); + assert_eq!(one_of[1]["additionalProperties"], json!(false)); +}