From b2e0528073e0960cb8ab4e477e3e97d13161b897 Mon Sep 17 00:00:00 2001 From: Jiekang Tian Date: Sun, 26 Jul 2026 01:15:57 +0800 Subject: [PATCH] feat(core): expose property tools through automation and record runs Register properties.inspect, properties.set and properties.reset in the tool registry so agents, workflows and the command line reach the same planner the object inspector uses. The JSON adapter decodes each value against the property's own schema and then delegates planning and committing to properties::service, so the two entry points cannot drift apart on what a value means or which targets accept it. Planned and resulting targets now carry a TargetRef, letting a property tool expand one plot object into one target per series while every existing tool keeps addressing whole resources unchanged. Record a run manifest for every tool call that does not come from a person, single calls included, and mark the document dirty when one is appended so the record cannot be lost on exit. A single data.transform call now carries the same table provenance a workflow node records, and an export that fails part way still records the files it had already written. Show a tool's result value in the automation window; without it an inspect call reported success and displayed no readings at all. --- crates/app/src/ui/batch_workflow.rs | 36 +- crates/core/src/automation/mod.rs | 3 + crates/core/src/automation/properties.rs | 567 +++++++++++ .../src/automation/properties_audit_tests.rs | 325 +++++++ .../core/src/automation/properties_tests.rs | 489 ++++++++++ crates/core/src/automation/registry.rs | 55 ++ crates/core/src/automation/resources.rs | 18 +- crates/core/src/automation/tests.rs | 81 +- crates/core/src/automation/tool_executors.rs | 658 +++++++++++++ crates/core/src/automation/tools.rs | 899 ++++++------------ crates/core/src/automation/types.rs | 20 +- crates/core/src/automation/workflow.rs | 15 +- crates/core/src/properties/contour.rs | 22 +- crates/core/src/properties/mod.rs | 23 +- crates/core/src/properties/model.rs | 11 +- crates/core/src/properties/service.rs | 25 +- crates/core/src/properties/tests.rs | 34 +- crates/core/tests/slice2d.rs | 420 +++++++- docs/src/content/docs/guides/automation.md | 68 +- .../src/content/docs/guides/contour-levels.md | 43 +- .../content/docs/reference/command-palette.md | 9 +- .../content/docs/zh-cn/guides/automation.md | 60 +- .../docs/zh-cn/guides/contour-levels.md | 30 +- .../docs/zh-cn/reference/command-palette.md | 8 +- 24 files changed, 3252 insertions(+), 667 deletions(-) create mode 100644 crates/core/src/automation/properties.rs create mode 100644 crates/core/src/automation/properties_audit_tests.rs create mode 100644 crates/core/src/automation/properties_tests.rs create mode 100644 crates/core/src/automation/tool_executors.rs 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*（降低最低层）。 设置：