diff --git a/lib/components/fabro-petri/Cargo.toml b/lib/components/fabro-petri/Cargo.toml index 1933209e3..3061d5f7a 100644 --- a/lib/components/fabro-petri/Cargo.toml +++ b/lib/components/fabro-petri/Cargo.toml @@ -39,7 +39,6 @@ async-trait.workspace = true serde.workspace = true serde_json.workspace = true sqlx.workspace = true -tempfile = "3" thiserror.workspace = true tokio.workspace = true tokio-util.workspace = true @@ -50,4 +49,5 @@ fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] fabro-llm = { path = "../fabro-llm", features = ["test-support"] } fabro-store = { path = "../fabro-store", features = ["test-support"] } petri_testkit.workspace = true +tempfile = "3" tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/lib/components/fabro-petri/README.md b/lib/components/fabro-petri/README.md index 9b8134b11..3a3761e85 100644 --- a/lib/components/fabro-petri/README.md +++ b/lib/components/fabro-petri/README.md @@ -23,10 +23,12 @@ Every adapter the integration plan describes lands here. and at execution: the Fabro frontend with the server's settings layer, the Attractor step kinds (real, or simulated for a dry run), the model client as the `PebbleClient` capability, the Fabro home. -- `check`: Petri compiles at create time. The workflow version's bundle is - materialized into a temporary directory (`Runtime::check` reads files from - disk), lowered with the run's inputs and launch, and the admitted graphs or - Petri's diagnostics come back in a shape the server maps onto Fabro's. +- `check`: Petri compiles at create time. The workflow version's bundle goes + into an in-memory file map (`frontend::MapFiles`, laid out as the bundle: + `workflow.toml` beside the workflow, `.fabro/project.toml` at the root), + `Runtime::check_source` lowers it with the run's inputs and launch, and the + admitted graphs or Petri's diagnostics come back in a shape the server maps + onto Fabro's. Nothing is written to disk. - `admission`: the admitted graphs in Fabro's blob store, named on the run spec as `RunEngine::Petri(PetriAdmission)`, verified by digest on load. - `engine`: a run executed by Petri, started from its admitted graphs or @@ -71,8 +73,11 @@ Integration tests live under `tests/`: is not on `PATH` (every run takes its scope's environment through it); the sandbox-plugins CI job requires them. - `check.rs` admits the `hello` bundle and round-trips its graph through - the blob store, binds the launch, and refuses an unknown attribute and, - with a model client over the test catalog, an unknown model + the blob store, binds the launch, admits a version whose `workflow.toml` + names `engine = "petri"`, reads the project settings from the map, and + refuses an unknown attribute, an unknown `[workflow]` key + (`unsupported.workflow_toml.key`, named in `workflow.toml`) and, with a + model client over the test catalog, an unknown model (`attractor.model.unknown`). No plugin is needed. - `sqlite_store.rs` runs Petri's store conformance suite (`petri_testkit::run_store::conformance`) against `SqliteRunStore`, plus the diff --git a/lib/components/fabro-petri/src/check.rs b/lib/components/fabro-petri/src/check.rs index 5b4022f09..29d57671d 100644 --- a/lib/components/fabro-petri/src/check.rs +++ b/lib/components/fabro-petri/src/check.rs @@ -1,14 +1,14 @@ //! Petri compiles: the create handler hands a workflow version's files, the -//! run's inputs and the launch to `Runtime::check`, and gets back either the -//! admitted graphs or Petri's diagnostics. +//! run's inputs and the launch to `Runtime::check_source`, and gets back +//! either the admitted graphs or Petri's diagnostics. //! -//! `Runtime::check` reads the workflow and its settings files from disk, so -//! the bundle is materialized into a temporary directory first, laid out the -//! way the Fabro frontend expects: the workflow file with `workflow.toml` -//! beside it under a bundle root that holds a `.fabro` directory (with -//! `.fabro/project.toml` when the caller has one). The directory is removed -//! when the check returns. An in-memory `FileSource` entry point on -//! `Runtime` would remove the round trip; that is a Petri follow-up. +//! The version's files never touch the disk. They go into a +//! `frontend::MapFiles` map laid out the way the Fabro frontend expects a +//! bundle: every file at its bundle-relative path, so `workflow.toml` sits +//! beside the workflow file, and `.fabro/project.toml` at the root when the +//! caller has one. The frontend reads the settings files and `@file` +//! references from that map, and every diagnostic names the bundle-relative +//! path the map holds the file under. //! //! The launch binds the compile variables the Fabro frontend reads: //! `petri.launch_model` and `petri.launch_provider` as the model default @@ -17,12 +17,11 @@ //! and the run starts from an empty workspace. use std::collections::BTreeMap; -use std::io; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use petri_runtime::LoadError; use petri_runtime::frontend::{ - self, CompileInputs, LAUNCH_MODEL_VAR, LAUNCH_PROVIDER_VAR, REPOSITORY_VAR, Severity, + self, CompileInputs, LAUNCH_MODEL_VAR, LAUNCH_PROVIDER_VAR, MapFiles, REPOSITORY_VAR, Severity, }; use petri_runtime::ir::Graph; use serde::{Deserialize, Serialize}; @@ -30,13 +29,8 @@ use serde_json::Value; use crate::runtime::RuntimeSpec; -/// The directory under the temporary bundle root the version's files land -/// in. Its parent holds `.fabro`, so the Fabro frontend takes the parent as -/// the bundle root. -const BUNDLE_DIR: &str = "bundle"; - /// The project settings file the Fabro frontend reads at the bundle root. -const PROJECT_FILE: &str = ".fabro/project.toml"; +const PROJECT_FILE: &str = petri_frontend_fabro::PROJECT_FILE; /// One workflow bundle to check: its files by bundle-relative path. #[derive(Clone, Debug, Default)] @@ -48,9 +42,23 @@ pub struct Bundle { /// The workflow file to check, one of `files`. pub entrypoint: String, /// `.fabro/project.toml` at the bundle root, when the caller has one. + /// It takes that path in the map, over a bundle file of the same name. pub project_toml: Option, } +impl Bundle { + /// The bundle as the Fabro frontend reads it: every file at its + /// bundle-relative path, and the project settings at + /// `.fabro/project.toml`. + fn files(&self) -> MapFiles { + let mut files = self.files.clone(); + if let Some(project) = &self.project_toml { + files.insert(PROJECT_FILE.to_string(), project.clone()); + } + MapFiles(files) + } +} + /// What the launch binds below the file layers. #[derive(Clone, Debug, Default)] pub struct Launch { @@ -111,38 +119,33 @@ pub enum CheckError { /// included; at least one is an error. #[error("Petri refused the workflow with {} diagnostic(s)", .0.len())] Rejected(Vec), - /// The bundle could not be materialized for the check. - #[error("could not materialize the workflow bundle at `{path}`")] - Materialize { - path: PathBuf, - #[source] - source: io::Error, - }, - /// The bundle's entrypoint is not one of its files, or no frontend - /// claims it. + /// The bundle's entrypoint is not one of its files. + #[error("the workflow entrypoint `{entrypoint}` is not one of the bundle's files")] + MissingEntrypoint { entrypoint: String }, + /// No frontend claims the bundle's entrypoint. #[error("the workflow could not be loaded")] Load(#[source] LoadError), } -/// Materialize the bundle, run `Runtime::check`, and hand back the admitted -/// graphs or the diagnostics. Blocking: it reads and writes files and -/// lowers the graph, so a server calls it from its blocking pool. +/// Run `Runtime::check_source` over the bundle, and hand back the admitted +/// graphs or the diagnostics. Blocking: it lowers the graph and runs the +/// admission passes synchronously, so a server calls it from its blocking +/// pool. pub fn check(request: &CheckRequest) -> Result { - let root = tempfile::tempdir().map_err(|source| CheckError::Materialize { - path: std::env::temp_dir(), - source, - })?; - let workflow = materialize(root.path(), &request.bundle)?; + let bundle = &request.bundle; + let text = + bundle + .files + .get(&bundle.entrypoint) + .ok_or_else(|| CheckError::MissingEntrypoint { + entrypoint: bundle.entrypoint.clone(), + })?; let runtime = request.runtime.runtime(false); let inputs = compile_inputs(&request.inputs, &request.launch); let lowered = runtime - .check(&workflow, None, None, &inputs) + .check_source(&bundle.entrypoint, text, &bundle.files(), None, &inputs) .map_err(CheckError::Load)?; - let diagnostics: Vec = lowered - .diagnostics - .iter() - .map(|diagnostic| convert(diagnostic, root.path())) - .collect(); + let diagnostics: Vec = lowered.diagnostics.iter().map(convert).collect(); match lowered.graph { Some(graph) => Ok(Admitted { graph, @@ -153,39 +156,6 @@ pub fn check(request: &CheckRequest) -> Result { } } -/// Write the bundle under `root/bundle/`, with `root/.fabro` beside it so -/// the frontend takes `root` as the bundle root. Returns the entrypoint's -/// path. -#[expect( - clippy::disallowed_methods, - reason = "the check is a blocking function; its caller runs it on the blocking pool" -)] -fn materialize(root: &Path, bundle: &Bundle) -> Result { - let write = |relative: &str, text: &str| -> Result<(), CheckError> { - let path = root.join(relative); - let materialize = |source| CheckError::Materialize { - path: path.clone(), - source, - }; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).map_err(materialize)?; - } - std::fs::write(&path, text).map_err(materialize) - }; - let fabro_dir = root.join(".fabro"); - std::fs::create_dir_all(&fabro_dir).map_err(|source| CheckError::Materialize { - path: fabro_dir, - source, - })?; - if let Some(project) = &bundle.project_toml { - write(PROJECT_FILE, project)?; - } - for (relative, text) in &bundle.files { - write(&format!("{BUNDLE_DIR}/{relative}"), text)?; - } - Ok(root.join(BUNDLE_DIR).join(&bundle.entrypoint)) -} - /// The compile inputs: the intent's inputs, and the launch variables. fn compile_inputs(inputs: &BTreeMap, launch: &Launch) -> CompileInputs { let mut compile = CompileInputs::new(); @@ -202,8 +172,9 @@ fn compile_inputs(inputs: &BTreeMap, launch: &Launch) -> CompileI compile .vars .insert(LAUNCH_PROVIDER_VAR.into(), text(&launch.provider)); - // Bound even when absent: `Runtime::lower` would otherwise bind the - // temporary bundle root, which is gone by the time the run starts. + // `Runtime::check_source` uses the inputs as given, so the repository + // is the host's to bind: the launch's path, or `null` for a run that + // starts from an empty workspace. let repository = launch.repository.as_ref().map_or(Value::Null, |path| { Value::String(path.to_string_lossy().into_owned()) }); @@ -211,30 +182,20 @@ fn compile_inputs(inputs: &BTreeMap, launch: &Launch) -> CompileI compile } -/// Petri's diagnostic in Fabro's shape, with the file made relative to the -/// bundle. -fn convert(diagnostic: &frontend::Diagnostic, root: &Path) -> Diagnostic { - let file = diagnostic.span.file.as_str(); - let prefix = format!("{BUNDLE_DIR}/"); - let file = Path::new(file) - .strip_prefix(root) - .map_or(file, |relative| relative.to_str().unwrap_or(file)) - .to_string(); - let file = file - .strip_prefix(&prefix) - .map_or(file.as_str(), |relative| relative) - .to_string(); +/// Petri's diagnostic in Fabro's shape. The file is the path the map holds +/// it under, which is bundle-relative already. +fn convert(diagnostic: &frontend::Diagnostic) -> Diagnostic { Diagnostic { severity: match diagnostic.severity { Severity::Error => DiagnosticSeverity::Error, Severity::Warning => DiagnosticSeverity::Warning, }, - code: diagnostic.code.to_string(), - message: diagnostic.message.clone(), - hint: diagnostic.hint.clone(), - file, - line: (diagnostic.span.line > 0).then_some(diagnostic.span.line), - column: (diagnostic.span.column > 0).then_some(diagnostic.span.column), + code: diagnostic.code.to_string(), + message: diagnostic.message.clone(), + hint: diagnostic.hint.clone(), + file: diagnostic.span.file.to_string(), + line: (diagnostic.span.line > 0).then_some(diagnostic.span.line), + column: (diagnostic.span.column > 0).then_some(diagnostic.span.column), } } diff --git a/lib/components/fabro-petri/tests/check.rs b/lib/components/fabro-petri/tests/check.rs index e936f716a..84cce9111 100644 --- a/lib/components/fabro-petri/tests/check.rs +++ b/lib/components/fabro-petri/tests/check.rs @@ -1,7 +1,8 @@ -//! Petri compiles at create time: `fabro_petri::check` materializes a bundle, -//! hands it to `Runtime::check`, and returns the admitted graphs or Petri's -//! diagnostics in Fabro's shape; `fabro_petri::admission` round-trips the -//! admitted graphs through Fabro's blob store. +//! Petri compiles at create time: `fabro_petri::check` hands a bundle to +//! `Runtime::check_source` as an in-memory file map, and returns the +//! admitted graphs or Petri's diagnostics in Fabro's shape; +//! `fabro_petri::admission` round-trips the admitted graphs through Fabro's +//! blob store. //! //! No sandbox plugin is needed: nothing here runs a graph. @@ -152,6 +153,94 @@ async fn a_launch_binds_the_repository_and_the_model_default() { ); } +#[tokio::test] +async fn a_version_that_names_the_petri_engine_is_admitted() { + let settings = format!("{SETTINGS}engine = \"petri\"\n"); + let request = request( + bundle(&[ + ("workflow.fabro", COMMAND_WORKFLOW), + ("workflow.toml", &settings), + ]), + RuntimeSpec::default(), + ); + + let admitted = check::check(&request).expect("`engine = \"petri\"` is a known key"); + + assert!( + admitted + .warnings + .iter() + .all(|w| w.code != "unsupported.workflow_toml.key"), + "{:?}", + admitted.warnings + ); +} + +#[tokio::test] +async fn an_unknown_workflow_key_is_refused_and_named_in_workflow_toml() { + let settings = format!("{SETTINGS}bogus = \"1\"\n"); + let request = request( + bundle(&[ + ("workflow.fabro", COMMAND_WORKFLOW), + ("workflow.toml", &settings), + ]), + RuntimeSpec::default(), + ); + + let Err(CheckError::Rejected(diagnostics)) = check::check(&request) else { + panic!("an unknown `[workflow]` key should be refused"); + }; + + let error = diagnostics + .iter() + .find(|d| d.code == "unsupported.workflow_toml.key") + .unwrap_or_else(|| panic!("no unknown-key diagnostic in {diagnostics:?}")); + assert!(error.is_error()); + assert!(error.message.contains("workflow.bogus"), "{error:?}"); + assert_eq!(error.file, "workflow.toml"); +} + +#[tokio::test] +async fn the_project_settings_are_read_from_the_map() { + let workflow = UNKNOWN_MODEL_WORKFLOW.replace(", model=\"no-such-model-9000\"", ""); + let request = request( + Bundle { + project_toml: Some("[run.model]\nname = \"gpt-5.4\"\n".to_string()), + ..bundle(&[("workflow.fabro", &workflow), ("workflow.toml", SETTINGS)]) + }, + runtime_with_openai(), + ); + + let admitted = check::check(&request).expect("the project model is admitted"); + + let work = admitted + .graph + .body + .nodes + .iter() + .find(|node| node.name == "work") + .expect("the work node is in the graph"); + assert_eq!(work.step.config["provider"], "openai"); + assert_eq!(work.step.config["model"], "gpt-5.4"); +} + +#[tokio::test] +async fn a_missing_entrypoint_is_an_error() { + let request = request( + Bundle { + entrypoint: "missing.fabro".to_string(), + ..bundle(&[("workflow.fabro", COMMAND_WORKFLOW)]) + }, + RuntimeSpec::default(), + ); + + let Err(CheckError::MissingEntrypoint { entrypoint }) = check::check(&request) else { + panic!("an entrypoint outside the bundle should be an error"); + }; + + assert_eq!(entrypoint, "missing.fabro"); +} + #[tokio::test] async fn an_unknown_attribute_is_refused_with_petris_code() { let request = request(