scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-06 02:48:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

refactor: collapse the split validation paths

Browse source 
Follow-up cleanup on the catalog-free validation split. Same behavior,
fewer parallel code paths.

- Make the catalog an explicit `Option<&Catalog>` on `pipeline::validate`
  instead of a `validate` / `validate_with_catalog` pair, so each call
  site states whether catalog rules run.
- Collapse `preprocess_and_validate`, `preprocess_and_validate_structural`,
  and `preprocess` into one function that takes `TransformOptions`. Its
  `model_resolution` field is now the single source of truth for catalog
  awareness, which drops a 12-argument signature and the
  `too_many_arguments` allow.
- Replace the duplicated resolve-and-preprocess block in
  `operations::validate` with one `validate_in_scope` helper, and drop the
  HashSet -> Vec -> HashSet round trip on the catalog path.
- Extract `configured_default_provider`, previously duplicated between
  `operations::create` and `operations::validate`.
- Delete `validate_manifest_with_environment_defaults`, which had no
  callers outside its own module.
- Share the `server-model.fabro` fixture between the two CLI tests instead
  of inlining it twice. The validate test now asserts the rendered output
  through the usual snapshot helper, which also removes a hand-rolled
  `std::fs::write` and its clippy allow.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-07-28 14:13:45 -04:00
parent 0b24649e76
commit 8592a34968
No known key found for this signature in database
9 changed files with 191 additions and 290 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
lib/apps/fabro-cli/tests/it/cmd/create.rs
View file

@ -114,25 +114,13 @@ fn create_defers_provider_validation_to_the_server() {
.header("Content-Type", "application/json")
.body(run_status_response(run_id.as_str(), "submitted").to_string());
});
let workflow_path = context.temp_dir.join("server-model.fabro");
context.write_temp(
"server-model.fabro",
r#"digraph ServerModel {
graph [goal="Use a server-owned model"]
start [shape=Mdiamond]
work [prompt="Do work", model="private-model", provider="server-only"]
exit [shape=Msquare]
start -> work -> exit
}"#,
);
let output = context
.create_cmd()
.args([
"--server",
&format!("{}/api/v1", server.base_url()),
"--dry-run",
workflow_path.to_str().unwrap(),
fixture("server-model.fabro").to_str().unwrap(),
])
.output()
.expect("command should execute");

43
lib/apps/fabro-cli/tests/it/cmd/validate.rs
View file

@ -69,37 +69,22 @@ fn simple_does_not_connect_to_configured_server() {
);
}
/// Offline validation has no model catalog, so a model or provider the server
/// owns must pass rather than be reported as unknown.
#[test]
#[expect(
clippy::disallowed_methods,
reason = "sync CLI test writes one workflow fixture before spawning the subprocess"
)]
fn server_owned_provider_is_not_rejected_by_offline_validation() {
let cli = LightweightCli::new();
let workflow = cli.home().join("server-model.fabro");
std::fs::write(
&workflow,
r#"digraph ServerModel {
graph [goal="Use a server-owned model"]
start [shape=Mdiamond]
work [prompt="Do work", model="private-model", provider="server-only"]
exit [shape=Msquare]
start -> work -> exit
}"#,
)
.expect("workflow fixture should be written");
let mut cmd = cli.command();
cmd.env("FABRO_SERVER", "http://127.0.0.1:9")
.arg("validate")
.arg(&workflow);
let output = cmd.output().expect("validate should execute");
assert!(
output.status.success(),
"offline validation should leave provider availability to the server\nstdout:\n{}\nstderr:\n{}",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr),
);
let context = test_context!();
let mut cmd = context.validate();
cmd.arg(fixture("server-model.fabro"));
fabro_snapshot!(context.filters(), cmd, @"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Workflow: ServerModel (3 nodes, 2 edges)
Graph: [FIXTURES]/server-model.fabro
Validation: OK
");
}
#[test]

35
lib/apps/fabro-server/src/manifest_validation.rs
View file

@ -3,53 +3,46 @@ use std::sync::Arc;
use anyhow::Result;
use fabro_api::types;
use fabro_config::{EnvironmentLayer, MergeMap, RunLayer};
use fabro_config::RunLayer;
use fabro_model::Catalog;
use fabro_workflow::pipeline::TEMPLATE_UNDEFINED_VARIABLE_RULE;
use crate::run_manifest;
/// Validate a manifest without a model catalog. Model and provider
/// availability is deferred to the server, which owns the catalog.
pub fn validate_manifest(
manifest_run_defaults: &RunLayer,
manifest: &types::RunManifest,
) -> Result<types::ValidateResponse> {
validate_manifest_with_environment_defaults(
manifest_run_defaults,
&fabro_environment::seeded_catalog_layer(),
manifest,
)
let prepared = prepare(manifest_run_defaults, manifest)?;
let validated = run_manifest::validate_prepared_manifest_structural(&prepared)
.map_err(anyhow::Error::new)?;
Ok(run_manifest::validate_response(&prepared, &validated))
}
/// Validate a manifest including the catalog-backed model and provider rules.
pub fn validate_manifest_with_catalog(
manifest_run_defaults: &RunLayer,
manifest: &types::RunManifest,
catalog: &Arc<Catalog>,
) -> Result<types::ValidateResponse> {
let prepared = run_manifest::prepare_manifest_with_environment_defaults(
manifest_run_defaults,
&fabro_environment::seeded_catalog_layer(),
&HashMap::new(),
manifest,
)?;
let prepared = prepare(manifest_run_defaults, manifest)?;
let validated =
run_manifest::validate_prepared_manifest(&prepared, catalog).map_err(anyhow::Error::new)?;
Ok(run_manifest::validate_response(&prepared, &validated))
}
pub fn validate_manifest_with_environment_defaults(
fn prepare(
manifest_run_defaults: &RunLayer,
manifest_environment_defaults: &MergeMap<EnvironmentLayer>,
manifest: &types::RunManifest,
) -> Result<types::ValidateResponse> {
let prepared = run_manifest::prepare_manifest_with_environment_defaults(
) -> Result<run_manifest::PreparedManifest> {
run_manifest::prepare_manifest_with_environment_defaults(
manifest_run_defaults,
manifest_environment_defaults,
&fabro_environment::seeded_catalog_layer(),
&HashMap::new(),
manifest,
)?;
let validated = run_manifest::validate_prepared_manifest_structural(&prepared)
.map_err(anyhow::Error::new)?;
Ok(run_manifest::validate_response(&prepared, &validated))
)
}
pub fn promote_template_undefined_variables_to_errors(response: &mut types::ValidateResponse) {

221
lib/components/fabro-workflow/src/operations/create.rs
View file

@ -24,13 +24,11 @@ use crate::error::Error;
use crate::event::{Event, append_event, to_run_event_at};
use crate::file_resolver::FileResolver;
use crate::pipeline::types::PersistOptions;
use crate::pipeline::{
self, ModelResolutionOptions, Persisted, TransformOptions, Transformed, Validated,
};
use crate::pipeline::{self, ModelResolutionOptions, Persisted, TransformOptions, Validated};
use crate::records::RunSpec;
use crate::run_lookup::default_scratch_base;
use crate::run_materialization::materialize_run;
use crate::transforms::{RenderMode, Transform};
use crate::transforms::RenderMode;
use crate::workflow_bundle::{RunDefinition, WorkflowBundle};
#[derive(Clone, Debug)]
@ -295,28 +293,20 @@ fn create_from_source(
file_resolver: Option<Arc<dyn FileResolver>>,
goal_override: Option<&str>,
) -> Result<Persisted, Error> {
let template_context = template_context(Some(&options.settings), vars);
let mut validated = preprocess_and_validate(
dot_source,
options.source_name.clone(),
let mut validated = preprocess_and_validate(dot_source, goal_override, &TransformOptions {
current_dir,
file_resolver,
Vec::new(),
template_context,
goal_override,
RenderMode::Structural,
options
.settings
.run
.model
.provider
.as_deref()
.filter(|provider| !provider.is_empty())
.map(ProviderId::new),
&options.configured_providers,
false,
&options.catalog,
)?;
template_context: template_context(Some(&options.settings), vars),
source_name: options.source_name.clone(),
render_mode: RenderMode::Structural,
custom_transforms: Vec::new(),
model_resolution: Some(ModelResolutionOptions {
catalog: Arc::clone(&options.catalog),
default_provider: configured_default_provider(&options.settings),
eligible_providers: options.configured_providers.iter().cloned().collect(),
catalog_fallback: false,
}),
})?;
validated.promote_template_undefined_variables_to_errors();
if validated.has_errors() {
@ -328,96 +318,37 @@ fn create_from_source(
persist_validated(validated, options)
}
/// Parse, transform, and validate `dot_source`.
///
/// `options.model_resolution` drives both halves of catalog awareness: it
/// selects concrete models during TRANSFORM and enables the catalog-backed
/// lint rules during VALIDATE. `None` leaves authored model and provider
/// selectors untouched for offline structural validation.
pub(super) fn preprocess_and_validate(
dot_source: &str,
source_name: Option<String>,
current_dir: Option<PathBuf>,
file_resolver: Option<Arc<dyn FileResolver>>,
custom_transforms: Vec<Box<dyn Transform>>,
template_context: TemplateContext,
goal_override: Option<&str>,
render_mode: RenderMode,
default_provider: Option<ProviderId>,
eligible_providers: &[ProviderId],
catalog_fallback: bool,
catalog: &Arc<Catalog>,
options: &TransformOptions,
) -> Result<Validated, Error> {
let model_resolution = ModelResolutionOptions {
catalog: Arc::clone(catalog),
default_provider,
eligible_providers: eligible_providers.iter().cloned().collect(),
catalog_fallback,
};
let transformed = preprocess(
dot_source,
source_name,
current_dir,
file_resolver,
custom_transforms,
template_context,
goal_override,
render_mode,
Some(model_resolution),
)?;
Ok(pipeline::validate_with_catalog(
transformed,
catalog.as_ref(),
&[],
))
}
pub(super) fn preprocess_and_validate_structural(
dot_source: &str,
source_name: Option<String>,
current_dir: Option<PathBuf>,
file_resolver: Option<Arc<dyn FileResolver>>,
custom_transforms: Vec<Box<dyn Transform>>,
template_context: TemplateContext,
goal_override: Option<&str>,
render_mode: RenderMode,
) -> Result<Validated, Error> {
let transformed = preprocess(
dot_source,
source_name,
current_dir,
file_resolver,
custom_transforms,
template_context,
goal_override,
render_mode,
None,
)?;
Ok(pipeline::validate(transformed, &[]))
}
#[expect(
clippy::too_many_arguments,
reason = "pipeline stages have distinct source, rendering, and model-resolution inputs"
)]
fn preprocess(
dot_source: &str,
source_name: Option<String>,
current_dir: Option<PathBuf>,
file_resolver: Option<Arc<dyn FileResolver>>,
custom_transforms: Vec<Box<dyn Transform>>,
template_context: TemplateContext,
goal_override: Option<&str>,
render_mode: RenderMode,
model_resolution: Option<ModelResolutionOptions>,
) -> Result<Transformed, Error> {
let mut parsed = pipeline::parse(dot_source)?;
apply_goal_override(&mut parsed.graph, goal_override);
let transformed = pipeline::transform(parsed, &TransformOptions {
current_dir,
file_resolver,
template_context,
source_name,
render_mode,
custom_transforms,
model_resolution,
})?;
Ok(transformed)
let transformed = pipeline::transform(parsed, options)?;
let catalog = options
.model_resolution
.as_ref()
.map(|resolution| resolution.catalog.as_ref());
Ok(pipeline::validate(transformed, catalog, &[]))
}
/// The workflow-level default provider, treating an empty setting as unset.
pub(super) fn configured_default_provider(settings: &WorkflowSettings) -> Option<ProviderId> {
settings
.run
.model
.provider
.as_deref()
.filter(|provider| !provider.is_empty())
.map(ProviderId::new)
}
pub(super) fn template_context(
@ -526,6 +457,7 @@ mod tests {
use super::*;
use crate::operations::{ValidateInput, validate, validate_with_catalog};
use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE};
use crate::transforms::Transform;
use crate::workflow_bundle::BundledWorkflow;
fn memory_store() -> Arc<Database> {
Arc::new(Database::new(
@ -637,21 +569,40 @@ reasoning = false
fn validate_dot_with_vars(dot_source: &str, vars: HashMap<String, String>) -> Validated {
preprocess_and_validate(
dot_source,
Some("workflow.fabro".to_string()),
Some(PathBuf::from(".")),
None,
Vec::new(),
template_context(Some(&WorkflowSettings::default()), vars),
None,
RenderMode::Structural,
None,
&test_provider_ids(),
false,
&test_catalog(),
&test_transform_options(
PathBuf::from("."),
None,
RenderMode::Structural,
template_context(Some(&WorkflowSettings::default()), vars),
),
)
.unwrap()
}
/// Catalog-backed TRANSFORM options for the built-in test catalog.
fn test_transform_options(
current_dir: PathBuf,
file_resolver: Option<Arc<dyn FileResolver>>,
render_mode: RenderMode,
template_context: TemplateContext,
) -> TransformOptions {
TransformOptions {
current_dir: Some(current_dir),
file_resolver,
template_context,
source_name: Some("workflow.fabro".to_string()),
render_mode,
custom_transforms: Vec::new(),
model_resolution: Some(ModelResolutionOptions {
catalog: test_catalog(),
default_provider: None,
eligible_providers: test_provider_ids().into_iter().collect(),
catalog_fallback: false,
}),
}
}
const MINIMAL_DOT: &str = r#"digraph Test {
graph [goal="Build feature"]
start [shape=Mdiamond]
@ -808,17 +759,13 @@ reasoning = false
let result = preprocess_and_validate(
dot,
Some("workflow.fabro".to_string()),
Some(PathBuf::from(".")),
None,
Vec::new(),
template_context(Some(&WorkflowSettings::default()), HashMap::new()),
None,
RenderMode::Strict,
None,
&test_provider_ids(),
false,
&test_catalog(),
&test_transform_options(
PathBuf::from("."),
None,
RenderMode::Strict,
template_context(Some(&WorkflowSettings::default()), HashMap::new()),
),
);
let Err(err) = result else {
panic!("expected strict mode to hard-fail on unbound inline prompt");
@ -845,19 +792,15 @@ reasoning = false
let result = preprocess_and_validate(
dot,
Some("workflow.fabro".to_string()),
Some(dir.path().to_path_buf()),
Some(Arc::new(crate::file_resolver::FilesystemFileResolver::new(
None,
))),
Vec::new(),
template_context(Some(&WorkflowSettings::default()), HashMap::new()),
None,
RenderMode::Strict,
None,
&test_provider_ids(),
false,
&test_catalog(),
&test_transform_options(
dir.path().to_path_buf(),
Some(Arc::new(crate::file_resolver::FilesystemFileResolver::new(
None,
))),
RenderMode::Strict,
template_context(Some(&WorkflowSettings::default()), HashMap::new()),
),
);
let Err(err) = result else {
panic!("expected strict mode to hard-fail on unbound imported prompt");

114
lib/components/fabro-workflow/src/operations/validate.rs
View file

@ -1,17 +1,15 @@
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::path::PathBuf;
use std::sync::Arc;
use fabro_model::{Catalog, ProviderId};
use fabro_types::WorkflowSettings;
use super::create::{
preprocess_and_validate, preprocess_and_validate_structural, template_context,
};
use super::create::{configured_default_provider, preprocess_and_validate, template_context};
use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow};
use crate::error::Error;
use crate::operations::RenderMode;
use crate::pipeline::Validated;
use crate::pipeline::{ModelResolutionOptions, TransformOptions, Validated};
use crate::transforms::Transform;
pub struct ValidateInput {
@ -24,39 +22,25 @@ pub struct ValidateInput {
pub custom_transforms: Vec<Box<dyn Transform>>,
}
/// Which providers catalog-backed model resolution may select from. The
/// workflow's own default provider is read from the resolved settings, so it
/// is not part of the caller's request.
struct CatalogScope<'a> {
catalog: &'a Arc<Catalog>,
eligible_providers: HashSet<ProviderId>,
/// Fall back to the full catalog when the eligible providers cannot
/// supply a requested model, instead of erroring.
catalog_fallback: bool,
}
/// Parse, transform, and structurally validate a DOT source string without a
/// model catalog.
/// model catalog. Model and provider availability is left to the caller that
/// owns a catalog — typically the server.
///
/// Returns `Validated` even when validation produced errors. Call
/// `validated.raise_on_errors()` if the caller wants to fail fast.
pub fn validate(input: ValidateInput) -> Result<Validated, Error> {
let ValidateInput {
workflow,
settings,
vars,
cwd,
custom_transforms,
} = input;
let resolved = resolve_workflow(ResolveWorkflowInput {
workflow,
settings,
cwd,
})
.map_err(|err| Error::Parse(err.to_string()))?;
preprocess_and_validate_structural(
&resolved.raw_source,
resolved
.dot_path
.as_ref()
.map(|path| path.display().to_string()),
resolved.current_dir,
resolved.file_resolver,
custom_transforms,
template_context(Some(&resolved.settings), vars),
resolved.goal_override.as_deref(),
RenderMode::Structural,
)
validate_in_scope(input, None)
}
/// Parse, transform, and validate a DOT source string against `catalog`.
@ -64,8 +48,14 @@ pub fn validate_with_catalog(
input: ValidateInput,
catalog: &Arc<Catalog>,
) -> Result<Validated, Error> {
let eligible_providers = catalog.all_provider_ids().into_iter().collect::<Vec<_>>();
validate_with_eligible_providers(input, catalog, &eligible_providers, false)
validate_in_scope(
input,
Some(CatalogScope {
catalog,
eligible_providers: catalog.all_provider_ids(),
catalog_fallback: false,
}),
)
}
/// Parse, transform, and validate, resolving models against the ready
@ -76,14 +66,19 @@ pub fn validate_with_ready_providers(
catalog: &Arc<Catalog>,
ready_providers: &[ProviderId],
) -> Result<Validated, Error> {
validate_with_eligible_providers(input, catalog, ready_providers, true)
validate_in_scope(
input,
Some(CatalogScope {
catalog,
eligible_providers: ready_providers.iter().cloned().collect(),
catalog_fallback: true,
}),
)
}
fn validate_with_eligible_providers(
fn validate_in_scope(
input: ValidateInput,
catalog: &Arc<Catalog>,
eligible_providers: &[ProviderId],
catalog_fallback: bool,
scope: Option<CatalogScope<'_>>,
) -> Result<Validated, Error> {
let ValidateInput {
workflow,
@ -99,28 +94,27 @@ fn validate_with_eligible_providers(
})
.map_err(|err| Error::Parse(err.to_string()))?;
let model_resolution = scope.map(|scope| ModelResolutionOptions {
catalog: Arc::clone(scope.catalog),
default_provider: configured_default_provider(&resolved.settings),
eligible_providers: scope.eligible_providers,
catalog_fallback: scope.catalog_fallback,
});
preprocess_and_validate(
&resolved.raw_source,
resolved
.dot_path
.as_ref()
.map(|path| path.display().to_string()),
resolved.current_dir,
resolved.file_resolver,
custom_transforms,
template_context(Some(&resolved.settings), vars),
resolved.goal_override.as_deref(),
RenderMode::Structural,
resolved
.settings
.run
.model
.provider
.as_deref()
.filter(|provider| !provider.is_empty())
.map(fabro_model::ProviderId::new),
eligible_providers,
catalog_fallback,
catalog,
&TransformOptions {
current_dir: resolved.current_dir,
file_resolver: resolved.file_resolver,
template_context: template_context(Some(&resolved.settings), vars),
source_name: resolved
.dot_path
.as_ref()
.map(|path| path.display().to_string()),
render_mode: RenderMode::Structural,
custom_transforms,
model_resolution,
},
)
}

2
lib/components/fabro-workflow/src/pipeline/mod.rs
View file

@ -26,4 +26,4 @@ pub use types::{
ModelResolutionOptions, Parsed, Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec,
TEMPLATE_UNDEFINED_VARIABLE_RULE, TransformOptions, Transformed, Validated,
};
pub use validate::{validate, validate_with_catalog};
pub use validate::validate;

36
lib/components/fabro-workflow/src/pipeline/validate.rs
View file

@ -1,28 +1,19 @@
use fabro_model::Catalog;
use fabro_validate::LintRule;
use super::types::{Transformed, Validated};
/// VALIDATE phase: run catalog-free lint rules against the transformed graph.
/// VALIDATE phase: run lint rules against the transformed graph.
///
/// Catalog-backed rules (model and provider availability) run only when
/// `catalog` is `Some`. Offline callers pass `None` so a workflow naming a
/// server-owned model is left for the server to judge.
///
/// **Infallible.** Always returns `Validated` with diagnostics. Caller decides
/// whether to fail via `validated.raise_on_errors()`.
pub fn validate(transformed: Transformed, extra_rules: &[&dyn LintRule]) -> Validated {
let Transformed {
graph,
source,
mut diagnostics,
} = transformed;
diagnostics.extend(fabro_validate::validate(&graph, extra_rules));
Validated::new(graph, source, diagnostics)
}
/// VALIDATE phase: run catalog-free and catalog-backed lint rules.
///
/// **Infallible.** Always returns `Validated` with diagnostics. Caller decides
/// whether to fail via `validated.raise_on_errors()`.
pub fn validate_with_catalog(
pub fn validate(
transformed: Transformed,
catalog: &fabro_model::Catalog,
catalog: Option<&Catalog>,
extra_rules: &[&dyn LintRule],
) -> Validated {
let Transformed {
@ -30,11 +21,10 @@ pub fn validate_with_catalog(
source,
mut diagnostics,
} = transformed;
diagnostics.extend(fabro_validate::validate_with_catalog(
&graph,
catalog,
extra_rules,
));
diagnostics.extend(match catalog {
Some(catalog) => fabro_validate::validate_with_catalog(&graph, catalog, extra_rules),
None => fabro_validate::validate(&graph, extra_rules),
});
Validated::new(graph, source, diagnostics)
}
@ -57,7 +47,7 @@ mod tests {
model_resolution: None,
})
.unwrap();
validate(transformed, &[])
validate(transformed, None, &[])
}
#[test]

6
lib/components/fabro-workflow/tests/it/integration.rs
View file

@ -4853,9 +4853,7 @@ async fn manager_loop_child_workflow_e2e() {
#[tokio::test]
async fn import_e2e_through_engine() {
use fabro_workflow::pipeline::{
ModelResolutionOptions, TransformOptions, transform, validate_with_catalog,
};
use fabro_workflow::pipeline::{ModelResolutionOptions, TransformOptions, transform, validate};
let dir = tempfile::tempdir().unwrap();
let catalog = std::sync::Arc::new(
@ -4910,7 +4908,7 @@ async fn import_e2e_through_engine() {
model_resolution: Some(ModelResolutionOptions::new(std::sync::Arc::clone(&catalog))),
})
.unwrap();
let validated = validate_with_catalog(transformed, catalog.as_ref(), &[]);
let validated = validate(transformed, Some(catalog.as_ref()), &[]);
validated
.raise_on_errors()
.expect("validation should pass after imports expand");

10
test/server-model.fabro Normal file
View file

@ -0,0 +1,10 @@
digraph ServerModel {
graph [goal="Use a server-owned model"]
start [shape=Mdiamond, label="Start"]
exit [shape=Msquare, label="Exit"]
work [label="Work", prompt="Do work", model="private-model", provider="server-only"]
start -> work -> exit
}