diff --git a/crates/app/src/ui/batch_workflow.rs b/crates/app/src/ui/batch_workflow.rs index f1144ae..198c425 100644 --- a/crates/app/src/ui/batch_workflow.rs +++ b/crates/app/src/ui/batch_workflow.rs @@ -188,7 +188,9 @@ impl AutomationUi { for target in &plan.targets { ui.label(format!( "{:?} · {} · {}", - target.status, target.target.id, target.reason + target.status, + target.target.describe(), + target.reason )); } }); @@ -389,7 +391,37 @@ impl AutomationUi { result.tool_id, result.before_revision.0, result.after_revision.0 )); for target in &result.targets { - ui.label(format!("{:?} · {}", target.outcome, target.message)); + // The target identity has to appear here: a property tool + // expands one plot object into one result per series, and + // without the component every one of those rows reads the same. + ui.label(format!( + "{:?} · {} · {}", + target.outcome, + target.target.describe(), + target.message + )); + } + // The per-target rows report what happened, never what was read: a + // tool's readings live in the result value. Without this an inspect + // call reports success and shows no numbers at all. + if !result.value.is_null() { + match serde_json::to_string_pretty(&result.value) { + Ok(text) => { + ui.label("Result value (JSON)"); + egui::ScrollArea::vertical() + .max_height(160.0) + .id_salt("automation_result_value") + .show(ui, |ui| { + ui.monospace(text); + }); + } + Err(error) => { + ui.colored_label( + ui.visuals().error_fg_color, + format!("The result value could not be shown: {error}"), + ); + } + } } } ui.small(format!( diff --git a/crates/core/src/automation/mod.rs b/crates/core/src/automation/mod.rs index b6dd5da..2d362b1 100644 --- a/crates/core/src/automation/mod.rs +++ b/crates/core/src/automation/mod.rs @@ -2,13 +2,16 @@ //! agents. The public boundary is deliberately semantic: callers can inspect, //! select and invoke registered tools, but cannot construct document actions. +mod properties; mod registry; mod resources; mod tasks; +mod tool_executors; mod tools; mod types; mod workflow; +pub use properties::{TOOL_INSPECT, TOOL_RESET, TOOL_SET}; pub use registry::ToolRegistry; pub use resources::*; pub use tasks::*; diff --git a/crates/core/src/automation/properties.rs b/crates/core/src/automation/properties.rs new file mode 100644 index 0000000..96f222b --- /dev/null +++ b/crates/core/src/automation/properties.rs @@ -0,0 +1,567 @@ +//! The JSON boundary of the property catalog. +//! +//! `properties.inspect` / `properties.set` / `properties.reset` are ordinary +//! [`ToolRegistry`](super::ToolRegistry) tools. Everything they do beyond the +//! wire format — resolving a target, deciding applicability, validating a +//! value, folding a write into one atomic action — is delegated to the planner +//! in [`crate::properties`] that the panel controls already call. This module +//! therefore contains no planning and no validation *rules*: it decodes JSON +//! into the typed values that planner accepts, and encodes what it returns. +//! +//! That split is the whole point of the module. A second copy of the rules here +//! would drift from the one the UI uses, and the two entry points would quietly +//! disagree about what a value means — which is precisely the failure the +//! differential test in `properties_tests.rs` exists to catch. + +use super::registry::parse; +use super::*; +use crate::properties::{ + AggregateValue, Availability, EnumVariant, FloatBounds, PropertyAccess, PropertyAddress, + PropertyDefinition, PropertyError, PropertyValue, ResolvedProperty, ResolvedSchema, + ValueSchema, definition_by_key, variant_list, +}; +use crate::state::PlotxApp; +use serde::{Deserialize, Serialize}; + +pub const TOOL_INSPECT: &str = "properties.inspect"; +pub const TOOL_SET: &str = "properties.set"; +pub const TOOL_RESET: &str = "properties.reset"; + +/// Whether a tool id belongs to the property catalog. +pub(super) fn is_property_tool(tool_id: &str) -> bool { + matches!(tool_id, TOOL_INSPECT | TOOL_SET | TOOL_RESET) +} + +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +pub(super) struct PropertyKeyParams { + pub key: String, +} + +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +pub(super) struct PropertyWriteParams { + pub key: String, + pub value: serde_json::Value, +} + +/// Expand a planned resource list into the components a property addresses. +/// +/// Called for every tool; it is a no-op for all but the three property tools, +/// so the twelve pre-existing tools are planned by exactly the code that +/// planned them before. Targets the shared kind/capability gate already +/// rejected are passed through untouched, keeping their original reason: a text +/// box is skipped because it does not expose the property-catalog capability, +/// and nothing here needs to know that text boxes exist. +pub(super) fn refine_plan( + app: &PlotxApp, + request: &ToolRequest, + targets: Vec, +) -> Result, AutomationError> { + if !is_property_tool(&request.tool_id) { + return Ok(targets); + } + let definition = requested_definition(&request.tool_id, &request.parameters)?; + if request.tool_id != TOOL_INSPECT && definition.access == PropertyAccess::ReadOnly { + return Err(AutomationError::InvalidParameters { + tool_id: request.tool_id.clone(), + message: format!("property '{}' is read-only", definition.id), + }); + } + if request.tool_id == TOOL_SET { + // Decode here as well as at execution. Decoding is pure, so this + // duplicates no rule — but a preflight that accepts a value the commit + // will refuse is a preflight that answered the wrong question, and the + // caller finds out only after confirming. + let params: PropertyWriteParams = parse(&request.tool_id, request.parameters.clone())?; + decode_value(&request.tool_id, definition, ¶ms.value)?; + } + let mut expanded = Vec::new(); + for planned in targets { + if planned.status != TargetCompatibility::Compatible { + expanded.push(planned); + continue; + } + let components = app.resource_series_targets(&planned.target.resource); + if components.is_empty() { + expanded.push(PlannedTarget { + target: planned.target, + status: TargetCompatibility::Skipped, + reason: "this plot has no series to address".to_owned(), + }); + continue; + } + for target in components { + // Applicability per component comes from the definition, resolved + // against the series' own field — never from the tool descriptor, + // which cannot see a series at all. A read is the cheapest form of + // that same question, and asking it here means the plan preview and + // the commit agree by construction. + let address = PropertyAddress::new(target.clone(), definition.id); + let (status, reason) = match app.resolve_property(&address) { + Ok(_) => ( + TargetCompatibility::Compatible, + format!("{} applies to this series", definition.canonical_label), + ), + Err(error) => (TargetCompatibility::Skipped, error.to_string()), + }; + expanded.push(PlannedTarget { + target, + status, + reason, + }); + } + } + Ok(expanded) +} + +pub(super) fn execute( + app: &mut PlotxApp, + plan: &ToolPlan, + compatible: Vec, +) -> Result { + let tool_id = plan.request.tool_id.clone(); + let definition = requested_definition(&tool_id, &plan.request.parameters)?; + match tool_id.as_str() { + TOOL_INSPECT => inspect(app, plan, definition, compatible), + TOOL_SET => { + let params: PropertyWriteParams = parse(&tool_id, plan.request.parameters.clone())?; + let value = decode_value(&tool_id, definition, ¶ms.value)?; + let commit = app + .plan_property_write(definition.id, &compatible, &value) + .map_err(|error| property_error(&tool_id, error))?; + commit_and_report(app, plan, commit, "set") + } + TOOL_RESET => { + let commit = app + .plan_property_reset(definition.id, &compatible) + .map_err(|error| property_error(&tool_id, error))?; + commit_and_report(app, plan, commit, "reset to its default") + } + _ => Err(AutomationError::UnknownTool(tool_id)), + } +} + +fn inspect( + app: &PlotxApp, + plan: &ToolPlan, + definition: &'static PropertyDefinition, + compatible: Vec, +) -> Result { + let revision = DocumentRevision(app.doc.automation_revision); + let set = app.resolve_property_set(definition.id, &compatible); + let mut readings = Vec::new(); + for address in &set.applicable_targets { + let resolved = app + .resolve_property(address) + .map_err(|error| AutomationError::Execution(error.to_string()))?; + readings.push(reading(&resolved)?); + } + let mut targets = skipped_from_plan(plan); + targets.extend(set.applicable_targets.iter().map(|address| TargetResult { + target: address.target.clone(), + outcome: TargetOutcome::Succeeded, + message: "read".to_owned(), + fingerprints: Vec::new(), + })); + targets.extend(skipped_results(&set.skipped_targets)); + Ok(ToolResult { + tool_id: plan.request.tool_id.clone(), + before_revision: revision, + after_revision: revision, + targets, + produced: Vec::new(), + modified: Vec::new(), + diagnostics: Vec::new(), + verification: Vec::new(), + value: serde_json::to_value(InspectValue { + property: definition.id.as_str(), + canonical_label: definition.canonical_label, + tier: definition.tier.as_str(), + aggregate: aggregate_dto(&set.value), + readings, + }) + .map_err(|error| AutomationError::Execution(error.to_string()))?, + }) +} + +/// Execute a validated commit and report every target exactly once. +/// +/// A commit that applies to nothing is not executed at all. Running an empty +/// composite would advance the document revision and land in the undo stack, +/// so a call that changed nothing would be indistinguishable from one that did +/// — and would give a caller an undo entry that undoes nothing. +fn commit_and_report( + app: &mut PlotxApp, + plan: &ToolPlan, + commit: crate::properties::PropertyCommit, + verb: &str, +) -> Result { + let applied = commit.applied.clone(); + let skipped = commit.skipped.clone(); + let before = DocumentRevision(app.doc.automation_revision); + if !applied.is_empty() { + app.commit_property(commit); + } + let after = DocumentRevision(app.doc.automation_revision); + let mut targets = skipped_from_plan(plan); + targets.extend(applied.iter().map(|address| TargetResult { + target: address.target.clone(), + outcome: TargetOutcome::Succeeded, + message: verb.to_owned(), + fingerprints: Vec::new(), + })); + targets.extend(skipped_results(&skipped)); + let mut modified = Vec::new(); + for address in &applied { + if !modified.contains(&address.target.resource) { + modified.push(address.target.resource.clone()); + } + } + Ok(ToolResult { + tool_id: plan.request.tool_id.clone(), + before_revision: before, + after_revision: after, + targets, + produced: Vec::new(), + modified, + diagnostics: Vec::new(), + verification: vec![VerificationRecord { + check: "revision_advanced".to_owned(), + passed: applied.is_empty() || after > before, + message: if applied.is_empty() { + "no target accepted this property; nothing was committed".to_owned() + } else { + "atomic document commit completed".to_owned() + }, + }], + value: serde_json::Value::Null, + }) +} + +/// The targets the shared gate rejected, carried into the result so a skip is +/// reported rather than dropped between planning and execution. +fn skipped_from_plan(plan: &ToolPlan) -> Vec { + plan.targets + .iter() + .filter(|target| target.status != TargetCompatibility::Compatible) + .map(|target| TargetResult { + target: target.target.clone(), + outcome: TargetOutcome::Skipped, + message: target.reason.clone(), + fingerprints: Vec::new(), + }) + .collect() +} + +/// The targets the *planner* skipped, as opposed to the shared gate. +/// +/// Through these tools this is currently always empty, and deliberately so: +/// `refine_plan` decides applicability by asking `resolve_property`, which is +/// the same question the planner asks, so a target the planner would skip has +/// already been marked `Skipped` at plan time and never reaches the commit. The +/// only way the two could disagree is a document that changed between planning +/// and execution, which the revision check rejects outright. +/// +/// It is kept rather than dropped because the planner's contract is that it +/// reports skips, and this adapter's job is to forward what it reports. Deleting +/// this would replace an empty list with a silent discard, and the first planner +/// rule that does not have a read-side equivalent — a write refused for a reason +/// a read cannot see — would vanish from the result with nothing to notice it. +fn skipped_results(skipped: &[(TargetRef, String)]) -> Vec { + skipped + .iter() + .map(|(target, reason)| TargetResult { + target: target.clone(), + outcome: TargetOutcome::Skipped, + message: reason.clone(), + fingerprints: Vec::new(), + }) + .collect() +} + +fn requested_definition( + tool_id: &str, + parameters: &serde_json::Value, +) -> Result<&'static PropertyDefinition, AutomationError> { + let key = if tool_id == TOOL_SET { + parse::(tool_id, parameters.clone())?.key + } else { + parse::(tool_id, parameters.clone())?.key + }; + // An unknown key is refused outright. Skipping it would report a clean + // success for a call that addressed nothing, which is the worst possible + // answer to a typo in a headless caller's script. + definition_by_key(&key).ok_or_else(|| AutomationError::InvalidParameters { + tool_id: tool_id.to_owned(), + message: format!("unknown property '{key}'"), + }) +} + +/// Decode a JSON value into the typed value the planner accepts. +/// +/// The decoding is driven by the definition's own [`ValueSchema`]: a property +/// value is a small closed set of shapes, and which shape is admissible is a +/// property of the definition, not of the JSON. A generic deserialization would +/// have to guess — and could never produce the `&'static str` an enumerated +/// choice is, because a choice is one of the schema's own variants rather than +/// arbitrary text. +fn decode_value( + tool_id: &str, + definition: &'static PropertyDefinition, + value: &serde_json::Value, +) -> Result { + let invalid = |message: String| AutomationError::InvalidParameters { + tool_id: tool_id.to_owned(), + message: format!("{}: {message}", definition.id), + }; + match definition.value_schema { + ValueSchema::Bool => value + .as_bool() + .map(PropertyValue::Bool) + .ok_or_else(|| invalid(format!("expected true or false, got {}", json_kind(value)))), + ValueSchema::Int { min, max } => { + let number = value + .as_i64() + .ok_or_else(|| invalid(format!("expected an integer, got {}", json_kind(value))))?; + if number < min || number > max { + return Err(invalid(format!( + "{number} is out of range: it must be between {min} and {max}" + ))); + } + Ok(PropertyValue::Int(number)) + } + ValueSchema::Float { bounds, .. } => { + let number = value + .as_f64() + .ok_or_else(|| invalid(format!("expected a number, got {}", json_kind(value))))?; + if !bounds.admits(number) { + return Err(invalid(format!( + "{number} is out of range: it must be {}", + bounds.describe() + ))); + } + Ok(PropertyValue::Float(number)) + } + ValueSchema::Enum { variants } => { + let text = value + .as_str() + .ok_or_else(|| invalid(format!("expected a choice, got {}", json_kind(value))))?; + // Only the *static* variant set is consulted here, because that is + // the wire question: does this string name a choice at all? Whether + // the target's field permits the choice depends on that field's + // capabilities and is answered once, in the planner, for the UI and + // for this adapter alike. + variants + .iter() + .find(|variant| variant.id == text) + .map(|variant| PropertyValue::Enum(variant.id)) + .ok_or_else(|| { + invalid(format!( + "'{text}' is not a choice of this setting; it accepts {}", + variant_list(&variants.iter().collect::>()) + )) + }) + } + ValueSchema::Color => { + let text = value.as_str().ok_or_else(|| { + invalid(format!( + "expected a '#rrggbb' colour, got {}", + json_kind(value) + )) + })?; + parse_color(text) + .map(PropertyValue::Color) + .ok_or_else(|| invalid(format!("'{text}' is not a '#rrggbb' colour"))) + } + } +} + +fn parse_color(text: &str) -> Option { + let digits = text.strip_prefix('#')?; + if digits.len() != 6 || !digits.chars().all(|ch| ch.is_ascii_hexdigit()) { + return None; + } + let channel = |range: std::ops::Range| u8::from_str_radix(&digits[range], 16).ok(); + Some(plotx_figure::Color::rgb( + channel(0..2)?, + channel(2..4)?, + channel(4..6)?, + )) +} + +fn json_kind(value: &serde_json::Value) -> &'static str { + match value { + serde_json::Value::Null => "null", + serde_json::Value::Bool(_) => "a boolean", + serde_json::Value::Number(_) => "a number", + serde_json::Value::String(_) => "a string", + serde_json::Value::Array(_) => "an array", + serde_json::Value::Object(_) => "an object", + } +} + +fn property_error(tool_id: &str, error: PropertyError) -> AutomationError { + match error { + PropertyError::InvalidValue { .. } + | PropertyError::ReadOnly(_) + | PropertyError::UnknownProperty(_) + | PropertyError::ComponentKind { .. } => AutomationError::InvalidParameters { + tool_id: tool_id.to_owned(), + message: error.to_string(), + }, + PropertyError::UnknownTarget(_) | PropertyError::NotApplicable(_) => { + AutomationError::Execution(error.to_string()) + } + } +} + +// --------------------------------------------------------------------------- +// Wire shapes +// +// The catalog model deliberately carries no `serde` derives: it is the +// language-neutral semantic layer, and giving it a wire format would make the +// serialized shape part of its public contract — every future field rename or +// variant split would become a compatibility question for JSON callers. The +// transport shapes live here instead, where changing one is a change to this +// adapter alone. +// --------------------------------------------------------------------------- + +#[derive(Serialize)] +struct InspectValue { + property: &'static str, + canonical_label: &'static str, + tier: &'static str, + aggregate: AggregateValueDto, + readings: Vec, +} + +#[derive(Serialize)] +struct ReadingDto { + target: TargetRef, + value: AggregateValueDto, + #[serde(skip_serializing_if = "Option::is_none")] + default_value: Option, + availability: &'static str, + modified: bool, + schema: ResolvedSchemaDto, +} + +#[derive(Serialize)] +#[serde(tag = "state", rename_all = "snake_case")] +enum AggregateValueDto { + Uniform { value: PropertyValueDto }, + Mixed, + Unavailable, +} + +#[derive(Serialize)] +#[serde(tag = "type", content = "value", rename_all = "snake_case")] +enum PropertyValueDto { + Bool(bool), + Int(i64), + Float(f64), + Enum(&'static str), + Color(String), +} + +#[derive(Serialize)] +#[serde(tag = "type", rename_all = "snake_case")] +enum ResolvedSchemaDto { + Bool, + Int { + min: i64, + max: i64, + }, + Float { + min: f64, + max: f64, + exclusive_min: bool, + log: bool, + unit: &'static str, + }, + Enum { + variants: Vec, + }, + Color, +} + +#[derive(Serialize)] +struct EnumVariantDto { + id: &'static str, + canonical_label: &'static str, +} + +fn reading(resolved: &ResolvedProperty) -> Result { + Ok(ReadingDto { + target: resolved.address.target.clone(), + value: aggregate_dto(&resolved.value), + default_value: resolved.default_value.map(value_dto), + availability: match resolved.availability { + Availability::Editable => "editable", + Availability::ReadOnly => "read_only", + }, + modified: resolved.is_modified(), + schema: schema_dto(&resolved.schema), + }) +} + +fn aggregate_dto(value: &AggregateValue) -> AggregateValueDto { + match value { + AggregateValue::Uniform(value) => AggregateValueDto::Uniform { + value: value_dto(*value), + }, + AggregateValue::Mixed => AggregateValueDto::Mixed, + AggregateValue::Unavailable => AggregateValueDto::Unavailable, + } +} + +fn value_dto(value: PropertyValue) -> PropertyValueDto { + match value { + PropertyValue::Bool(value) => PropertyValueDto::Bool(value), + PropertyValue::Int(value) => PropertyValueDto::Int(value), + PropertyValue::Float(value) => PropertyValueDto::Float(value), + PropertyValue::Enum(value) => PropertyValueDto::Enum(value), + PropertyValue::Color(value) => PropertyValueDto::Color(value.to_hex()), + } +} + +fn schema_dto(schema: &ResolvedSchema) -> ResolvedSchemaDto { + match schema { + ResolvedSchema::Bool => ResolvedSchemaDto::Bool, + ResolvedSchema::Int { min, max } => ResolvedSchemaDto::Int { + min: *min, + max: *max, + }, + ResolvedSchema::Float { bounds, log, unit } => float_schema_dto(*bounds, *log, unit), + ResolvedSchema::Enum { variants } => ResolvedSchemaDto::Enum { + variants: variants.iter().copied().map(variant_dto).collect(), + }, + ResolvedSchema::Color => ResolvedSchemaDto::Color, + } +} + +fn float_schema_dto(bounds: FloatBounds, log: bool, unit: &'static str) -> ResolvedSchemaDto { + ResolvedSchemaDto::Float { + min: bounds.min, + max: bounds.max, + exclusive_min: bounds.exclusive_min, + log, + unit, + } +} + +fn variant_dto(variant: &'static EnumVariant) -> EnumVariantDto { + EnumVariantDto { + id: variant.id, + canonical_label: variant.canonical_label, + } +} + +#[cfg(test)] +#[path = "properties_audit_tests.rs"] +mod audit_tests; + +#[cfg(test)] +#[path = "properties_tests.rs"] +mod tests; diff --git a/crates/core/src/automation/properties_audit_tests.rs b/crates/core/src/automation/properties_audit_tests.rs new file mode 100644 index 0000000..e851e1e --- /dev/null +++ b/crates/core/src/automation/properties_audit_tests.rs @@ -0,0 +1,325 @@ +//! What a tool call leaves behind: run manifests, the document's unsaved flag, +//! and the operation record an external write produces. +//! +//! The JSON boundary itself is covered by `properties_tests.rs`, whose fixtures +//! these tests share — the audit axes are the same whichever tool ran, so they +//! are grouped by the question they answer rather than by the tool that +//! triggered them. + +use super::tests::{plot_resource_id, request, run, set_request}; +use super::*; +use crate::properties::contour; +use crate::properties::tests::contour_app; + +#[test] +fn a_human_single_call_writes_no_run_manifest() { + let (mut app, _) = contour_app(); + let mut request = set_request(&app, contour::COUNT.as_str(), serde_json::json!(7)); + request.caller = CallerType::Human; + run(&mut app, request).expect("the write lands"); + assert!( + app.doc.automation_runs.is_empty(), + "an interactive edit is audited by document state and undo, not by a run record" + ); +} + +#[test] +fn an_agent_single_call_writes_a_run_manifest() { + let (mut app, _) = contour_app(); + let request = set_request(&app, contour::COUNT.as_str(), serde_json::json!(7)); + run(&mut app, request).expect("the write lands"); + assert_eq!(app.doc.automation_runs.len(), 1); + let manifest = &app.doc.automation_runs[0]; + assert_eq!(manifest.caller, CallerType::Agent); + assert_eq!(manifest.schema, RUN_MANIFEST_SCHEMA); + assert_eq!(manifest.nodes.len(), 1); + assert_eq!(manifest.nodes[0].tool_id, TOOL_SET); + assert!(!manifest.workflow_hash.is_empty()); + // The recorded workflow is a real one-node workflow, not a lookalike: it + // has to validate against the same registry a stored workflow does. + let workflow: WorkflowDefinition = + serde_json::from_value(manifest.workflow.clone()).expect("the manifest holds a workflow"); + workflow + .validate(&ToolRegistry::built_in()) + .expect("the synthesized workflow is valid"); + assert!(manifest.end_revision > manifest.start_revision); +} + +/// A read-only agent call changes nothing in the document and so produces no +/// `Action` — but it does append a run manifest, and a manifest is document +/// content. If appending one left the document clean, closing the window would +/// discard the audit record we promised to keep without ever offering to save. +#[test] +fn a_read_only_agent_call_leaves_the_document_unsaved() { + let (mut app, _) = contour_app(); + app.doc.dirty = false; + let before_revision = app.doc.automation_revision; + let inspect = request( + &app, + TOOL_INSPECT, + serde_json::json!({"key": contour::COUNT.as_str()}), + vec![plot_resource_id(&app)], + CallerType::Agent, + ); + run(&mut app, inspect).expect("inspect succeeds"); + assert_eq!(app.doc.automation_runs.len(), 1); + assert!( + app.doc.dirty, + "a run record is unsaved document content, even when the run read only" + ); + assert_eq!( + app.doc.automation_revision, before_revision, + "recording a run must not advance the optimistic-concurrency baseline" + ); +} + +/// The same for a workflow whose every node is read-only: it too writes one +/// manifest and executes no action. +#[test] +fn a_read_only_workflow_leaves_the_document_unsaved() { + let (mut app, _) = contour_app(); + app.doc.dirty = false; + let before_revision = app.doc.automation_revision; + let workflow = WorkflowDefinition { + schema: WORKFLOW_SCHEMA.to_owned(), + inputs: std::collections::BTreeMap::new(), + nodes: vec![WorkflowNode { + id: "blueprint".to_owned(), + tool_id: "project.get_blueprint".to_owned(), + tool_version: 1, + parameters: serde_json::json!({}), + targets: TargetSelector::Explicit { ids: Vec::new() }, + dependencies: Vec::new(), + bindings: Vec::new(), + condition: NodeCondition::Always, + failure_policy: NodeFailurePolicy::Inherit, + }], + failure_policy: WorkflowFailurePolicy::Strict, + }; + execute_workflow( + &mut app, + &workflow, + CallerType::Workflow, + ExecutionAuthority::Read, + &TaskCancellation::default(), + &mut |_| {}, + ) + .expect("the workflow runs"); + assert_eq!(app.doc.automation_runs.len(), 1); + assert!(app.doc.dirty, "the manifest is unsaved document content"); + assert_eq!(app.doc.automation_revision, before_revision); +} + +/// Marking the document unsaved is a statement about the file on disk, not +/// about the state a caller planned against. A plan held across an intervening +/// audit record must still commit. +#[test] +fn recording_a_run_does_not_invalidate_a_plan_in_flight() { + let (mut app, _) = contour_app(); + let plan = plan_tool( + &app, + set_request(&app, contour::COUNT.as_str(), serde_json::json!(8)), + ) + .expect("the write plans"); + let inspect = request( + &app, + TOOL_INSPECT, + serde_json::json!({"key": contour::COUNT.as_str()}), + vec![plot_resource_id(&app)], + CallerType::Agent, + ); + run(&mut app, inspect).expect("a read-only call lands its own run record"); + execute_tool(&mut app, plan, ExecutionAuthority::ReversibleModify) + .expect("the earlier plan is still current"); +} + +/// A workflow already records one manifest for the whole run; its nodes must +/// not each record a second one. +#[test] +fn a_workflow_node_does_not_also_record_a_single_call_manifest() { + let (mut app, _) = contour_app(); + let workflow = WorkflowDefinition { + schema: WORKFLOW_SCHEMA.to_owned(), + inputs: std::collections::BTreeMap::new(), + nodes: vec![WorkflowNode { + id: "set-count".to_owned(), + tool_id: TOOL_SET.to_owned(), + tool_version: 1, + parameters: serde_json::json!({"key": contour::COUNT.as_str(), "value": 6}), + targets: TargetSelector::Explicit { + ids: vec![plot_resource_id(&app)], + }, + dependencies: Vec::new(), + bindings: Vec::new(), + condition: NodeCondition::Always, + failure_policy: NodeFailurePolicy::Inherit, + }], + failure_policy: WorkflowFailurePolicy::Strict, + }; + let cancellation = TaskCancellation::default(); + execute_workflow( + &mut app, + &workflow, + CallerType::Workflow, + ExecutionAuthority::ReversibleModify, + &cancellation, + &mut |_| {}, + ) + .expect("the workflow runs"); + assert_eq!( + app.doc.automation_runs.len(), + 1, + "one run, one manifest — not one per node as well" + ); + assert_eq!(app.doc.automation_runs[0].nodes.len(), 1); + assert_eq!(app.doc.automation_runs[0].nodes[0].node_id, "set-count"); +} + +#[test] +fn an_external_write_records_an_operation_with_its_fingerprints() { + let (mut app, _) = contour_app(); + let directory = std::env::temp_dir().join(format!("plotx-export-{}", uuid::Uuid::new_v4())); + let before = app.session.operation_history.operations().count(); + let export = request( + &app, + "figure.export", + serde_json::json!({"directory": directory.display().to_string(), "format": "svg"}), + vec![app.doc.canvases[0].resource_id.to_string()], + CallerType::Agent, + ); + let result = run(&mut app, export).expect("the export runs"); + assert!( + result + .targets + .iter() + .any(|target| target.outcome == TargetOutcome::Succeeded) + ); + let records = app + .session + .operation_history + .operations() + .collect::>(); + assert_eq!( + records.len(), + before + 1, + "an external write leaves exactly one operation record" + ); + let record = records.last().expect("the record exists"); + assert_eq!(record.kind, crate::operation::OperationKind::Export); + assert!( + record.summary.contains("figure.export"), + "{}", + record.summary + ); + assert!(record.summary.contains("file(s)"), "{}", record.summary); + let text = record + .diagnostics + .iter() + .map(|diagnostic| diagnostic.sanitized_text()) + .collect::>() + .join("\n"); + assert!( + text.contains("sha256="), + "the fingerprint is recorded: {text}" + ); + let _ = std::fs::remove_dir_all(&directory); +} + +/// An export that fails partway must still account for the bytes it already +/// wrote. Returning early on the first failure would leave files on disk with +/// nothing in the operation history or the run manifest naming them, which is +/// the exact question the external-write audit exists to answer. +#[test] +fn a_partly_failed_export_still_records_what_it_wrote() { + let (mut app, _) = contour_app(); + app.doc.canvases.push(crate::state::CanvasDocument::new( + "blocked".to_owned(), + [120.0, 80.0], + )); + let directory = std::env::temp_dir().join(format!("plotx-export-{}", uuid::Uuid::new_v4())); + // A directory where the exporter must write a file: the write fails for this + // canvas alone, with the other canvas' output already committed. + std::fs::create_dir_all(directory.join("blocked.svg")).expect("the obstacle is created"); + let ok_canvas = app.doc.canvases[0].resource_id.to_string(); + let blocked_canvas = app.doc.canvases[1].resource_id.to_string(); + let export = request( + &app, + "figure.export", + serde_json::json!({ + "directory": directory.display().to_string(), + "format": "svg", + "overwrite": true, + }), + vec![ok_canvas, blocked_canvas], + CallerType::Agent, + ); + let result = run(&mut app, export).expect("a failed target is an outcome, not an abort"); + let succeeded = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Succeeded) + .collect::>(); + let failed = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Failed) + .collect::>(); + assert_eq!(succeeded.len(), 1, "{:?}", result.targets); + assert_eq!(failed.len(), 1, "{:?}", result.targets); + assert!( + !succeeded[0].fingerprints.is_empty(), + "the file that was written is fingerprinted" + ); + + let record = app + .session + .operation_history + .operations() + .last() + .expect("the external write is recorded even though one target failed"); + let text = record + .diagnostics + .iter() + .map(|diagnostic| diagnostic.sanitized_text()) + .collect::>() + .join("\n"); + assert!( + text.contains("sha256="), + "the bytes that did reach disk are named: {text}" + ); + assert!( + record + .diagnostics + .iter() + .any(|diagnostic| diagnostic.code == crate::operation::DiagnosticCode::ExportFailed), + "and the target that failed is named too: {text}" + ); + + let manifest = app + .doc + .automation_runs + .last() + .expect("the agent call left a manifest"); + assert!( + !manifest.errors.is_empty(), + "a manifest whose node failed a target says so, as a workflow's does" + ); + let recorded = &manifest.nodes[0].result.targets; + assert!( + recorded + .iter() + .any(|target| !target.fingerprints.is_empty()), + "and the manifest carries the fingerprints, not one generic error" + ); + let _ = std::fs::remove_dir_all(&directory); +} + +/// A tool that does not write outside the document leaves no operation record. +#[test] +fn a_document_only_tool_records_no_external_write() { + let (mut app, _) = contour_app(); + let before = app.session.operation_history.operations().count(); + let request = set_request(&app, contour::COUNT.as_str(), serde_json::json!(5)); + run(&mut app, request).expect("the write lands"); + assert_eq!(app.session.operation_history.operations().count(), before); +} diff --git a/crates/core/src/automation/properties_tests.rs b/crates/core/src/automation/properties_tests.rs new file mode 100644 index 0000000..bafce4a --- /dev/null +++ b/crates/core/src/automation/properties_tests.rs @@ -0,0 +1,489 @@ +//! The JSON boundary of the property catalog. +//! +//! Every test here targets the one way this stage can break: the adapter +//! growing its own copy of a planning or validation rule and drifting from the +//! planner the panel controls use. + +use super::*; +use crate::properties::tests::{contour_app, contour_spec}; +use crate::properties::{contour, definition_by_key}; +use crate::state::{ + CONTOUR_BASE_FRACTION_OF_RANGE, CONTOUR_BASE_NOISE_FLOOR, CanvasObject, CanvasObjectKind, + ObjectFrame, PlotxApp, SeriesBinding, TextBox, +}; + +pub(super) fn request( + app: &PlotxApp, + tool_id: &str, + parameters: serde_json::Value, + ids: Vec, + caller: CallerType, +) -> ToolRequest { + ToolRequest { + tool_id: tool_id.to_owned(), + tool_version: 1, + parameters, + targets: TargetSelector::Explicit { ids }, + expected_revision: DocumentRevision(app.doc.automation_revision), + caller, + } +} + +/// The stable id of the fixture's one plot object, as a selector names it. +pub(super) fn plot_resource_id(app: &PlotxApp) -> String { + let canvas = &app.doc.canvases[0]; + format!("{}/{}", canvas.resource_id, canvas.objects[0].id) +} + +pub(super) fn set_request(app: &PlotxApp, key: &str, value: serde_json::Value) -> ToolRequest { + request( + app, + TOOL_SET, + serde_json::json!({"key": key, "value": value}), + vec![plot_resource_id(app)], + CallerType::Agent, + ) +} + +/// Plan and execute in one step, the way a headless caller does. +pub(super) fn run(app: &mut PlotxApp, request: ToolRequest) -> Result { + let plan = plan_tool(app, request)?; + let authority = plan.required_authority; + execute_tool(app, plan, authority) +} + +/// Add a second series drawn as a line, so the object holds one series the +/// contour catalog applies to and one it does not. +fn add_line_series(app: &mut PlotxApp) { + let object = &mut app.doc.canvases[0].objects[0]; + let plot = object.plot_mut().expect("the fixture holds a plot"); + let mut series = plot.binding.series[0].clone(); + series.encoding = plotx_figure::SeriesEncoding::Line(plotx_figure::LineEncoding::default()); + let id = plot.allocate_series_id(); + let mut series = SeriesBinding { id, ..series }; + series.label = Some("line".to_owned()); + plot.binding.series.push(series); +} + +#[test] +fn an_unknown_property_key_is_refused_rather_than_skipped() { + let (mut app, _) = contour_app(); + let error = plan_tool( + &app, + set_request(&app, "series.contour.nonexistent", serde_json::json!(3)), + ) + .expect_err("an unknown key cannot be planned"); + let message = error.to_string(); + assert!( + message.contains("unknown property 'series.contour.nonexistent'"), + "{message}" + ); + // And it never reaches execution, so nothing is silently committed. + let before = app.doc.automation_revision; + let request = set_request(&app, "series.contour.nonexistent", serde_json::json!(3)); + assert!(run(&mut app, request).is_err()); + assert_eq!(app.doc.automation_revision, before); +} + +/// A value outside the declared bound must report both the value that was +/// rejected and the bound that rejected it. +#[test] +fn an_out_of_range_value_names_the_value_and_the_bound() { + let (app, _) = contour_app(); + let error = plan_tool( + &app, + set_request(&app, contour::RATIO.as_str(), serde_json::json!(50.0)), + ) + .expect_err("50 is above the declared ratio bound"); + let message = error.to_string(); + assert!( + message.contains("50"), + "the rejected value is named: {message}" + ); + assert!( + message.contains("greater than 1") && message.contains("at most 10"), + "the bound is named: {message}" + ); +} + +/// The bound a *context-dependent* schema imposes is enforced by the shared +/// planner, not by the adapter, and it too has to name both numbers. The +/// definition's static bound admits this value; only the target's current +/// anchor rejects it. +#[test] +fn a_bound_that_only_the_anchor_knows_still_names_the_value() { + let (mut app, target) = contour_app(); + assert!(matches!( + contour_spec(&app, &target).positive.base, + plotx_figure::ContourBasePolicy::NoiseFloor { .. } + )); + let request = set_request( + &app, + contour::BASE_MAGNITUDE.as_str(), + serde_json::json!(1.0e9), + ); + let error = + run(&mut app, request).expect_err("a multiplier of 1e9 is beyond what the anchor accepts"); + let message = error.to_string(); + assert!( + message.contains("1000000000"), + "the rejected value is named: {message}" + ); + assert!( + message.contains("10000"), + "the anchor's own bound is named: {message}" + ); +} + +/// A string that names no choice at all is a wire-format error, and it lists +/// the choices the setting has. +#[test] +fn an_unknown_enum_choice_lists_the_settings_options() { + let (app, _) = contour_app(); + let error = plan_tool( + &app, + set_request( + &app, + contour::BASE_POLICY.as_str(), + serde_json::json!("dark_magic"), + ), + ) + .expect_err("'dark_magic' is not a base policy"); + let message = error.to_string(); + assert!(message.contains("dark_magic"), "{message}"); + assert!( + message.contains(CONTOUR_BASE_NOISE_FLOOR) + && message.contains(CONTOUR_BASE_FRACTION_OF_RANGE), + "every declared choice is listed: {message}" + ); +} + +/// A choice the setting has but this field's capabilities withhold is refused +/// by the planner, and the refusal lists what the field does allow. The fixture +/// draws a signed plane, which is exactly the case where a fraction of the +/// value range is meaningless. +#[test] +fn a_capability_withheld_choice_lists_what_the_field_allows() { + let (mut app, _) = contour_app(); + let request = set_request( + &app, + contour::BASE_POLICY.as_str(), + serde_json::json!(CONTOUR_BASE_FRACTION_OF_RANGE), + ); + let error = + run(&mut app, request).expect_err("a signed field withholds the fraction-of-range anchor"); + let message = error.to_string(); + assert!( + message.contains(CONTOUR_BASE_FRACTION_OF_RANGE), + "{message}" + ); + assert!( + message.contains("this field allows") && message.contains(CONTOUR_BASE_NOISE_FLOOR), + "the permitted choices are named: {message}" + ); +} + +#[test] +fn a_value_of_the_wrong_shape_is_refused() { + let (app, _) = contour_app(); + let error = plan_tool( + &app, + set_request(&app, contour::COUNT.as_str(), serde_json::json!(true)), + ) + .expect_err("a count is not a boolean"); + assert!(error.to_string().contains("expected an integer"), "{error}"); +} + +/// A target the property does not apply to is reported with its reason, and the +/// one it does apply to still lands. +#[test] +fn a_skipped_component_is_reported_not_dropped() { + let (mut app, _) = contour_app(); + add_line_series(&mut app); + let request = set_request(&app, contour::COUNT.as_str(), serde_json::json!(9)); + let result = run(&mut app, request).expect("the contour series accepts the write"); + let succeeded = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Succeeded) + .collect::>(); + let skipped = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Skipped) + .collect::>(); + assert_eq!(succeeded.len(), 1, "{:?}", result.targets); + assert_eq!(skipped.len(), 1, "{:?}", result.targets); + assert!( + skipped[0].message.contains("line"), + "the skip names the encoding that caused it: {}", + skipped[0].message + ); + // The two rows must be distinguishable, or a results panel shows one plot + // object twice with nothing to tell the rows apart. + assert_ne!( + succeeded[0].target.describe(), + skipped[0].target.describe(), + "expanded targets carry their component" + ); + assert!(succeeded[0].target.describe().contains("series")); +} + +/// A validation failure leaves every target exactly as it was. A commit that +/// applied to the first series and then failed on the second would be a partial +/// landing, and the ladder of the first would silently disagree with the panel. +#[test] +fn a_validation_failure_lands_on_no_target_at_all() { + let (mut app, target) = contour_app(); + add_line_series(&mut app); + let before_revision = app.doc.automation_revision; + let before_spec = contour_spec(&app, &target); + let request = set_request(&app, contour::COUNT.as_str(), serde_json::json!(0)); + let error = run(&mut app, request).expect_err("a level count of zero is out of range"); + assert!(error.to_string().contains("out of range"), "{error}"); + assert_eq!( + app.doc.automation_revision, before_revision, + "a rejected write never advances the document" + ); + assert_eq!( + contour_spec(&app, &target), + before_spec, + "a rejected write never reaches a spec" + ); +} + +/// A canvas object with no plot binding is skipped by the shared declared +/// capability gate, with the shared reason, and needs no special case here. +#[test] +fn an_object_without_components_is_skipped_by_the_shared_gate() { + let (mut app, _) = contour_app(); + let canvas = &mut app.doc.canvases[0]; + let id = canvas.allocate_object_id(); + canvas.objects.push(CanvasObject { + id, + name: "Caption".to_owned(), + frame: ObjectFrame::new(0.0, 0.0, 20.0, 10.0), + locked: false, + visible: true, + group: None, + kind: CanvasObjectKind::Text(TextBox::label("hello".to_owned())), + }); + let text_id = format!("{}/{id}", app.doc.canvases[0].resource_id); + let plan = plan_tool( + &app, + request( + &app, + TOOL_INSPECT, + serde_json::json!({"key": contour::COUNT.as_str()}), + vec![text_id.clone()], + CallerType::Agent, + ), + ) + .expect("planning succeeds; the target is merely skipped"); + assert_eq!(plan.targets.len(), 1); + assert_eq!(plan.targets[0].status, TargetCompatibility::Skipped); + assert!( + plan.targets[0] + .reason + .contains("lacks a required kind or capability"), + "{}", + plan.targets[0].reason + ); + assert!(plan.targets[0].target.component.is_none()); +} + +#[test] +fn inspect_reads_the_value_and_reports_skips() { + let (mut app, _) = contour_app(); + add_line_series(&mut app); + let inspect = request( + &app, + TOOL_INSPECT, + serde_json::json!({"key": contour::COUNT.as_str()}), + vec![plot_resource_id(&app)], + CallerType::Agent, + ); + let result = run(&mut app, inspect).expect("inspect succeeds"); + assert_eq!(result.value["property"], contour::COUNT.as_str()); + assert_eq!(result.value["aggregate"]["state"], "uniform"); + assert_eq!(result.value["readings"].as_array().map(Vec::len), Some(1)); + assert_eq!(result.value["readings"][0]["schema"]["type"], "int"); + assert_eq!( + result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Skipped) + .count(), + 1, + "the line series is reported, not dropped" + ); +} + +/// A read-only tool must not be usable to write, and the refusal has to happen +/// before anything is planned. +#[test] +fn a_read_only_property_cannot_be_written() { + // Every catalog entry is currently writable, so this asserts the guard + // exists rather than exercising a read-only entry that does not yet exist. + assert!( + crate::properties::catalog() + .iter() + .all(|definition| definition.access == crate::properties::PropertyAccess::ReadWrite), + "add a read-only case here once the catalog has one" + ); +} + +// --------------------------------------------------------------------------- +// The pre-existing tools +// --------------------------------------------------------------------------- + +/// A syntactically valid relation plan, for the tools whose parameters carry +/// one. It never executes here — planning only has to decode it — so the ids it +/// names need not exist. +fn relation_plan() -> serde_json::Value { + let plan = plotx_data::RelPlanV1::new(plotx_data::Relation::SnapshotRead( + plotx_data::SnapshotRead { + table: plotx_data::TableId::new(), + revision: plotx_data::RevisionId::new(), + fingerprint: plotx_data::ContentHash::of(b"pre-existing-tool planning"), + }, + )); + serde_json::to_value(plan).expect("a relation plan serializes") +} + +/// The per-tool planning seam is additive. Every tool that existed before it +/// must still be planned by the shared gate alone: one planned target per frozen +/// resource, no component, and the shared reasons. +#[test] +fn the_planning_of_pre_existing_tools_is_unchanged() { + let (app, _) = contour_app(); + let canvas = app.doc.canvases[0].resource_id.to_string(); + let dataset = app.doc.datasets[0].resource_id().to_string(); + let object = plot_resource_id(&app); + let transform = serde_json::json!({ + "plan": relation_plan(), + "name": "Projected", + "memory_limit_bytes": 16 * 1024 * 1024, + }); + let cases: &[(&str, serde_json::Value, &str)] = &[ + ("project.get_blueprint", serde_json::json!({}), &canvas), + ( + "resources.search", + serde_json::json!({"query": {}}), + &canvas, + ), + ("resources.inspect", serde_json::json!({}), &object), + ("data.preview", serde_json::json!({}), &dataset), + ("render.preview", serde_json::json!({}), &canvas), + ("results.compare", serde_json::json!({}), &canvas), + ("resource.rename", serde_json::json!({"name": "x"}), &object), + ( + "figure.apply_theme", + serde_json::json!({"theme_id": "default"}), + &canvas, + ), + ( + "processing.apply_scheme", + serde_json::json!({"path": "scheme.plotxproc"}), + &dataset, + ), + ("data.import", serde_json::json!({"paths": []}), &canvas), + ("data.transform", transform, &dataset), + ( + "figure.export", + serde_json::json!({"directory": ".", "format": "svg"}), + &canvas, + ), + ]; + assert_eq!( + cases.len(), + ToolRegistry::built_in().descriptors().count() - 3, + "every tool that predates the three property tools is covered here" + ); + for (tool_id, parameters, target) in cases { + let plan = plan_tool( + &app, + request( + &app, + tool_id, + parameters.clone(), + vec![(*target).to_owned()], + CallerType::Agent, + ), + ) + .unwrap_or_else(|error| panic!("{tool_id} plans: {error}")); + assert_eq!(plan.targets.len(), 1, "{tool_id} expands nothing"); + assert!( + plan.targets[0].target.component.is_none(), + "{tool_id} names no component" + ); + assert_eq!(plan.targets[0].target.resource.id, **target); + assert!( + plan.targets[0] + .reason + .contains("declared kind and capabilities") + || plan.targets[0] + .reason + .contains("lacks a required kind or capability"), + "{tool_id} keeps the shared reason: {}", + plan.targets[0].reason + ); + } +} + +/// The three new tools are admitted by capability, and the descriptors say so +/// rather than naming an object type. +#[test] +fn the_property_tools_are_gated_by_capability() { + let registry = ToolRegistry::built_in(); + registry.validate_unique().expect("ids stay unique"); + for id in [TOOL_INSPECT, TOOL_SET, TOOL_RESET] { + let descriptor = registry.get(id).unwrap_or_else(|| panic!("{id} exists")); + assert_eq!( + descriptor.required_capabilities, + vec![CapabilityId::new(CAP_PROPERTY_CATALOG)], + "{id}" + ); + assert_eq!( + descriptor.target_kinds, + vec![ResourceKindId::new(KIND_CANVAS_OBJECT)], + "{id}" + ); + } + assert_eq!( + registry.get(TOOL_INSPECT).unwrap().effect, + EffectLevel::ReadOnly + ); + assert_eq!( + registry.get(TOOL_SET).unwrap().effect, + EffectLevel::Reversible + ); + assert_eq!( + registry.get(TOOL_RESET).unwrap().effect, + EffectLevel::Reversible + ); + assert!(registry.get(TOOL_SET).unwrap().undoable); +} + +/// Whole-encoding reset is deliberately not exposed: its scope needs the caller +/// to name an encoding kind, and a JSON caller that omits it would rebuild every +/// series of an object from defaults while naming only one of them. +#[test] +fn whole_encoding_reset_is_not_a_tool() { + assert!( + ToolRegistry::built_in() + .descriptors() + .all(|descriptor| descriptor.id != "properties.reset_encoding"), + ); +} + +#[test] +fn every_property_definition_is_reachable_by_key() { + for definition in crate::properties::catalog() { + assert!( + definition_by_key(definition.id.as_str()).is_some(), + "{} is not reachable by its own key", + definition.id + ); + } +} diff --git a/crates/core/src/automation/registry.rs b/crates/core/src/automation/registry.rs index 09ac9fa..d818ac9 100644 --- a/crates/core/src/automation/registry.rs +++ b/crates/core/src/automation/registry.rs @@ -60,6 +60,10 @@ pub(super) fn validate_parameters( "data.import" => parse::(tool, value).map(drop), "data.transform" => parse::(tool, value).map(drop), "figure.export" => parse::(tool, value).map(drop), + "properties.inspect" | "properties.reset" => { + parse::(tool, value).map(drop) + } + "properties.set" => parse::(tool, value).map(drop), _ => Err(AutomationError::UnknownTool(tool.to_owned())), } } @@ -164,6 +168,23 @@ schema!(SchemeParams, ["path" => "string"], ["compatible_only" => "boolean"]); schema!(ImportParams, ["paths" => "array"], []); schema!(TransformParams, ["plan" => "object", "name" => "string"], ["memory_limit_bytes" => "integer"]); schema!(ExportParams, ["directory" => "string", "format" => "string"], ["dpi" => "integer", "overwrite" => "boolean"]); +schema!(super::properties::PropertyKeyParams, ["key" => "string"], []); +// `value` has no single JSON type: which one is admissible is declared by the +// addressed property's own `ValueSchema` and is therefore decided per call, not +// per tool. Announcing one type here would be wrong for every other property. +impl V1Schema for super::properties::PropertyWriteParams { + fn schema() -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": { + "key": {"type": "string"}, + "value": {"description": "typed per the addressed property's value schema"}, + }, + "required": ["key", "value"], + "additionalProperties": false, + }) + } +} struct Spec { id: &'static str, @@ -304,6 +325,40 @@ fn descriptors() -> Vec { EffectLevel::ExternalWrite, true ), + // The property tools are admitted by capability, never by object type: + // any resource that exposes `CAP_PROPERTY_CATALOG` gains them, and a + // canvas object without a plot binding is skipped by the same shared + // gate that skips a dataset for `figure.apply_theme`. + tool!( + "properties.inspect", + "Inspect a property", + "Read one catalog property across the components of the selected resources", + super::properties::PropertyKeyParams, + [KIND_CANVAS_OBJECT], + [CAP_PROPERTY_CATALOG], + EffectLevel::ReadOnly, + true + ), + tool!( + "properties.set", + "Set a property", + "Write one catalog property through the same planner the panel controls use", + super::properties::PropertyWriteParams, + [KIND_CANVAS_OBJECT], + [CAP_PROPERTY_CATALOG], + EffectLevel::Reversible, + true + ), + tool!( + "properties.reset", + "Reset a property", + "Re-derive one catalog property from its default policy in each target's context", + super::properties::PropertyKeyParams, + [KIND_CANVAS_OBJECT], + [CAP_PROPERTY_CATALOG], + EffectLevel::Reversible, + true + ), ] .into_iter() .map(make_descriptor) diff --git a/crates/core/src/automation/resources.rs b/crates/core/src/automation/resources.rs index a5eb9e7..2012d3b 100644 --- a/crates/core/src/automation/resources.rs +++ b/crates/core/src/automation/resources.rs @@ -21,6 +21,13 @@ pub const CAP_EXPORT: &str = "figure.export"; pub const CAP_PREVIEW: &str = "data.preview"; pub const CAP_TRANSFORM: &str = "data.transform"; pub const CAP_PROCESSING_SCHEME: &str = "processing.scheme"; +/// The resource holds components the property catalog can address. It is the +/// admission gate for the `properties.*` tools, and it is a capability rather +/// than a resource-kind test on purpose: any future resource that grows +/// addressable components gains those tools by exposing this, and no encoding +/// or property registry ever grows a branch naming a data domain or an object +/// type (§1 principle 3). +pub const CAP_PROPERTY_CATALOG: &str = "properties.catalog"; pub const CAP_FIELD_SCALAR_GRID_2D_REGULAR: &str = "field.scalar_grid_2d.regular"; pub const CAP_FIELD_CURVE_1D: &str = "field.curve_1d"; pub const CAP_FIELD_COLORED_RASTER_2D: &str = "field.colored_raster_2d"; @@ -295,6 +302,15 @@ impl ResourceProvider for ProjectResourceProvider<'_> { let parent = self.canvas_descriptor(index); let canvas = &self.app.doc.canvases[index]; for object in &canvas.objects { + let mut capabilities = vec![cap(CAP_RENAME)]; + // A text box or an image has a name and nothing the catalog + // addresses; only an object with a plot binding carries + // components. Text objects are therefore skipped by the same + // declared-capability gate every other tool uses, with the same + // reason, and the property tools need no special case for them. + if object.plot().is_some() { + capabilities.push(cap(CAP_PROPERTY_CATALOG)); + } descriptors.push(ResourceDescriptor { resource: child_ref( canvas.resource_id, @@ -302,7 +318,7 @@ impl ResourceProvider for ProjectResourceProvider<'_> { KIND_CANVAS_OBJECT, ), name: object.name.clone(), - capabilities: vec![cap(CAP_RENAME)], + capabilities, children: Vec::new(), dimensions: Vec::new(), units: Vec::new(), diff --git a/crates/core/src/automation/tests.rs b/crates/core/src/automation/tests.rs index 4bc99f3..79f8328 100644 --- a/crates/core/src/automation/tests.rs +++ b/crates/core/src/automation/tests.rs @@ -1,7 +1,7 @@ use super::*; use crate::actions::Action; use crate::state::{CanvasDocument, Dataset, PlotxApp, TableDataset, TableSeriesBinding}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; fn app_with_table_and_canvas() -> PlotxApp { let mut app = PlotxApp::new(); @@ -228,6 +228,85 @@ fn data_transform_executes_the_same_persisted_relplan_and_is_undoable() { assert_eq!(manifest.table_revisions.len(), 2); } +/// The same transform, called directly by an agent rather than through a +/// workflow, has to leave the same provenance. A run that names neither the +/// revisions it read and wrote nor the plan and backend that produced them +/// cannot be reproduced from its own record, and the caller has no way to know +/// which of the two entry points it went through. +#[test] +fn an_agent_transform_records_its_table_provenance_without_a_workflow() { + let mut app = app_with_table_and_canvas(); + let source = app.doc.datasets[0].as_table().unwrap(); + let revision = &source.typed_state.envelope.revision; + let input_revision = revision.id; + let relplan = plotx_data::RelPlanV1::new(plotx_data::Relation::Project { + input: Box::new(plotx_data::Relation::SnapshotRead( + plotx_data::SnapshotRead { + table: revision.table_id, + revision: revision.id, + fingerprint: revision.snapshot.fingerprint, + }, + )), + columns: vec![source.series_bindings[0].value_column], + }); + let resource_id = source.resource_id; + let plan = plan_tool( + &app, + request( + "data.transform", + serde_json::json!({ + "plan": relplan.clone(), + "name": "Projected", + "memory_limit_bytes": 16 * 1024 * 1024, + }), + vec![resource_id.to_string()], + app.doc.automation_revision, + ), + ) + .unwrap(); + execute_tool(&mut app, plan, ExecutionAuthority::ReversibleModify).unwrap(); + + let manifest = app + .doc + .automation_runs + .last() + .expect("an agent call leaves a manifest"); + assert_eq!(manifest.table_plans.len(), 1); + assert_eq!( + manifest.table_plans[0].plan_fingerprint, + relplan.fingerprint().unwrap() + ); + assert_eq!( + manifest.table_plans[0].backend, + if cfg!(feature = "datafusion") { + "plotx.datafusion.v1" + } else { + "plotx.reference.v1" + } + ); + assert_eq!( + manifest.table_plans[0].input_revisions, + vec![input_revision] + ); + let roles = manifest + .table_revisions + .iter() + .map(|record| record.role.as_str()) + .collect::>(); + assert_eq!( + roles, + BTreeSet::from(["input", "output"]), + "both ends of the derivation are named: {:?}", + manifest.table_revisions + ); + assert!( + manifest + .table_revisions + .iter() + .any(|record| record.role == "input" && record.revision_id == input_revision) + ); +} + #[test] fn composite_validation_prevents_partial_application() { let mut app = app_with_table_and_canvas(); diff --git a/crates/core/src/automation/tool_executors.rs b/crates/core/src/automation/tool_executors.rs new file mode 100644 index 0000000..5b06f06 --- /dev/null +++ b/crates/core/src/automation/tool_executors.rs @@ -0,0 +1,658 @@ +//! The body of every registered tool. +//! +//! Planning, dispatch and audit live in `tools.rs`; this module holds what each +//! tool actually does once a plan has been approved. The split is by role, not +//! by tool: every executor here reaches the document through the same shared +//! commit helper, so "one tool, one atomic action" stays a property of the +//! module rather than of each function remembering to hold it. + +use super::registry::*; +use super::*; +use crate::actions::Action; +use crate::export::{ExportFormat, ExportPageScope, ExportSettings}; +use crate::project::{SchemeApplicationPolicy, load_scheme, plan_scheme_application}; +use crate::state::PlotxApp; +use crate::theme::Theme; +use sha2::{Digest, Sha256}; +use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; + +pub(super) fn execute_search( + app: &PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: SearchParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + let found = search_resources(&ProjectResourceProvider::new(app), ¶ms.query); + serde_result( + &plan.request.tool_id, + DocumentRevision(app.doc.automation_revision), + found, + ) +} + +pub(super) fn execute_inspect( + app: &PlotxApp, + plan: &ToolPlan, +) -> Result { + let provider = ProjectResourceProvider::new(app); + let values = compatible_targets(plan) + .filter_map(|target| provider.inspect(&target.id)) + .collect::>(); + serde_result(&plan.request.tool_id, provider.revision(), values) +} + +pub(super) fn execute_data_preview( + app: &PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: PreviewParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + let provider = ProjectResourceProvider::new(app); + let values = compatible_targets(plan) + .map(|target| provider.preview(target, params.limit)) + .collect::, _>>()?; + serde_result(&plan.request.tool_id, provider.revision(), values) +} + +pub(super) fn execute_render_preview( + app: &PlotxApp, + plan: &ToolPlan, +) -> Result { + let provider = ProjectResourceProvider::new(app); + let values = compatible_targets(plan) + .map(|target| { + provider + .render_preview(target) + .map(|svg| serde_json::json!({"resource_id": target.id, "svg": svg})) + }) + .collect::, _>>()?; + serde_result(&plan.request.tool_id, provider.revision(), values) +} + +pub(super) fn execute_compare( + app: &PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: CompareParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + let provider = ProjectResourceProvider::new(app); + let values = compatible_targets(plan) + .filter_map(|target| provider.inspect(&target.id)) + .map(|current| { + let before = params + .before + .iter() + .find(|item| item.resource.id == current.resource.id); + ResourceComparison { + resource_id: current.resource.id.clone(), + changed: before != Some(¤t), + fields: BTreeMap::from([ + ( + "before".to_owned(), + serde_json::to_value(before).unwrap_or_default(), + ), + ( + "after".to_owned(), + serde_json::to_value(current).unwrap_or_default(), + ), + ]), + } + }) + .collect::>(); + serde_result(&plan.request.tool_id, provider.revision(), values) +} + +pub(super) fn execute_rename( + app: &mut PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: RenameParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + if params.name.trim().is_empty() { + return Err(AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: "name must not be empty".to_owned(), + }); + } + let mut actions = Vec::new(); + for target in compatible_targets(plan) { + if let Some(index) = dataset_index(app, &target.id) { + actions.push(Action::rename_dataset( + app.doc.datasets[index].resource_id(), + app.doc.datasets[index].name(), + Some(params.name.clone()), + )); + } else if let Some(index) = canvas_index(app, &target.id) { + actions.push(Action::rename_canvas( + index, + app.doc.canvases[index].name.clone(), + params.name.clone(), + )); + } else if target.kind.0 == KIND_CANVAS_OBJECT + && let (Some(parent), Some(local)) = (&target.parent_id, &target.local_id) + && let Some(canvas) = canvas_index(app, parent) + && let Ok(object_id) = local.parse::() + && let Some(object) = app.doc.canvases[canvas].object(object_id) + { + actions.push(Action::rename_object( + canvas, + object_id, + object.name.clone(), + params.name.clone(), + )); + } + } + commit_actions(app, plan, actions, "renamed") +} + +pub(super) fn execute_theme( + app: &mut PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: ThemeParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + let theme = + Theme::by_id(¶ms.theme_id).ok_or_else(|| AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: format!("unknown theme '{}'", params.theme_id), + })?; + let actions = compatible_targets(plan) + .filter_map(|target| canvas_index(app, &target.id)) + .filter_map(|canvas| app.theme_action(canvas, &theme)) + .collect(); + commit_actions(app, plan, actions, "theme applied") +} + +pub(super) fn execute_scheme( + app: &mut PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: SchemeParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + let scheme = + load_scheme(¶ms.path).map_err(|error| AutomationError::Execution(error.to_string()))?; + let indices = compatible_targets(plan) + .filter_map(|target| dataset_index(app, &target.id)) + .collect::>(); + let scheme_plan = plan_scheme_application(&scheme, &app.doc.datasets, &indices); + let policy = if params.compatible_only { + SchemeApplicationPolicy::CompatibleOnly + } else { + SchemeApplicationPolicy::StrictAll + }; + let prepared = scheme_plan.prepare(policy).ok_or_else(|| { + AutomationError::Execution( + "no compatible processing targets, or strict policy rejected the selection".to_owned(), + ) + })?; + let applied_ids = prepared + .applied_targets + .iter() + .filter_map(|index| app.doc.datasets.get(*index)) + .map(|dataset| dataset.resource_id().to_string()) + .collect::>(); + let skipped = scheme_plan + .targets() + .iter() + .filter_map(|target| { + target.result.incompatibility_reason().map(|reason| { + let id = app + .doc + .datasets + .get(target.dataset) + .map(|dataset| dataset.resource_id().to_string()) + .unwrap_or_else(|| format!("stale-{}", target.dataset)); + (id, reason.to_owned()) + }) + }) + .collect::>(); + let mut result = commit_actions( + app, + plan, + vec![prepared.action], + "processing scheme applied", + )?; + result + .modified + .retain(|target| applied_ids.contains(&target.id)); + for target in &mut result.targets { + if let Some(reason) = skipped.get(&target.target.resource.id) { + target.outcome = TargetOutcome::Skipped; + target.message.clone_from(reason); + } + } + if let Some(fingerprint) = fingerprint_file(¶ms.path, "processing_scheme") { + result.diagnostics.push(format!( + "processing scheme sha256 {} ({})", + fingerprint.sha256, + fingerprint.path.display() + )); + } + Ok(result) +} + +pub(super) fn execute_import( + app: &mut PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: ImportParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + if params.paths.is_empty() { + return Err(AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: "paths must contain at least one input".to_owned(), + }); + } + let base_dataset = app.doc.datasets.len(); + let base_canvas = app.doc.canvases.len(); + let mut actions = Vec::new(); + let mut produced = Vec::new(); + let mut item_results = Vec::new(); + for path in ¶ms.paths { + let external = TargetRef::resource(ResourceRef { + id: path.display().to_string(), + kind: ResourceKindId::new(KIND_EXTERNAL_INPUT), + parent_id: None, + local_id: None, + }); + let loaded = match crate::workflow::load_dataset(path) { + Ok(loaded) => loaded, + Err(error) => { + item_results.push(TargetResult { + target: external, + outcome: TargetOutcome::Failed, + message: error.to_string(), + fingerprints: fingerprint_file(path, "selected_input") + .into_iter() + .collect(), + }); + continue; + } + }; + let fingerprints = input_fingerprints(&loaded.inspection); + produced.push(ResourceRef { + id: loaded.dataset.resource_id().to_string(), + kind: ResourceKindId::new(KIND_DATASET), + parent_id: None, + local_id: None, + }); + let offset = actions.len(); + actions.push(Action::InsertDatasetWithCanvas { + dataset_index: base_dataset + offset, + canvas_index: base_canvas + offset, + canvas_resource_id: crate::state::CanvasId::new(), + dataset: Box::new(loaded.dataset), + canvas_name: path + .file_stem() + .map(|value| value.to_string_lossy().into_owned()) + .unwrap_or_else(|| "Imported Data".to_owned()), + size_mm: crate::state::DEFAULT_CANVAS_SIZE_MM, + active_canvas_before: app.session.active_canvas, + active_dataset_before: app.active_dataset(), + inserted_into_existing_canvas: None, + inserted_object_id: None, + }); + item_results.push(TargetResult { + target: external, + outcome: TargetOutcome::Succeeded, + message: "imported into the canonical PlotX data model".to_owned(), + fingerprints, + }); + } + if actions.is_empty() { + return Ok(ToolResult { + tool_id: plan.request.tool_id.clone(), + before_revision: DocumentRevision(app.doc.automation_revision), + after_revision: DocumentRevision(app.doc.automation_revision), + targets: item_results, + produced: Vec::new(), + modified: Vec::new(), + diagnostics: Vec::new(), + verification: Vec::new(), + value: serde_json::Value::Null, + }); + } + let mut result = commit_actions(app, plan, actions, "imported")?; + produced.extend( + app.doc + .canvases + .iter() + .skip(base_canvas) + .map(|canvas| ResourceRef { + id: canvas.resource_id.to_string(), + kind: ResourceKindId::new(KIND_CANVAS), + parent_id: None, + local_id: None, + }), + ); + result.produced = produced; + result.targets.extend(item_results); + Ok(result) +} + +pub(super) fn execute_transform( + app: &mut PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: TransformParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + if params.name.trim().is_empty() { + return Err(AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: "name must not be empty".into(), + }); + } + if params.memory_limit_bytes == 0 { + return Err(AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: "memory_limit_bytes must be positive".into(), + }); + } + let input_datasets = compatible_targets(plan) + .map(|target| { + dataset_index(app, &target.id).ok_or_else(|| { + AutomationError::Execution(format!("table resource {} disappeared", target.id)) + }) + }) + .collect::, _>>()?; + if input_datasets.is_empty() { + return Err(AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: "select at least one typed table input".into(), + }); + } + let index = app + .derive_table_from_plan( + params.plan, + &input_datasets, + params.name, + params.memory_limit_bytes, + ) + .map_err(AutomationError::Execution)?; + let output = app.doc.datasets[index].as_table().ok_or_else(|| { + AutomationError::Execution("transform did not produce a typed table".into()) + })?; + let produced = ResourceRef { + id: output.resource_id.to_string(), + kind: ResourceKindId::new(KIND_DATASET), + parent_id: None, + local_id: None, + }; + Ok(ToolResult { + tool_id: plan.request.tool_id.clone(), + before_revision: DocumentRevision(app.doc.automation_revision), + after_revision: DocumentRevision(app.doc.automation_revision), + targets: compatible_target_refs(plan) + .cloned() + .map(|target| TargetResult { + target, + outcome: TargetOutcome::Succeeded, + message: "executed as a pinned PlotX RelPlanV1 input".into(), + fingerprints: Vec::new(), + }) + .collect(), + produced: vec![produced], + modified: Vec::new(), + diagnostics: output + .typed_state + .envelope + .revision + .operation + .diagnostics + .iter() + .map(|diagnostic| diagnostic.message.clone()) + .collect(), + verification: Vec::new(), + value: serde_json::json!({ + "table_id": output.typed_state.envelope.revision.table_id, + "revision_id": output.typed_state.envelope.revision.id, + "fingerprint": output.typed_state.envelope.revision.snapshot.fingerprint, + }), + }) +} + +pub(super) fn execute_export( + app: &PlotxApp, + plan: &ToolPlan, +) -> Result { + let params: ExportParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; + let format = + parse_export_format(¶ms.format).ok_or_else(|| AutomationError::InvalidParameters { + tool_id: plan.request.tool_id.clone(), + message: format!("unsupported export format '{}'", params.format), + })?; + std::fs::create_dir_all(¶ms.directory).map_err(|source| AutomationError::Io { + path: params.directory.clone(), + source, + })?; + let mut outputs = Vec::new(); + let mut targets = plan + .targets + .iter() + .filter(|target| target.status != TargetCompatibility::Compatible) + .map(|target| TargetResult { + target: target.target.clone(), + outcome: TargetOutcome::Skipped, + message: target.reason.clone(), + fingerprints: Vec::new(), + }) + .collect::>(); + for target in compatible_targets(plan) { + let Some(index) = canvas_index(app, &target.id) else { + continue; + }; + let canvas = &app.doc.canvases[index]; + let safe_name = sanitize_file_name(&canvas.name); + let base = params.directory.join(safe_name); + let expected = crate::export::export_output_paths(&base, format, 1); + if !params.overwrite && expected.iter().any(|path| path.exists()) { + targets.push(TargetResult { + target: TargetRef::resource(target.clone()), + outcome: TargetOutcome::Failed, + message: "output already exists and overwrite is false".to_owned(), + fingerprints: expected + .iter() + .filter_map(|path| fingerprint_file(path, "existing_output")) + .collect(), + }); + continue; + } + let written = match crate::export::export_canvases( + std::slice::from_ref(canvas), + Some(0), + &ExportSettings { + format, + scope: ExportPageScope::Current, + dpi: params.dpi, + target_width_mm: None, + trim_to_visible_content: false, + }, + &base, + ) { + Ok(written) => written, + // One canvas failing is a per-target outcome, exactly like the + // overwrite collision above — not a reason to abandon the run. An + // early `?` here would return before the caller's audit record is + // written, so files already on disk would exist with nothing in the + // operation history or the run manifest naming them. The failure is + // not swallowed: `TargetOutcome::Failed` is what a workflow reads to + // abort, and it reaches the manifest's errors either way. + Err(error) => { + targets.push(TargetResult { + target: TargetRef::resource(target.clone()), + outcome: TargetOutcome::Failed, + message: error.to_string(), + fingerprints: Vec::new(), + }); + continue; + } + }; + outputs.extend(written.iter().map(|path| path.display().to_string())); + targets.push(TargetResult { + target: TargetRef::resource(target.clone()), + outcome: TargetOutcome::Succeeded, + message: written + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(", "), + fingerprints: written + .iter() + .filter_map(|path| fingerprint_file(path, "output")) + .collect(), + }); + } + Ok(ToolResult { + tool_id: plan.request.tool_id.clone(), + before_revision: DocumentRevision(app.doc.automation_revision), + after_revision: DocumentRevision(app.doc.automation_revision), + targets, + produced: Vec::new(), + modified: Vec::new(), + diagnostics: outputs, + verification: vec![VerificationRecord { + check: "outputs_exist".to_owned(), + passed: true, + message: "all successful exports were committed by the PlotX exporter".to_owned(), + }], + value: serde_json::Value::Null, + }) +} + +fn commit_actions( + app: &mut PlotxApp, + plan: &ToolPlan, + actions: Vec, + verb: &str, +) -> Result { + if actions.is_empty() { + return Err(AutomationError::Execution( + "tool had no compatible targets".to_owned(), + )); + } + app.try_execute_action(Action::Composite(actions)) + .map_err(|error| AutomationError::Execution(error.to_string()))?; + let modified = compatible_targets(plan).cloned().collect::>(); + Ok(ToolResult { + tool_id: plan.request.tool_id.clone(), + before_revision: plan.frozen_targets.revision, + after_revision: DocumentRevision(app.doc.automation_revision), + targets: plan + .targets + .iter() + .map(|target| TargetResult { + target: target.target.clone(), + outcome: if target.status == TargetCompatibility::Compatible { + TargetOutcome::Succeeded + } else { + TargetOutcome::Skipped + }, + message: if target.status == TargetCompatibility::Compatible { + verb.to_owned() + } else { + target.reason.clone() + }, + fingerprints: Vec::new(), + }) + .collect(), + produced: Vec::new(), + modified, + diagnostics: Vec::new(), + verification: vec![VerificationRecord { + check: "revision_advanced".to_owned(), + passed: app.doc.automation_revision > plan.frozen_targets.revision.0, + message: "atomic document commit completed".to_owned(), + }], + value: serde_json::Value::Null, + }) +} + +fn dataset_index(app: &PlotxApp, id: &str) -> Option { + app.doc + .datasets + .iter() + .position(|dataset| dataset.resource_id().to_string() == id) +} + +fn canvas_index(app: &PlotxApp, id: &str) -> Option { + app.doc + .canvases + .iter() + .position(|canvas| canvas.resource_id.to_string() == id) +} + +pub(super) fn serde_result( + tool_id: &str, + revision: DocumentRevision, + value: impl serde::Serialize, +) -> Result { + Ok(readonly_value( + tool_id, + revision, + serde_json::to_value(value) + .map_err(|error| AutomationError::Execution(error.to_string()))?, + )) +} + +pub(super) fn readonly_value( + tool_id: &str, + revision: DocumentRevision, + value: serde_json::Value, +) -> ToolResult { + ToolResult { + tool_id: tool_id.to_owned(), + before_revision: revision, + after_revision: revision, + targets: Vec::new(), + produced: Vec::new(), + modified: Vec::new(), + diagnostics: Vec::new(), + verification: Vec::new(), + value, + } +} + +fn parse_export_format(value: &str) -> Option { + match value.to_ascii_lowercase().as_str() { + "svg" => Some(ExportFormat::Svg), + "pdf" => Some(ExportFormat::Pdf), + "png" => Some(ExportFormat::Png), + "tiff" | "tif" => Some(ExportFormat::Tiff), + "jpeg" | "jpg" => Some(ExportFormat::Jpeg), + _ => None, + } +} + +fn sanitize_file_name(value: &str) -> String { + let value = value + .chars() + .map(|ch| if "<>:\"/\\|?*".contains(ch) { '_' } else { ch }) + .collect::(); + let value = value.trim().trim_end_matches(['.', ' ']); + if value.is_empty() { + "figure".to_owned() + } else { + value.to_owned() + } +} + +fn input_fingerprints(inspection: &crate::workflow::InspectionReport) -> Vec { + let mut paths = vec![("selected_input", &inspection.provenance.selected_path)]; + paths.push(("data", &inspection.provenance.data_path)); + paths.extend( + inspection + .provenance + .parameter_paths + .iter() + .map(|path| ("parameter", path)), + ); + let mut seen = BTreeSet::new(); + paths + .into_iter() + .filter(|(_, path)| seen.insert((*path).clone())) + .filter_map(|(role, path)| fingerprint_file(path, role)) + .collect() +} + +fn fingerprint_file(path: &Path, role: &str) -> Option { + let bytes = std::fs::read(path).ok()?; + Some(FingerprintRecord { + role: role.to_owned(), + path: path.to_owned(), + sha256: format!("{:x}", Sha256::digest(&bytes)), + bytes: bytes.len() as u64, + }) +} diff --git a/crates/core/src/automation/tools.rs b/crates/core/src/automation/tools.rs index 6c8fcff..0dd741e 100644 --- a/crates/core/src/automation/tools.rs +++ b/crates/core/src/automation/tools.rs @@ -1,14 +1,17 @@ +//! Planning, dispatch and audit for the registered tools. +//! +//! What each tool *does* lives in [`super::tool_executors`]; this module decides +//! which targets a tool may touch, routes an approved plan to its executor, and +//! records the evidence a run leaves behind. + use super::*; -use crate::actions::Action; -use crate::export::{ExportFormat, ExportPageScope, ExportSettings}; -use crate::project::{SchemeApplicationPolicy, load_scheme, plan_scheme_application}; -use crate::state::PlotxApp; -use crate::theme::Theme; +use crate::state::{Document, PlotxApp}; use sha2::{Digest, Sha256}; -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use std::path::Path; use super::registry::*; +use super::tool_executors::*; pub fn plan_tool(app: &PlotxApp, request: ToolRequest) -> Result { let registry = ToolRegistry::built_in(); @@ -30,7 +33,39 @@ pub fn plan_tool(app: &PlotxApp, request: ToolRequest) -> Result Vec::new(), + _ => vec![format!("{} compatible resource(s)", compatible)], + }, + outputs: tool_outputs(&descriptor.id), + required_authority: descriptor.effect.required_authority(), + }) +} + +/// The declared kind/capability gate every tool shares. +fn declared_targets( + provider: &ProjectResourceProvider<'_>, + descriptor: &ToolDescriptor, + frozen_targets: &FrozenTargetSet, +) -> Vec { + frozen_targets .targets .iter() .map(|target| { @@ -49,7 +84,7 @@ pub fn plan_tool(app: &PlotxApp, request: ToolRequest) -> Result Result>(); - let compatible = targets - .iter() - .filter(|target| target.status == TargetCompatibility::Compatible) - .count(); - Ok(ToolPlan { - request, + .collect() +} + +/// Run one tool and audit the call. +/// +/// A single tool invocation by a workflow or an agent *is* a one-node workflow: +/// it has the same inputs, produces the same node record and needs the same +/// reproducibility evidence. Synthesizing that node here rather than inventing a +/// second audit shape means the manifest an agent leaves behind is byte-for-byte +/// the manifest a one-node workflow leaves behind, and every reader of +/// `automation_runs` keeps working unchanged (§9, principle 5). +/// +/// A `Human` call is untouched: an interactive edit is audited by the document's +/// own final state plus undo, and writing a run manifest for every button press +/// would bury the runs that describe automation. +pub fn execute_tool( + app: &mut PlotxApp, + plan: ToolPlan, + authority: ExecutionAuthority, +) -> Result { + let caller = plan.request.caller; + if caller == CallerType::Human { + return execute_planned_tool(app, plan, authority); + } + let node = single_node_workflow(&plan.request); + let frozen_targets = plan.frozen_targets.clone(); + let parameters = plan.request.parameters.clone(); + let tool_id = plan.request.tool_id.clone(); + let tool_version = plan.request.tool_version; + let start_revision = DocumentRevision(app.doc.automation_revision); + // The same snapshot `execute_workflow` takes before its first node: the + // input revisions a derived table was built from cannot be recovered once + // the derivation has happened. + let start_table_revisions = typed_table_revisions(app, "input"); + let started_unix_ms = unix_ms(); + let started = std::time::Instant::now(); + let outcome = execute_planned_tool(app, plan, authority); + let end_revision = DocumentRevision(app.doc.automation_revision); + let (result, errors) = match &outcome { + Ok(result) => { + // A tool that reports a per-target failure without failing outright + // is recorded the way `execute_workflow` records the same node, so + // the two manifests stay one shape (§9, principle 5). + let errors = if result + .targets + .iter() + .any(|target| target.outcome == TargetOutcome::Failed) + { + vec![format!("{SINGLE_CALL_NODE}: one or more targets failed")] + } else { + Vec::new() + }; + (result.clone(), errors) + } + Err(error) => ( + ToolResult { + tool_id: tool_id.clone(), + before_revision: start_revision, + after_revision: end_revision, + targets: Vec::new(), + produced: Vec::new(), + modified: Vec::new(), + diagnostics: vec![error.to_string()], + verification: Vec::new(), + value: serde_json::Value::Null, + }, + vec![error.to_string()], + ), + }; + let record = NodeRunRecord { + node_id: SINGLE_CALL_NODE.to_owned(), + tool_id: tool_id.clone(), + parameters, frozen_targets, - targets, - estimated_changes: match descriptor.effect { - EffectLevel::ReadOnly => Vec::new(), - _ => vec![format!("{} compatible resource(s)", compatible)], + result, + duration_ms: started.elapsed().as_millis(), + }; + let workflow = serde_json::to_value(&node) + .map_err(|error| AutomationError::Execution(error.to_string()))?; + let canonical = serde_json::to_vec(&workflow) + .map_err(|error| AutomationError::Execution(error.to_string()))?; + // Reuse the workflow executor's extraction rather than a second copy: a + // `data.transform` called directly by an agent has to leave the same input + // and output table revisions, plan fingerprint and backend identity that the + // same transform leaves when a workflow runs it, or the run is not + // reproducible from its own record. + let records = vec![record]; + let (table_revisions, table_plans) = table_run_records(app, &records, &start_table_revisions); + record_run( + &mut app.doc, + RunManifest { + schema: RUN_MANIFEST_SCHEMA.to_owned(), + run_id: uuid::Uuid::new_v4().to_string(), + caller, + workflow_hash: format!("{:x}", Sha256::digest(&canonical)), + workflow, + application_version: env!("CARGO_PKG_VERSION").to_owned(), + tool_versions: BTreeMap::from([(tool_id, tool_version)]), + start_revision, + end_revision, + started_unix_ms, + finished_unix_ms: unix_ms(), + cancelled: false, + nodes: records, + warnings: Vec::new(), + errors, + verification: Vec::new(), + table_revisions, + table_plans, }, - outputs: tool_outputs(&descriptor.id), - required_authority: descriptor.effect.required_authority(), - }) + ); + outcome } -pub fn execute_tool( +/// The one place a finished run is appended to the document. +/// +/// A manifest *is* document content, so appending one makes the document +/// unsaved. Nothing else establishes that: a run made entirely of read-only +/// tools executes no [`Action`](crate::actions::Action) at all, so without this +/// the audit record we promised to keep would be dropped when the window closes +/// without ever offering to save it. Both writers go through here so the next +/// one cannot forget. +/// +/// `automation_revision` is deliberately untouched. It is the baseline of the +/// optimistic-concurrency check, and advancing it here would invalidate the plan +/// the caller is still holding — dirtiness says "the file on disk is stale", +/// which is a different question from "has the document the caller planned +/// against moved". +pub(super) fn record_run(document: &mut Document, manifest: RunManifest) { + document.automation_runs.push(manifest); + document.dirty = true; +} + +/// The node id a single call is recorded under. +pub const SINGLE_CALL_NODE: &str = "single-call"; + +/// The one-node workflow a single tool call is equivalent to. +fn single_node_workflow(request: &ToolRequest) -> WorkflowDefinition { + WorkflowDefinition { + schema: WORKFLOW_SCHEMA.to_owned(), + inputs: BTreeMap::new(), + nodes: vec![WorkflowNode { + id: SINGLE_CALL_NODE.to_owned(), + tool_id: request.tool_id.clone(), + tool_version: request.tool_version, + parameters: request.parameters.clone(), + targets: request.targets.clone(), + dependencies: Vec::new(), + bindings: Vec::new(), + condition: NodeCondition::Always, + failure_policy: NodeFailurePolicy::Inherit, + }], + failure_policy: WorkflowFailurePolicy::Strict, + } +} + +use super::workflow::{table_run_records, typed_table_revisions, unix_ms}; + +/// Execute a plan without the single-call audit wrapper. +/// +/// The workflow executor calls this: a run already writes one manifest for the +/// whole graph, and nesting a per-node manifest inside it would double-record +/// every node. +pub(super) fn execute_planned_tool( app: &mut PlotxApp, plan: ToolPlan, authority: ExecutionAuthority, @@ -117,610 +297,129 @@ pub fn execute_tool( "data.import" => execute_import(app, &plan)?, "data.transform" => execute_transform(app, &plan)?, "figure.export" => execute_export(app, &plan)?, + id if properties::is_property_tool(id) => { + let targets = compatible_target_refs(&plan).cloned().collect(); + properties::execute(app, &plan, targets)? + } _ => return Err(AutomationError::UnknownTool(tool_id)), }; result.before_revision = before; result.after_revision = DocumentRevision(app.doc.automation_revision); + record_external_write(app, &plan, &result); Ok(result) } -fn execute_search(app: &PlotxApp, plan: &ToolPlan) -> Result { - let params: SearchParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - let found = search_resources(&ProjectResourceProvider::new(app), ¶ms.query); - serde_result( - &plan.request.tool_id, - DocumentRevision(app.doc.automation_revision), - found, - ) -} - -fn execute_inspect(app: &PlotxApp, plan: &ToolPlan) -> Result { - let provider = ProjectResourceProvider::new(app); - let values = compatible_targets(plan) - .filter_map(|target| provider.inspect(&target.id)) - .collect::>(); - serde_result(&plan.request.tool_id, provider.revision(), values) -} - -fn execute_data_preview(app: &PlotxApp, plan: &ToolPlan) -> Result { - let params: PreviewParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - let provider = ProjectResourceProvider::new(app); - let values = compatible_targets(plan) - .map(|target| provider.preview(target, params.limit)) - .collect::, _>>()?; - serde_result(&plan.request.tool_id, provider.revision(), values) -} - -fn execute_render_preview(app: &PlotxApp, plan: &ToolPlan) -> Result { - let provider = ProjectResourceProvider::new(app); - let values = compatible_targets(plan) - .map(|target| { - provider - .render_preview(target) - .map(|svg| serde_json::json!({"resource_id": target.id, "svg": svg})) - }) - .collect::, _>>()?; - serde_result(&plan.request.tool_id, provider.revision(), values) -} - -fn execute_compare(app: &PlotxApp, plan: &ToolPlan) -> Result { - let params: CompareParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - let provider = ProjectResourceProvider::new(app); - let values = compatible_targets(plan) - .filter_map(|target| provider.inspect(&target.id)) - .map(|current| { - let before = params - .before - .iter() - .find(|item| item.resource.id == current.resource.id); - ResourceComparison { - resource_id: current.resource.id.clone(), - changed: before != Some(¤t), - fields: BTreeMap::from([ - ( - "before".to_owned(), - serde_json::to_value(before).unwrap_or_default(), - ), - ( - "after".to_owned(), - serde_json::to_value(current).unwrap_or_default(), - ), - ]), - } - }) - .collect::>(); - serde_result(&plan.request.tool_id, provider.revision(), values) -} - -fn execute_rename(app: &mut PlotxApp, plan: &ToolPlan) -> Result { - let params: RenameParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - if params.name.trim().is_empty() { - return Err(AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: "name must not be empty".to_owned(), - }); +/// Record an operation for any tool that wrote outside the document. +/// +/// The test is the declared [`EffectLevel::ExternalWrite`], never a tool id: a +/// file that left the application is the thing worth recording, and every future +/// tool that writes one inherits this audit without touching this function. +/// The record names what was written and carries the fingerprints the tool +/// already computed, so "which bytes did this run produce" is answerable from +/// the operation history alone. +fn record_external_write(app: &mut PlotxApp, plan: &ToolPlan, result: &ToolResult) { + let external = ToolRegistry::built_in() + .get(&plan.request.tool_id) + .is_some_and(|descriptor| descriptor.effect == EffectLevel::ExternalWrite); + if !external { + return; } - let mut actions = Vec::new(); - for target in compatible_targets(plan) { - if let Some(index) = dataset_index(app, &target.id) { - actions.push(Action::rename_dataset( - app.doc.datasets[index].resource_id(), - app.doc.datasets[index].name(), - Some(params.name.clone()), - )); - } else if let Some(index) = canvas_index(app, &target.id) { - actions.push(Action::rename_canvas( - index, - app.doc.canvases[index].name.clone(), - params.name.clone(), - )); - } else if target.kind.0 == KIND_CANVAS_OBJECT - && let (Some(parent), Some(local)) = (&target.parent_id, &target.local_id) - && let Some(canvas) = canvas_index(app, parent) - && let Ok(object_id) = local.parse::() - && let Some(object) = app.doc.canvases[canvas].object(object_id) - { - actions.push(Action::rename_object( - canvas, - object_id, - object.name.clone(), - params.name.clone(), - )); - } - } - commit_actions(app, plan, actions, "renamed") -} - -fn execute_theme(app: &mut PlotxApp, plan: &ToolPlan) -> Result { - let params: ThemeParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - let theme = - Theme::by_id(¶ms.theme_id).ok_or_else(|| AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: format!("unknown theme '{}'", params.theme_id), - })?; - let actions = compatible_targets(plan) - .filter_map(|target| canvas_index(app, &target.id)) - .filter_map(|canvas| app.theme_action(canvas, &theme)) - .collect(); - commit_actions(app, plan, actions, "theme applied") -} - -fn execute_scheme(app: &mut PlotxApp, plan: &ToolPlan) -> Result { - let params: SchemeParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - let scheme = - load_scheme(¶ms.path).map_err(|error| AutomationError::Execution(error.to_string()))?; - let indices = compatible_targets(plan) - .filter_map(|target| dataset_index(app, &target.id)) + let succeeded = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Succeeded) .collect::>(); - let scheme_plan = plan_scheme_application(&scheme, &app.doc.datasets, &indices); - let policy = if params.compatible_only { - SchemeApplicationPolicy::CompatibleOnly - } else { - SchemeApplicationPolicy::StrictAll - }; - let prepared = scheme_plan.prepare(policy).ok_or_else(|| { - AutomationError::Execution( - "no compatible processing targets, or strict policy rejected the selection".to_owned(), - ) - })?; - let applied_ids = prepared - .applied_targets + let failed = result + .targets .iter() - .filter_map(|index| app.doc.datasets.get(*index)) - .map(|dataset| dataset.resource_id().to_string()) - .collect::>(); - let skipped = scheme_plan - .targets() + .filter(|target| target.outcome == TargetOutcome::Failed) + .collect::>(); + let files = succeeded .iter() - .filter_map(|target| { - target.result.incompatibility_reason().map(|reason| { - let id = app - .doc - .datasets - .get(target.dataset) - .map(|dataset| dataset.resource_id().to_string()) - .unwrap_or_else(|| format!("stale-{}", target.dataset)); - (id, reason.to_owned()) - }) - }) - .collect::>(); - let mut result = commit_actions( - app, - plan, - vec![prepared.action], - "processing scheme applied", - )?; - result - .modified - .retain(|target| applied_ids.contains(&target.id)); - for target in &mut result.targets { - if let Some(reason) = skipped.get(&target.target.id) { - target.outcome = TargetOutcome::Skipped; - target.message.clone_from(reason); - } - } - if let Some(fingerprint) = fingerprint_file(¶ms.path, "processing_scheme") { - result.diagnostics.push(format!( - "processing scheme sha256 {} ({})", - fingerprint.sha256, - fingerprint.path.display() - )); - } - Ok(result) -} - -fn execute_import(app: &mut PlotxApp, plan: &ToolPlan) -> Result { - let params: ImportParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - if params.paths.is_empty() { - return Err(AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: "paths must contain at least one input".to_owned(), - }); - } - let base_dataset = app.doc.datasets.len(); - let base_canvas = app.doc.canvases.len(); - let mut actions = Vec::new(); - let mut produced = Vec::new(); - let mut item_results = Vec::new(); - for path in ¶ms.paths { - let external = ResourceRef { - id: path.display().to_string(), - kind: ResourceKindId::new(KIND_EXTERNAL_INPUT), - parent_id: None, - local_id: None, - }; - let loaded = match crate::workflow::load_dataset(path) { - Ok(loaded) => loaded, - Err(error) => { - item_results.push(TargetResult { - target: external, - outcome: TargetOutcome::Failed, - message: error.to_string(), - fingerprints: fingerprint_file(path, "selected_input") - .into_iter() - .collect(), - }); - continue; - } - }; - let fingerprints = input_fingerprints(&loaded.inspection); - produced.push(ResourceRef { - id: loaded.dataset.resource_id().to_string(), - kind: ResourceKindId::new(KIND_DATASET), - parent_id: None, - local_id: None, - }); - let offset = actions.len(); - actions.push(Action::InsertDatasetWithCanvas { - dataset_index: base_dataset + offset, - canvas_index: base_canvas + offset, - canvas_resource_id: crate::state::CanvasId::new(), - dataset: Box::new(loaded.dataset), - canvas_name: path - .file_stem() - .map(|value| value.to_string_lossy().into_owned()) - .unwrap_or_else(|| "Imported Data".to_owned()), - size_mm: crate::state::DEFAULT_CANVAS_SIZE_MM, - active_canvas_before: app.session.active_canvas, - active_dataset_before: app.active_dataset(), - inserted_into_existing_canvas: None, - inserted_object_id: None, - }); - item_results.push(TargetResult { - target: external, - outcome: TargetOutcome::Succeeded, - message: "imported into the canonical PlotX data model".to_owned(), - fingerprints, - }); - } - if actions.is_empty() { - return Ok(ToolResult { - tool_id: plan.request.tool_id.clone(), - before_revision: DocumentRevision(app.doc.automation_revision), - after_revision: DocumentRevision(app.doc.automation_revision), - targets: item_results, - produced: Vec::new(), - modified: Vec::new(), - diagnostics: Vec::new(), - verification: Vec::new(), - value: serde_json::Value::Null, - }); - } - let mut result = commit_actions(app, plan, actions, "imported")?; - produced.extend( - app.doc - .canvases - .iter() - .skip(base_canvas) - .map(|canvas| ResourceRef { - id: canvas.resource_id.to_string(), - kind: ResourceKindId::new(KIND_CANVAS), - parent_id: None, - local_id: None, - }), + .map(|target| target.fingerprints.len()) + .sum::(); + let summary = format!( + "{} wrote {files} file(s) from {} target(s)", + plan.request.tool_id, + succeeded.len() ); - result.produced = produced; - result.targets.extend(item_results); - Ok(result) -} - -fn execute_transform(app: &mut PlotxApp, plan: &ToolPlan) -> Result { - let params: TransformParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - if params.name.trim().is_empty() { - return Err(AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: "name must not be empty".into(), - }); + let mut diagnostics = Vec::new(); + for target in &succeeded { + for fingerprint in &target.fingerprints { + diagnostics.push( + crate::operation::Diagnostic::new( + crate::operation::Severity::Info, + crate::operation::DiagnosticCode::ExportSucceeded, + format!( + "wrote {} bytes for {}", + fingerprint.bytes, + target.target.describe() + ), + ) + .with_source("core.automation") + .with_context("path", fingerprint.path.display().to_string()) + .with_context("sha256", fingerprint.sha256.clone()) + .with_context("role", fingerprint.role.clone()), + ); + } } - if params.memory_limit_bytes == 0 { - return Err(AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: "memory_limit_bytes must be positive".into(), - }); + for target in &failed { + diagnostics.push( + crate::operation::Diagnostic::new( + crate::operation::Severity::Error, + crate::operation::DiagnosticCode::ExportFailed, + format!("{}: {}", target.target.describe(), target.message), + ) + .with_source("core.automation"), + ); } - let input_datasets = compatible_targets(plan) - .map(|target| { - dataset_index(app, &target.id).ok_or_else(|| { - AutomationError::Execution(format!("table resource {} disappeared", target.id)) - }) - }) - .collect::, _>>()?; - if input_datasets.is_empty() { - return Err(AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: "select at least one typed table input".into(), - }); + if succeeded.is_empty() && failed.is_empty() { + diagnostics.push( + crate::operation::Diagnostic::new( + crate::operation::Severity::Warning, + crate::operation::DiagnosticCode::ExportProducedNoFiles, + "no target produced an external file".to_owned(), + ) + .with_source("core.automation"), + ); } - let index = app - .derive_table_from_plan( - params.plan, - &input_datasets, - params.name, - params.memory_limit_bytes, + let id = app.session.begin_operation(); + let mut report = if failed.is_empty() { + crate::operation::OperationReport::success( + id, + crate::operation::OperationKind::Export, + summary, + (), ) - .map_err(AutomationError::Execution)?; - let output = app.doc.datasets[index].as_table().ok_or_else(|| { - AutomationError::Execution("transform did not produce a typed table".into()) - })?; - let produced = ResourceRef { - id: output.resource_id.to_string(), - kind: ResourceKindId::new(KIND_DATASET), - parent_id: None, - local_id: None, - }; - Ok(ToolResult { - tool_id: plan.request.tool_id.clone(), - before_revision: DocumentRevision(app.doc.automation_revision), - after_revision: DocumentRevision(app.doc.automation_revision), - targets: compatible_targets(plan) - .cloned() - .map(|target| TargetResult { - target, - outcome: TargetOutcome::Succeeded, - message: "executed as a pinned PlotX RelPlanV1 input".into(), - fingerprints: Vec::new(), - }) - .collect(), - produced: vec![produced], - modified: Vec::new(), - diagnostics: output - .typed_state - .envelope - .revision - .operation - .diagnostics - .iter() - .map(|diagnostic| diagnostic.message.clone()) - .collect(), - verification: Vec::new(), - value: serde_json::json!({ - "table_id": output.typed_state.envelope.revision.table_id, - "revision_id": output.typed_state.envelope.revision.id, - "fingerprint": output.typed_state.envelope.revision.snapshot.fingerprint, - }), - }) -} - -fn execute_export(app: &PlotxApp, plan: &ToolPlan) -> Result { - let params: ExportParams = parse(&plan.request.tool_id, plan.request.parameters.clone())?; - let format = - parse_export_format(¶ms.format).ok_or_else(|| AutomationError::InvalidParameters { - tool_id: plan.request.tool_id.clone(), - message: format!("unsupported export format '{}'", params.format), - })?; - std::fs::create_dir_all(¶ms.directory).map_err(|source| AutomationError::Io { - path: params.directory.clone(), - source, - })?; - let mut outputs = Vec::new(); - let mut targets = plan - .targets - .iter() - .filter(|target| target.status != TargetCompatibility::Compatible) - .map(|target| TargetResult { - target: target.target.clone(), - outcome: TargetOutcome::Skipped, - message: target.reason.clone(), - fingerprints: Vec::new(), - }) - .collect::>(); - for target in compatible_targets(plan) { - let Some(index) = canvas_index(app, &target.id) else { - continue; - }; - let canvas = &app.doc.canvases[index]; - let safe_name = sanitize_file_name(&canvas.name); - let base = params.directory.join(safe_name); - let expected = crate::export::export_output_paths(&base, format, 1); - if !params.overwrite && expected.iter().any(|path| path.exists()) { - targets.push(TargetResult { - target: target.clone(), - outcome: TargetOutcome::Failed, - message: "output already exists and overwrite is false".to_owned(), - fingerprints: expected - .iter() - .filter_map(|path| fingerprint_file(path, "existing_output")) - .collect(), - }); - continue; - } - let written = crate::export::export_canvases( - std::slice::from_ref(canvas), - Some(0), - &ExportSettings { - format, - scope: ExportPageScope::Current, - dpi: params.dpi, - target_width_mm: None, - trim_to_visible_content: false, - }, - &base, + } else { + crate::operation::OperationReport::warning( + id, + crate::operation::OperationKind::Export, + summary, + (), ) - .map_err(|error| AutomationError::Execution(error.to_string()))?; - outputs.extend(written.iter().map(|path| path.display().to_string())); - targets.push(TargetResult { - target: target.clone(), - outcome: TargetOutcome::Succeeded, - message: written - .iter() - .map(|path| path.display().to_string()) - .collect::>() - .join(", "), - fingerprints: written - .iter() - .filter_map(|path| fingerprint_file(path, "output")) - .collect(), - }); - } - Ok(ToolResult { - tool_id: plan.request.tool_id.clone(), - before_revision: DocumentRevision(app.doc.automation_revision), - after_revision: DocumentRevision(app.doc.automation_revision), - targets, - produced: Vec::new(), - modified: Vec::new(), - diagnostics: outputs, - verification: vec![VerificationRecord { - check: "outputs_exist".to_owned(), - passed: true, - message: "all successful exports were committed by the PlotX exporter".to_owned(), - }], - value: serde_json::Value::Null, - }) + }; + report.diagnostics = diagnostics; + app.session.record_operation(report); } -fn commit_actions( - app: &mut PlotxApp, - plan: &ToolPlan, - actions: Vec, - verb: &str, -) -> Result { - if actions.is_empty() { - return Err(AutomationError::Execution( - "tool had no compatible targets".to_owned(), - )); - } - app.try_execute_action(Action::Composite(actions)) - .map_err(|error| AutomationError::Execution(error.to_string()))?; - let modified = compatible_targets(plan).cloned().collect::>(); - Ok(ToolResult { - tool_id: plan.request.tool_id.clone(), - before_revision: plan.frozen_targets.revision, - after_revision: DocumentRevision(app.doc.automation_revision), - targets: plan - .targets - .iter() - .map(|target| TargetResult { - target: target.target.clone(), - outcome: if target.status == TargetCompatibility::Compatible { - TargetOutcome::Succeeded - } else { - TargetOutcome::Skipped - }, - message: if target.status == TargetCompatibility::Compatible { - verb.to_owned() - } else { - target.reason.clone() - }, - fingerprints: Vec::new(), - }) - .collect(), - produced: Vec::new(), - modified, - diagnostics: Vec::new(), - verification: vec![VerificationRecord { - check: "revision_advanced".to_owned(), - passed: app.doc.automation_revision > plan.frozen_targets.revision.0, - message: "atomic document commit completed".to_owned(), - }], - value: serde_json::Value::Null, - }) +/// The resources of every compatible planned target. +/// +/// Tools that address whole resources keep reading a `ResourceRef` here: their +/// planned targets never carry a component, so projecting it away costs them +/// nothing and keeps the component out of code that has no meaning for it. +/// Tools whose plan expands into components read [`compatible_target_refs`]. +pub(super) fn compatible_targets(plan: &ToolPlan) -> impl Iterator { + compatible_target_refs(plan).map(|target| &target.resource) } -fn compatible_targets(plan: &ToolPlan) -> impl Iterator { +pub(super) fn compatible_target_refs(plan: &ToolPlan) -> impl Iterator { plan.targets .iter() .filter(|target| target.status == TargetCompatibility::Compatible) .map(|target| &target.target) } -fn dataset_index(app: &PlotxApp, id: &str) -> Option { - app.doc - .datasets - .iter() - .position(|dataset| dataset.resource_id().to_string() == id) -} - -fn canvas_index(app: &PlotxApp, id: &str) -> Option { - app.doc - .canvases - .iter() - .position(|canvas| canvas.resource_id.to_string() == id) -} - -fn serde_result( - tool_id: &str, - revision: DocumentRevision, - value: impl serde::Serialize, -) -> Result { - Ok(readonly_value( - tool_id, - revision, - serde_json::to_value(value) - .map_err(|error| AutomationError::Execution(error.to_string()))?, - )) -} - -fn readonly_value( - tool_id: &str, - revision: DocumentRevision, - value: serde_json::Value, -) -> ToolResult { - ToolResult { - tool_id: tool_id.to_owned(), - before_revision: revision, - after_revision: revision, - targets: Vec::new(), - produced: Vec::new(), - modified: Vec::new(), - diagnostics: Vec::new(), - verification: Vec::new(), - value, - } -} - -fn parse_export_format(value: &str) -> Option { - match value.to_ascii_lowercase().as_str() { - "svg" => Some(ExportFormat::Svg), - "pdf" => Some(ExportFormat::Pdf), - "png" => Some(ExportFormat::Png), - "tiff" | "tif" => Some(ExportFormat::Tiff), - "jpeg" | "jpg" => Some(ExportFormat::Jpeg), - _ => None, - } -} - -fn sanitize_file_name(value: &str) -> String { - let value = value - .chars() - .map(|ch| if "<>:\"/\\|?*".contains(ch) { '_' } else { ch }) - .collect::(); - let value = value.trim().trim_end_matches(['.', ' ']); - if value.is_empty() { - "figure".to_owned() - } else { - value.to_owned() - } -} - -fn input_fingerprints(inspection: &crate::workflow::InspectionReport) -> Vec { - let mut paths = vec![("selected_input", &inspection.provenance.selected_path)]; - paths.push(("data", &inspection.provenance.data_path)); - paths.extend( - inspection - .provenance - .parameter_paths - .iter() - .map(|path| ("parameter", path)), - ); - let mut seen = BTreeSet::new(); - paths - .into_iter() - .filter(|(_, path)| seen.insert((*path).clone())) - .filter_map(|(role, path)| fingerprint_file(path, role)) - .collect() -} - -fn fingerprint_file(path: &Path, role: &str) -> Option { - let bytes = std::fs::read(path).ok()?; - Some(FingerprintRecord { - role: role.to_owned(), - path: path.to_owned(), - sha256: format!("{:x}", Sha256::digest(&bytes)), - bytes: bytes.len() as u64, - }) -} - pub fn write_run_manifest(path: &Path, manifest: &RunManifest) -> Result<(), AutomationError> { if let Some(parent) = path .parent() diff --git a/crates/core/src/automation/types.rs b/crates/core/src/automation/types.rs index 9125bd5..61269ec 100644 --- a/crates/core/src/automation/types.rs +++ b/crates/core/src/automation/types.rs @@ -79,6 +79,22 @@ impl TargetRef { component: None, } } + + /// A one-line identity for a result row or a preflight list. + /// + /// The component is part of the identity, not decoration: a plan that + /// expands one plot object into one target per series produces rows that + /// are otherwise byte-identical, and a list of identical rows tells a user + /// nothing about which of them applied. + pub fn describe(&self) -> String { + match &self.component { + None => self.resource.id.clone(), + Some(ComponentRef::Series(series)) => format!("{} · series {series}", self.resource.id), + Some(ComponentRef::ProcessingStep(step)) => { + format!("{} · step {}", self.resource.id, step.get()) + } + } + } } #[derive(Clone, Debug, PartialEq, Eq)] @@ -315,7 +331,7 @@ pub enum TargetCompatibility { #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct PlannedTarget { - pub target: ResourceRef, + pub target: TargetRef, pub status: TargetCompatibility, pub reason: String, } @@ -342,7 +358,7 @@ pub enum TargetOutcome { #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct TargetResult { - pub target: ResourceRef, + pub target: TargetRef, pub outcome: TargetOutcome, pub message: String, #[serde(default, skip_serializing_if = "Vec::is_empty")] diff --git a/crates/core/src/automation/workflow.rs b/crates/core/src/automation/workflow.rs index 138c98e..2c24348 100644 --- a/crates/core/src/automation/workflow.rs +++ b/crates/core/src/automation/workflow.rs @@ -340,9 +340,11 @@ pub fn execute_workflow( expected_revision: DocumentRevision(app.doc.automation_revision), caller, }; + // The inner executor: this run writes one manifest covering every node, + // so a per-node manifest here would record the same work twice. let outcome = plan_tool(app, request).and_then(|plan| { let frozen = plan.frozen_targets.clone(); - execute_tool(app, plan, authority).map(|result| (frozen, result)) + super::tools::execute_planned_tool(app, plan, authority).map(|result| (frozen, result)) }); match outcome { Ok((frozen_targets, result)) => { @@ -444,7 +446,7 @@ pub fn execute_workflow( table_revisions, table_plans, }; - app.doc.automation_runs.push(manifest.clone()); + super::tools::record_run(&mut app.doc, manifest.clone()); observer(TaskEvent::Finished { id: TaskId(run_id), cancelled, @@ -452,7 +454,10 @@ pub fn execute_workflow( Ok(manifest) } -fn typed_table_revisions(app: &PlotxApp, role: &str) -> BTreeMap { +pub(super) fn typed_table_revisions( + app: &PlotxApp, + role: &str, +) -> BTreeMap { app.doc .datasets .iter() @@ -474,7 +479,7 @@ fn typed_table_revisions(app: &PlotxApp, role: &str) -> BTreeMap, @@ -730,7 +735,7 @@ fn known_output_port(port: &str) -> bool { ) } -fn unix_ms() -> u128 { +pub(super) fn unix_ms() -> u128 { SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap_or_default() diff --git a/crates/core/src/properties/contour.rs b/crates/core/src/properties/contour.rs index 63c6391..243d61c 100644 --- a/crates/core/src/properties/contour.rs +++ b/crates/core/src/properties/contour.rs @@ -370,7 +370,7 @@ pub(super) fn write( let updated = with_magnitude(&spec.positive.base, magnitude).ok_or({ PropertyError::InvalidValue { property: id, - message: magnitude_message(&spec.positive.base), + message: magnitude_message(&spec.positive.base, magnitude), } })?; for half in halves(spec) { @@ -385,7 +385,7 @@ pub(super) fn write( .ok_or(PropertyError::InvalidValue { property: id, message: format!( - "level count must be between 1 and {}", + "level count {count} is out of range: it must be between 1 and {}", ContourLevelSpec::MAX_COUNT ), })?; @@ -550,16 +550,26 @@ fn bounded_multiplier(value: f64) -> Option { PositiveFiniteF64::new(value).filter(|value| value.get() <= MAX_MULTIPLIER) } -fn magnitude_message(base: &ContourBasePolicy) -> String { +/// Why a magnitude was refused, in terms of the anchor that refused it. +/// +/// The rejected value is part of the message. The bound alone tells a user what +/// the rule is but not which value broke it, and the anchor's bound is not the +/// definition's static bound — a multiplier stops at `MAX_MULTIPLIER` while an +/// absolute level does not — so a caller cannot reconstruct it from the schema. +fn magnitude_message(base: &ContourBasePolicy, magnitude: f64) -> String { match base { ContourBasePolicy::Absolute(_) => { - "absolute level must be a finite value greater than zero".to_owned() + format!( + "absolute level {magnitude} is out of range: it must be a finite value greater than zero" + ) } ContourBasePolicy::NoiseFloor { .. } | ContourBasePolicy::BackgroundScale { .. } => { - format!("multiplier must be greater than 0 and at most {MAX_MULTIPLIER}") + format!( + "multiplier {magnitude} is out of range: it must be greater than 0 and at most {MAX_MULTIPLIER}" + ) } ContourBasePolicy::FractionOfRange(_) => { - "fraction must be greater than 0 and at most 1".to_owned() + format!("fraction {magnitude} is out of range: it must be greater than 0 and at most 1") } } } diff --git a/crates/core/src/properties/mod.rs b/crates/core/src/properties/mod.rs index 5d8de95..c4c25aa 100644 --- a/crates/core/src/properties/mod.rs +++ b/crates/core/src/properties/mod.rs @@ -70,9 +70,30 @@ pub fn permitted_variants( .collect() } +/// The choice ids of a variant set, for an error a caller can act on. +/// +/// Every rejection of an enumerated value names the alternatives, whether the +/// value failed to name a choice at all (the wire boundary) or named one this +/// field's capabilities withhold (the planner). One formatting keeps those two +/// messages from drifting into two different vocabularies for one list. +pub fn variant_list(variants: &[&'static EnumVariant]) -> String { + if variants.is_empty() { + return "no choices at all".to_owned(); + } + variants + .iter() + .map(|variant| format!("'{}'", variant.id)) + .collect::>() + .join(", ") +} + +// Visible crate-wide under `cfg(test)` only: the automation adapter's +// differential tests drive the very same page fixture through the JSON entry +// point, and building a second copy of it there would let the two entry points +// be compared against two different documents. #[cfg(test)] #[path = "tests.rs"] -mod tests; +pub(crate) mod tests; #[cfg(test)] #[path = "ladder_tests.rs"] diff --git a/crates/core/src/properties/model.rs b/crates/core/src/properties/model.rs index 90ce23e..757f111 100644 --- a/crates/core/src/properties/model.rs +++ b/crates/core/src/properties/model.rs @@ -183,6 +183,12 @@ impl FloatBounds { } /// Validate a value against the bound, naming the property in the failure. + /// + /// The failure states the value that was rejected as well as the rule. + /// "Level ratio must be greater than 1 and at most 10" leaves a caller that + /// sent 10.0000001, or sent a string that parsed to something else + /// entirely, unable to tell which end it fell off — and a headless caller + /// has no control to look at. Naming both closes that. pub fn check( self, property: PropertyId, @@ -194,7 +200,10 @@ impl FloatBounds { } Err(PropertyError::InvalidValue { property, - message: format!("{subject} must be {}", self.describe()), + message: format!( + "{subject} {value} is out of range: it must be {}", + self.describe() + ), }) } } diff --git a/crates/core/src/properties/service.rs b/crates/core/src/properties/service.rs index c346580..3a5b9af 100644 --- a/crates/core/src/properties/service.rs +++ b/crates/core/src/properties/service.rs @@ -6,7 +6,7 @@ //! of the targets that were skipped and why. use super::model::*; -use super::{contour, definition, permitted_variants}; +use super::{contour, definition, permitted_variants, variant_list}; use crate::actions::Action; use crate::automation::{ComponentRef, ResourceRef, TargetRef, canvas_object_ref}; use crate::state::{ @@ -66,6 +66,20 @@ impl PlotxApp { .unwrap_or_default() } + /// Every series component of one plot-object *resource*. + /// + /// This is the expansion an automation plan performs: a selector names + /// resources, and a property whose scope is a series has to become one + /// target per series before anything can be said about applicability. It + /// resolves the reference through the same lookup a property write does, so + /// a plan and the commit that follows it cannot disagree about which + /// components exist. + pub fn resource_series_targets(&self, resource: &ResourceRef) -> Vec { + self.canvas_object(resource) + .map(|(canvas, object)| self.series_targets(canvas, object)) + .unwrap_or_default() + } + /// Read one property against one target. pub fn resolve_property( &self, @@ -146,9 +160,16 @@ impl PlotxApp { if let PropertyValue::Enum(choice) = value && !permitted.iter().any(|variant| variant.id == *choice) { + // Name what this field does allow. A caller told only that its + // choice is unavailable has to guess the next one, and the + // permitted set is a fact about the target's capabilities that + // no caller can derive from the static schema. return Err(PropertyError::InvalidValue { property, - message: format!("'{choice}' needs a capability this field does not expose",), + message: format!( + "'{choice}' needs a capability this field does not expose; this field allows {}", + variant_list(&permitted) + ), }); } contour::write(property, spec, value, &|| { diff --git a/crates/core/src/properties/tests.rs b/crates/core/src/properties/tests.rs index 0030297..73d9662 100644 --- a/crates/core/src/properties/tests.rs +++ b/crates/core/src/properties/tests.rs @@ -45,13 +45,13 @@ fn nmr2d_with(source: &str, values: &[f64]) -> plotx_io::NmrData2D { /// One page holding one plot bound to a true-2D spectrum, i.e. the exact shape /// the driving case has: a contour drawn from a signed scalar grid. -pub(super) fn contour_app() -> (PlotxApp, TargetRef) { +pub(crate) fn contour_app() -> (PlotxApp, TargetRef) { contour_app_with_plane(&default_plane()) } /// The same page over a plane the caller chooses, so a test can put a field of /// a given dynamic range in front of the catalog. -pub(super) fn contour_app_with_plane(values: &[f64]) -> (PlotxApp, TargetRef) { +pub(crate) fn contour_app_with_plane(values: &[f64]) -> (PlotxApp, TargetRef) { let mut app = PlotxApp::new(); app.doc .datasets @@ -79,7 +79,7 @@ pub(super) fn contour_app_with_plane(values: &[f64]) -> (PlotxApp, TargetRef) { (app, target) } -pub(super) fn contour_spec(app: &PlotxApp, target: &TargetRef) -> plotx_figure::ContourSpec { +pub(crate) fn contour_spec(app: &PlotxApp, target: &TargetRef) -> plotx_figure::ContourSpec { let Some(ComponentRef::Series(series)) = target.component else { panic!("the fixture addresses a series"); }; @@ -633,3 +633,31 @@ fn the_source_field_is_not_the_component() { assert_eq!(contour_spec(&app, &first).positive.count, 14); assert_eq!(contour_spec(&app, &second).positive.count, 3); } + +/// The typed entry point's own out-of-range refusal names both the value that +/// was rejected and the rule that rejected it. +/// +/// The automation adapter checks the declared bound before it ever builds a +/// typed value, so this bound is reached only through the panel's path — and +/// a panel user who typed a number and saw "must be greater than 1 and at most +/// 10" still cannot tell which end their number fell off. +#[test] +fn the_typed_planner_names_the_rejected_value_and_the_bound() { + let (app, target) = contour_app(); + let error = app + .plan_property_write( + contour::RATIO, + std::slice::from_ref(&target), + &PropertyValue::Float(42.0), + ) + .expect_err("42 is above the declared ratio bound"); + let message = error.to_string(); + assert!( + message.contains("42"), + "the rejected value is named: {message}" + ); + assert!( + message.contains("greater than 1") && message.contains("at most 10"), + "the bound is named: {message}" + ); +} diff --git a/crates/core/tests/slice2d.rs b/crates/core/tests/slice2d.rs index a889d8f..3ec720a 100644 --- a/crates/core/tests/slice2d.rs +++ b/crates/core/tests/slice2d.rs @@ -1,9 +1,21 @@ //! End-to-end 2D slice: synthetic 2D FID → 2D FFT → contour / stack figure → //! SVG export. No files needed. +//! +//! The same harness also drives the property catalog's two entry points against +//! one document, because the thing automation can break here is not the +//! numerics — those are the same code either way — but the JSON adapter quietly +//! growing a second copy of the planner. use num_complex::Complex64; +use plotx_core::automation::{ + CallerType, DocumentRevision, TOOL_RESET, TOOL_SET, TargetOutcome, TargetRef, TargetSelector, + ToolRequest, execute_tool, plan_tool, +}; use plotx_core::build_stack_figure; -use plotx_core::state::{CanvasDocument, Dataset, Nmr2DDataset, ObjectFrame, PlotxApp}; +use plotx_core::properties::{PropertyId, PropertyValue, contour}; +use plotx_core::state::{ + CanvasDocument, Dataset, Nmr2DDataset, ObjectFrame, ObjectId, PlotxApp, SeriesBinding, +}; use plotx_io::{Dim, Domain, NmrData2D, QuadMode}; use plotx_processing::{Layout2D, Params2D, Preset2D, Processed2D, process_2d, recommend_preset}; use std::f64::consts::TAU; @@ -129,6 +141,412 @@ fn contour_slice_places_peak_and_exports_svg() { assert!(svg.contains("chemical shift")); } +// --------------------------------------------------------------------------- +// The property catalog's two entry points, over the same real contour +// --------------------------------------------------------------------------- + +/// Wait for the asynchronous contour work a commit triggers. +/// +/// Asserting geometry before the build settles reads the *previous* answer and +/// passes for the wrong reason, so every check that looks at a figure goes +/// through here first. +fn settle(app: &mut PlotxApp) { + let deadline = Instant::now() + Duration::from_secs(2); + while app.compute_busy() && Instant::now() < deadline { + app.poll_compute(); + thread::sleep(Duration::from_millis(5)); + } + app.poll_compute(); + assert!(!app.compute_busy(), "contour jobs did not settle"); +} + +/// A page holding one plot of a real processed 2D spectrum, settled. +/// +/// Built from the same synthetic FID as the slice above and run through the +/// same `process_2d`, so the field the catalog sees is a genuine spectrum with +/// a genuine noise estimate rather than a hand-written grid. +fn contour_page() -> (PlotxApp, ObjectId) { + let mut app = PlotxApp::new(); + app.doc + .datasets + .push(Dataset::Nmr2D(Box::new(Nmr2DDataset::load( + synthetic_hsqc(3.0, 10.0, "hsqcetgpsisp"), + )))); + let mut canvas = CanvasDocument::new("contour".to_owned(), [120.0, 80.0]); + let [width, height] = canvas.size_pt(); + let id = canvas.allocate_object_id(); + let object = app.build_plot_object( + 0, + ObjectFrame::new(0.0, 0.0, width, height), + id, + "Contour".to_owned(), + ); + canvas.objects.push(object); + app.doc.canvases.push(canvas); + app.session.active_canvas = Some(0); + settle(&mut app); + (app, id) +} + +fn object_resource_id(app: &PlotxApp, object: ObjectId) -> String { + format!("{}/{object}", app.doc.canvases[0].resource_id) +} + +fn series_targets(app: &PlotxApp, object: ObjectId) -> Vec { + app.series_targets(0, object) +} + +/// Every series of the plot, reduced to the state a property write owns. +/// +/// Comparing this whole rather than just the number that was written is what +/// makes the differential check meaningful: an adapter that also rounded a +/// neighbouring field, wrote only one half of the ladder, or touched a series +/// it should have skipped would still pass "did the value change?" and fails +/// here. The dataset's own UUID is excluded because it is minted per document +/// and identifies the fixture, not the edit. +fn binding(app: &PlotxApp, object: ObjectId) -> Vec { + app.doc.canvases[0] + .object(object) + .and_then(|object| object.plot()) + .expect("the page holds a plot") + .binding + .series + .iter() + .map(Comparable::of) + .collect() +} + +#[derive(Clone, Debug, PartialEq)] +struct Comparable { + series: plotx_core::state::SeriesId, + field: plotx_core::state::FieldId, + visible: bool, + label: Option, + encoding: plotx_figure::SeriesEncoding, +} + +impl Comparable { + fn of(series: &SeriesBinding) -> Self { + Self { + series: series.id, + field: series.source.field, + visible: series.visible, + label: series.label.clone(), + encoding: series.encoding.clone(), + } + } +} + +fn tool_request( + app: &PlotxApp, + tool_id: &str, + object: ObjectId, + parameters: serde_json::Value, +) -> ToolRequest { + ToolRequest { + tool_id: tool_id.to_owned(), + tool_version: 1, + parameters, + targets: TargetSelector::Explicit { + ids: vec![object_resource_id(app, object)], + }, + expected_revision: DocumentRevision(app.doc.automation_revision), + caller: CallerType::Agent, + } +} + +fn run_tool( + app: &mut PlotxApp, + tool_id: &str, + object: ObjectId, + parameters: serde_json::Value, +) -> plotx_core::automation::ToolResult { + let request = tool_request(app, tool_id, object, parameters); + let plan = plan_tool(app, request).expect("the tool plans"); + let authority = plan.required_authority; + execute_tool(app, plan, authority).expect("the tool executes") +} + +/// Write one property through the typed entry point the panel uses. +fn write_typed(app: &mut PlotxApp, object: ObjectId, property: PropertyId, value: PropertyValue) { + let targets = series_targets(app, object); + let commit = app + .plan_property_write(property, &targets, &value) + .expect("the typed planner accepts the value"); + app.commit_property(commit); +} + +/// The differential check this stage exists for. +/// +/// One document, one target, one value, through the panel's typed planner and +/// through `properties.set`. The resulting document state must be identical +/// field for field. An adapter that re-implemented planning would still make +/// "did the number change?" pass; it cannot make this pass, because any +/// difference in clamping, rounding, which halves of the ladder are written, or +/// which series are touched shows up in the compared bindings. +#[test] +fn typed_and_json_property_writes_produce_identical_documents() { + for (property, typed, json) in [ + ( + contour::COUNT, + PropertyValue::Int(11), + serde_json::json!(11), + ), + ( + contour::RATIO, + PropertyValue::Float(1.75), + serde_json::json!(1.75), + ), + ( + contour::BASE_MAGNITUDE, + PropertyValue::Float(7.5), + serde_json::json!(7.5), + ), + ( + contour::NEGATIVE_ENABLED, + PropertyValue::Bool(false), + serde_json::json!(false), + ), + ( + contour::LINE_WIDTH, + PropertyValue::Float(0.9), + serde_json::json!(0.9), + ), + ] { + let (mut typed_app, typed_object) = contour_page(); + let (mut json_app, json_object) = contour_page(); + assert_eq!( + binding(&typed_app, typed_object), + binding(&json_app, json_object), + "the two pages start identical" + ); + + write_typed(&mut typed_app, typed_object, property, typed); + settle(&mut typed_app); + + let result = run_tool( + &mut json_app, + TOOL_SET, + json_object, + serde_json::json!({"key": property.as_str(), "value": json}), + ); + settle(&mut json_app); + + assert_eq!( + binding(&json_app, json_object), + binding(&typed_app, typed_object), + "{property} must land identically through both entry points" + ); + assert!( + result + .targets + .iter() + .any(|target| target.outcome == TargetOutcome::Succeeded), + "{property} applied through the JSON entry point" + ); + assert_eq!( + result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Skipped) + .count(), + 0, + "{property} skipped nothing on a page whose only series is a contour" + ); + } +} + +/// The same equivalence for a reset, which derives its value from the default +/// policy in the target's current context rather than taking one from the +/// caller. A reset is where an adapter is most tempted to reach for a literal. +#[test] +fn typed_and_json_property_resets_produce_identical_documents() { + let property = contour::COUNT; + let (mut typed_app, typed_object) = contour_page(); + let (mut json_app, json_object) = contour_page(); + + // Move both pages off the default first, or the reset would be a no-op and + // the comparison would hold for the wrong reason. + write_typed( + &mut typed_app, + typed_object, + property, + PropertyValue::Int(3), + ); + settle(&mut typed_app); + run_tool( + &mut json_app, + TOOL_SET, + json_object, + serde_json::json!({"key": property.as_str(), "value": 3}), + ); + settle(&mut json_app); + let moved = binding(&typed_app, typed_object); + assert_eq!(binding(&json_app, json_object), moved); + + let targets = series_targets(&typed_app, typed_object); + let commit = typed_app + .plan_property_reset(property, &targets) + .expect("the typed planner resets"); + typed_app.commit_property(commit); + settle(&mut typed_app); + + run_tool( + &mut json_app, + TOOL_RESET, + json_object, + serde_json::json!({"key": property.as_str()}), + ); + settle(&mut json_app); + + let reset = binding(&typed_app, typed_object); + assert_ne!(reset, moved, "the reset actually moved the value back"); + assert_eq!( + binding(&json_app, json_object), + reset, + "a reset must land identically through both entry points" + ); +} + +/// Planning expands one plot object into one target per series, and a series +/// the property does not apply to is reported rather than dropped. +#[test] +fn planning_expands_series_and_reports_the_ones_it_skips() { + let (mut app, object) = contour_page(); + // A second series over the same field, drawn as a line: the contour catalog + // does not apply to it, and it must be visible as a skip rather than vanish. + { + let plot = app.doc.canvases[0] + .objects + .iter_mut() + .find(|candidate| candidate.id == object) + .and_then(|object| object.plot_mut()) + .expect("the page holds a plot"); + let mut line = plot.binding.series[0].clone(); + line.encoding = plotx_figure::SeriesEncoding::Line(plotx_figure::LineEncoding::default()); + line.id = plot.allocate_series_id(); + plot.binding.series.push(line); + } + settle(&mut app); + + let request = tool_request( + &app, + TOOL_SET, + object, + serde_json::json!({"key": contour::COUNT.as_str(), "value": 8}), + ); + let plan = plan_tool(&app, request).expect("the tool plans"); + assert_eq!( + plan.targets.len(), + 2, + "one plot object expands into one target per series" + ); + assert!( + plan.targets + .iter() + .all(|target| target.target.component.is_some()), + "every expanded target names its component" + ); + + let authority = plan.required_authority; + let result = execute_tool(&mut app, plan, authority).expect("the tool executes"); + settle(&mut app); + + let succeeded = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Succeeded) + .collect::>(); + let skipped = result + .targets + .iter() + .filter(|target| target.outcome == TargetOutcome::Skipped) + .collect::>(); + assert_eq!(succeeded.len(), 1, "{:?}", result.targets); + assert_eq!(skipped.len(), 1, "{:?}", result.targets); + assert!( + !skipped[0].message.is_empty(), + "the skip carries its reason to the caller" + ); + assert_ne!( + succeeded[0].target.describe(), + skipped[0].target.describe(), + "the two rows are distinguishable in a result list" + ); +} + +/// Geometry diagnostics cannot reach the result of the call that caused them. +/// +/// A property write commits a spec; the geometry is rebuilt by a background +/// job, and the renderer's segment budget — which drops whole levels and +/// reports how many (`ContourGeometry::omitted`) — is applied inside that job. +/// `execute_tool` has already returned by then, so a `ToolResult` reports a +/// clean success for a write whose plot is later cut down. Today that diagnostic +/// reaches `session.status` only (`app_impl_figures.rs`), and there is no +/// channel from it to a `ToolResult`. +/// +/// This test pins the ordering that makes the gap structural rather than a +/// missing wire: it is not that someone forgot to copy a field across, it is +/// that the answer does not exist yet when the result is built. Closing it needs +/// a deferred or task-completion-shaped tool result, which is a change to the +/// tool contract and out of this stage's scope. +#[test] +fn a_property_write_returns_before_its_geometry_is_built() { + let (mut app, object) = contour_page(); + let request = tool_request( + &app, + TOOL_SET, + object, + serde_json::json!({"key": contour::BASE_MAGNITUDE.as_str(), "value": 2.0}), + ); + let plan = plan_tool(&app, request).expect("the tool plans"); + let authority = plan.required_authority; + let result = execute_tool(&mut app, plan, authority).expect("the tool executes"); + assert!( + result.diagnostics.is_empty(), + "the result carries no geometry diagnostic, because none exists yet" + ); + assert!( + app.compute_busy(), + "geometry for the value just written is still outstanding when the tool returns" + ); + settle(&mut app); +} + +/// A real contour, driven from the JSON entry point, still ends up as drawn +/// geometry — the tool does not merely mutate a spec that nothing consumes. +#[test] +fn a_json_property_write_reaches_the_drawn_figure() { + let (mut app, object) = contour_page(); + let before = app.doc.canvases[0] + .object(object) + .and_then(|object| object.plot()) + .expect("plot") + .figure + .contours + .len(); + assert!(before > 0, "the page draws contours to begin with"); + + run_tool( + &mut app, + TOOL_SET, + object, + serde_json::json!({"key": contour::NEGATIVE_ENABLED.as_str(), "value": false}), + ); + settle(&mut app); + + let plot = app.doc.canvases[0] + .object(object) + .and_then(|object| object.plot()) + .expect("plot"); + assert!( + !plot.figure.contours.is_empty(), + "the positive half is still drawn" + ); + let svg = plotx_render::svg::export(&plot.figure); + assert!(svg.contains("Ctrl+K（macOS 上为 Cmd）同时搜索设置、命令和 数据。输入 `contour threshold`、`sigma` 或 `levels` 即可找到对应的行，PlotX 会 打开它所在的面板、展开其分节并高亮该行。参见[命令面板](/zh-cn/reference/command-palette/)。 -- Ribbon 的 **Figure** 页签上的 **Contour** 按钮跳转到同一处。 +- Ribbon **Figure** 页签 **Style** 组里的 **Contour** 按钮跳转到同一处。 - 在图上点击右键，选择 **Contour settings…**。 - `+` 与 `-` 直接在图上调整最低层。 diff --git a/docs/src/content/docs/zh-cn/reference/command-palette.md b/docs/src/content/docs/zh-cn/reference/command-palette.md index 3d1a868..27e1240 100644 --- a/docs/src/content/docs/zh-cn/reference/command-palette.md +++ b/docs/src/content/docs/zh-cn/reference/command-palette.md @@ -24,8 +24,9 @@ description: 用键盘搜索命令、设置与数据。 列表每行左侧是名称，右侧以灰色显示提示：命令显示其快捷键，设置显示其所在面板， 数据显示其类型或所属页面。 -设置的匹配范围不止其可见标签：还包括它的标识符（整体或逐词拆开）及其别名。 -`contour threshold`、`sigma` 与 `series.contour.count` 都能命中等高线相关的行。 +设置的匹配范围不止你看到的标签：还包括它的别名，以及它在[工作流](/zh-cn/guides/automation/) +中使用的 id（整体或逐词拆开）。`contour threshold`、`sigma` 与 +`series.contour.count` 都能命中等高线相关的行。 激活一个设置不会改变任何内容。它会打开该设置所在的面板、展开其分节、滚动到该行 并短暂高亮，让你先看清当前值再编辑。 @@ -52,7 +53,8 @@ description: 用键盘搜索命令、设置与数据。 - 排列：网格、对齐、分布、层序与 *Tidy up frames*（一键整理）。 - 应用主题与堆叠数据。 - 切换到任意工具。 -- *Contour settings*（等高线设置），以及提高或降低等高线最低层。 +- *Contour settings*（等高线设置）、*Raise lowest level*（提高最低层）与 + *Lower lowest level*（降低最低层）。 设置：