mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-11 22:53:00 +00:00
Add prepare_with_file_inlining API to prevent validate/run divergence
FileInliningTransform was treated as a "custom" transform that every caller had to remember to register manually. This caused arc validate to break and left latent bugs in run_from_branch and SubWorkflowHandler. Add prepare_with_file_inlining() and prepare_from_file() to WorkflowBuilder so file inlining is a built-in concern. Rename prepare_workflow to prepare_from_source for clarity. Fix the SubWorkflowHandler to use prepare_from_file when reading from stack.child_dotfile paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
3fdd1ec746
commit
3c476772e7
4 changed files with 84 additions and 46 deletions
|
|
@ -249,11 +249,8 @@ pub async fn run_command(
|
|||
None => source,
|
||||
};
|
||||
let dot_dir = dot_path.parent().unwrap_or(std::path::Path::new("."));
|
||||
let mut builder = WorkflowBuilder::new();
|
||||
builder.register_transform(Box::new(crate::transform::FileInliningTransform::new(
|
||||
dot_dir.to_path_buf(),
|
||||
)));
|
||||
let (mut graph, diagnostics) = builder.prepare(&source)?;
|
||||
let (mut graph, diagnostics) =
|
||||
WorkflowBuilder::new().prepare_with_file_inlining(&source, dot_dir)?;
|
||||
let toml_goal = run_cfg.as_ref().and_then(|c| c.goal.as_deref());
|
||||
apply_goal_override(&mut graph, args.goal.as_deref(), toml_goal);
|
||||
|
||||
|
|
@ -1009,13 +1006,13 @@ async fn run_from_branch(
|
|||
.ok_or_else(|| anyhow::anyhow!("no graph.dot found on metadata branch for run {run_id}"))?;
|
||||
|
||||
// If --pipeline was also provided, use it instead (allows overriding)
|
||||
let source = if let Some(ref workflow_path) = args.workflow {
|
||||
super::read_dot_file(workflow_path)?
|
||||
let (mut graph, diagnostics) = if let Some(ref workflow_path) = args.workflow {
|
||||
let source = super::read_dot_file(workflow_path)?;
|
||||
let dot_dir = workflow_path.parent().unwrap_or(std::path::Path::new("."));
|
||||
crate::workflow::WorkflowBuilder::new().prepare_with_file_inlining(&source, dot_dir)?
|
||||
} else {
|
||||
source
|
||||
crate::workflow::WorkflowBuilder::new().prepare(&source)?
|
||||
};
|
||||
|
||||
let (mut graph, diagnostics) = crate::workflow::WorkflowBuilder::new().prepare(&source)?;
|
||||
apply_goal_override(&mut graph, args.goal.as_deref(), None);
|
||||
|
||||
eprintln!(
|
||||
|
|
|
|||
|
|
@ -2,9 +2,9 @@ use anyhow::bail;
|
|||
use arc_util::terminal::Styles;
|
||||
|
||||
use crate::validation::Severity;
|
||||
use crate::workflow::WorkflowBuilder;
|
||||
use crate::workflow::prepare_from_file;
|
||||
|
||||
use super::{print_diagnostics, read_dot_file, ValidateArgs};
|
||||
use super::{print_diagnostics, ValidateArgs};
|
||||
|
||||
/// Parse and validate a workflow file without executing it.
|
||||
///
|
||||
|
|
@ -12,13 +12,7 @@ use super::{print_diagnostics, read_dot_file, ValidateArgs};
|
|||
///
|
||||
/// Returns an error if the file cannot be read, parsed, or has validation errors.
|
||||
pub fn validate_command(args: &ValidateArgs, styles: &Styles) -> anyhow::Result<()> {
|
||||
let source = read_dot_file(&args.workflow)?;
|
||||
let dot_dir = args.workflow.parent().unwrap_or(std::path::Path::new("."));
|
||||
let mut builder = WorkflowBuilder::new();
|
||||
builder.register_transform(Box::new(crate::transform::FileInliningTransform::new(
|
||||
dot_dir.to_path_buf(),
|
||||
)));
|
||||
let (graph, diagnostics) = builder.prepare(&source)?;
|
||||
let (graph, diagnostics) = prepare_from_file(&args.workflow)?;
|
||||
|
||||
eprintln!(
|
||||
"{} ({} nodes, {} edges)",
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ use crate::engine::{RunConfig, WorkflowRunEngine};
|
|||
use crate::error::ArcError;
|
||||
use crate::graph::{Graph, Node};
|
||||
use crate::outcome::{Outcome, StageStatus};
|
||||
use crate::workflow::prepare_workflow;
|
||||
use crate::workflow::{prepare_from_file, prepare_from_source};
|
||||
|
||||
use super::{EngineServices, Handler};
|
||||
|
||||
|
|
@ -42,23 +42,31 @@ fn parse_duration_str(s: &str) -> Duration {
|
|||
Duration::from_secs(45)
|
||||
}
|
||||
|
||||
/// Read DOT source from node attributes: inline `stack.child_dot_source` or
|
||||
/// file path `stack.child_dotfile`.
|
||||
fn read_child_dot(node: &Node) -> Result<String, ArcError> {
|
||||
/// Parse a child workflow graph from node attributes: inline `stack.child_dot_source`
|
||||
/// (no file inlining) or file path `stack.child_dotfile` (with file inlining).
|
||||
fn parse_child_graph(node: &Node) -> Result<Graph, ArcError> {
|
||||
if let Some(dot) = node
|
||||
.attrs
|
||||
.get("stack.child_dot_source")
|
||||
.and_then(|v| v.as_str())
|
||||
{
|
||||
return Ok(dot.to_string());
|
||||
return prepare_from_source(dot);
|
||||
}
|
||||
if let Some(path) = node
|
||||
.attrs
|
||||
.get("stack.child_dotfile")
|
||||
.and_then(|v| v.as_str())
|
||||
{
|
||||
return std::fs::read_to_string(path)
|
||||
.map_err(|e| ArcError::handler(format!("Failed to read child dotfile {path}: {e}")));
|
||||
let (graph, diagnostics) = prepare_from_file(std::path::Path::new(path))?;
|
||||
let errors: Vec<&crate::validation::Diagnostic> = diagnostics
|
||||
.iter()
|
||||
.filter(|d| d.severity == crate::validation::Severity::Error)
|
||||
.collect();
|
||||
if !errors.is_empty() {
|
||||
let messages: Vec<String> = errors.iter().map(|d| d.message.clone()).collect();
|
||||
return Err(ArcError::Validation(messages.join("; ")));
|
||||
}
|
||||
return Ok(graph);
|
||||
}
|
||||
Err(ArcError::handler("No child DOT source".to_string()))
|
||||
}
|
||||
|
|
@ -113,13 +121,8 @@ impl Handler for SubWorkflowHandler {
|
|||
.and_then(|v| v.as_str())
|
||||
.unwrap_or("");
|
||||
|
||||
// Read and parse child DOT source
|
||||
let dot_source = match read_child_dot(node) {
|
||||
Ok(s) => s,
|
||||
Err(e) => return Ok(Outcome::fail_classify(e.to_string())),
|
||||
};
|
||||
|
||||
let child_graph = match prepare_workflow(&dot_source) {
|
||||
// Read and parse child workflow graph
|
||||
let child_graph = match parse_child_graph(node) {
|
||||
Ok(g) => g,
|
||||
Err(e) => {
|
||||
return Ok(Outcome::fail_classify(format!(
|
||||
|
|
|
|||
|
|
@ -1,6 +1,10 @@
|
|||
use std::path::Path;
|
||||
|
||||
use crate::error::ArcError;
|
||||
use crate::graph::Graph;
|
||||
use crate::transform::{StylesheetApplicationTransform, Transform, VariableExpansionTransform};
|
||||
use crate::transform::{
|
||||
FileInliningTransform, StylesheetApplicationTransform, Transform, VariableExpansionTransform,
|
||||
};
|
||||
use crate::validation::{self, Diagnostic};
|
||||
|
||||
/// Builder for configuring and executing a workflow preparation.
|
||||
|
|
@ -29,12 +33,39 @@ impl WorkflowBuilder {
|
|||
///
|
||||
/// Returns an error if parsing or validation fails.
|
||||
pub fn prepare(&self, dot_source: &str) -> Result<(Graph, Vec<Diagnostic>), ArcError> {
|
||||
self.prepare_inner(dot_source, None)
|
||||
}
|
||||
|
||||
/// Prepare a workflow with file inlining: parse DOT, apply built-in transforms
|
||||
/// including `FileInliningTransform`, then custom transforms, then validate.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns an error if parsing or validation fails.
|
||||
pub fn prepare_with_file_inlining(
|
||||
&self,
|
||||
dot_source: &str,
|
||||
base_dir: &Path,
|
||||
) -> Result<(Graph, Vec<Diagnostic>), ArcError> {
|
||||
self.prepare_inner(dot_source, Some(base_dir))
|
||||
}
|
||||
|
||||
fn prepare_inner(
|
||||
&self,
|
||||
dot_source: &str,
|
||||
base_dir: Option<&Path>,
|
||||
) -> Result<(Graph, Vec<Diagnostic>), ArcError> {
|
||||
let mut graph = crate::parser::parse(dot_source)?;
|
||||
|
||||
// Built-in transforms (PreambleTransform moved to engine execution time)
|
||||
VariableExpansionTransform.apply(&mut graph);
|
||||
StylesheetApplicationTransform.apply(&mut graph);
|
||||
|
||||
// File inlining when base_dir is provided
|
||||
if let Some(dir) = base_dir {
|
||||
FileInliningTransform::new(dir.to_path_buf()).apply(&mut graph);
|
||||
}
|
||||
|
||||
// Custom transforms
|
||||
for transform in &self.transforms {
|
||||
transform.apply(&mut graph);
|
||||
|
|
@ -51,12 +82,25 @@ impl Default for WorkflowBuilder {
|
|||
}
|
||||
}
|
||||
|
||||
/// Convenience function: parse DOT, apply built-in transforms, validate, return graph.
|
||||
/// Convenience: read a DOT file, apply built-in transforms including file inlining, validate.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns an error if the file cannot be read, parsed, or validated.
|
||||
pub fn prepare_from_file(path: &Path) -> Result<(Graph, Vec<Diagnostic>), ArcError> {
|
||||
let source = std::fs::read_to_string(path)
|
||||
.map_err(|e| ArcError::Parse(format!("Failed to read {}: {e}", path.display())))?;
|
||||
let dot_dir = path.parent().unwrap_or(Path::new("."));
|
||||
WorkflowBuilder::new().prepare_with_file_inlining(&source, dot_dir)
|
||||
}
|
||||
|
||||
/// Convenience: parse DOT source (no file inlining), apply built-in transforms, validate.
|
||||
/// Returns the graph or an error if validation produces Error-severity diagnostics.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns an error if parsing fails or if validation produces Error-severity diagnostics.
|
||||
pub fn prepare_workflow(dot_source: &str) -> Result<Graph, ArcError> {
|
||||
pub fn prepare_from_source(dot_source: &str) -> Result<Graph, ArcError> {
|
||||
let builder = WorkflowBuilder::new();
|
||||
let (graph, diagnostics) = builder.prepare(dot_source)?;
|
||||
|
||||
|
|
@ -85,15 +129,15 @@ mod tests {
|
|||
}"#;
|
||||
|
||||
#[test]
|
||||
fn prepare_workflow_minimal() {
|
||||
let graph = prepare_workflow(MINIMAL_DOT).unwrap();
|
||||
fn prepare_from_source_minimal() {
|
||||
let graph = prepare_from_source(MINIMAL_DOT).unwrap();
|
||||
assert_eq!(graph.name, "Test");
|
||||
assert!(graph.find_start_node().is_some());
|
||||
assert!(graph.find_exit_node().is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prepare_workflow_applies_variable_expansion() {
|
||||
fn prepare_from_source_applies_variable_expansion() {
|
||||
let dot = r#"digraph Test {
|
||||
graph [goal="Fix bugs"]
|
||||
start [shape=Mdiamond]
|
||||
|
|
@ -101,7 +145,7 @@ mod tests {
|
|||
exit [shape=Msquare]
|
||||
start -> work -> exit
|
||||
}"#;
|
||||
let graph = prepare_workflow(dot).unwrap();
|
||||
let graph = prepare_from_source(dot).unwrap();
|
||||
let prompt = graph.nodes["work"]
|
||||
.attrs
|
||||
.get("prompt")
|
||||
|
|
@ -111,7 +155,7 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn prepare_workflow_applies_stylesheet() {
|
||||
fn prepare_from_source_applies_stylesheet() {
|
||||
let dot = r#"digraph Test {
|
||||
graph [goal="Test", model_stylesheet="* { llm_model: sonnet; }"]
|
||||
start [shape=Mdiamond]
|
||||
|
|
@ -119,7 +163,7 @@ mod tests {
|
|||
exit [shape=Msquare]
|
||||
start -> work -> exit
|
||||
}"#;
|
||||
let graph = prepare_workflow(dot).unwrap();
|
||||
let graph = prepare_from_source(dot).unwrap();
|
||||
assert_eq!(
|
||||
graph.nodes["work"].attrs.get("llm_model"),
|
||||
Some(&AttrValue::String("sonnet".into()))
|
||||
|
|
@ -127,18 +171,18 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn prepare_workflow_returns_error_on_invalid_dot() {
|
||||
let result = prepare_workflow("not a graph");
|
||||
fn prepare_from_source_returns_error_on_invalid_dot() {
|
||||
let result = prepare_from_source("not a graph");
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prepare_workflow_returns_error_on_validation_failure() {
|
||||
fn prepare_from_source_returns_error_on_validation_failure() {
|
||||
let dot = r#"digraph Test {
|
||||
graph [goal="Test"]
|
||||
work [label="Work"]
|
||||
}"#;
|
||||
let result = prepare_workflow(dot);
|
||||
let result = prepare_from_source(dot);
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue