diff --git a/lib/apps/fabro-server/src/server/handler/workflow_versions.rs b/lib/apps/fabro-server/src/server/handler/workflow_versions.rs index 18478f42a..4f8a2762a 100644 --- a/lib/apps/fabro-server/src/server/handler/workflow_versions.rs +++ b/lib/apps/fabro-server/src/server/handler/workflow_versions.rs @@ -64,33 +64,22 @@ fn json_rejection(rejection: JsonRejection) -> ApiError { err.body_text(), INVALID_VERSION_CODE, ), - JsonRejection::JsonSyntaxError(err) => { - ApiError::with_code(StatusCode::BAD_REQUEST, err.body_text(), INVALID_JSON_CODE) - } - JsonRejection::MissingJsonContentType(err) => { - ApiError::with_code(StatusCode::BAD_REQUEST, err.body_text(), INVALID_JSON_CODE) - } - JsonRejection::BytesRejection(err) => { - ApiError::with_code(StatusCode::BAD_REQUEST, err.body_text(), INVALID_JSON_CODE) - } - _ => ApiError::with_code( + other => ApiError::with_code( StatusCode::BAD_REQUEST, - "invalid JSON request", + other.body_text(), INVALID_JSON_CODE, ), } } fn store_error(err: WorkflowVersionStoreError) -> ApiError { - if err.is_dependency_unavailable() { - return ApiError::with_code( + match err { + err @ (WorkflowVersionStoreError::DependencyNotFound { .. } + | WorkflowVersionStoreError::DependencyInvalid { .. }) => ApiError::with_code( StatusCode::UNPROCESSABLE_ENTITY, err.to_string(), DEPENDENCY_NOT_FOUND_CODE, - ); - } - - match err { + ), WorkflowVersionStoreError::InvalidVersion(source) => ApiError::with_code( StatusCode::UNPROCESSABLE_ENTITY, source.to_string(), diff --git a/lib/components/fabro-graphviz/src/static_reference.rs b/lib/components/fabro-graphviz/src/static_reference.rs index 333c26d6a..55ec7013b 100644 --- a/lib/components/fabro-graphviz/src/static_reference.rs +++ b/lib/components/fabro-graphviz/src/static_reference.rs @@ -1,30 +1,20 @@ -use std::fmt; - use fabro_template::contains_template_syntax; use thiserror::Error; -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, strum::Display)] pub enum ReferenceKind { + #[strum(to_string = "file inline reference")] FileInline, + #[strum(to_string = "import reference")] Import, + #[strum(to_string = "child workflow reference")] ChildWorkflow, + #[strum(to_string = "Dockerfile reference")] Dockerfile, + #[strum(to_string = "graph goal file reference")] GraphGoalFile, } -impl fmt::Display for ReferenceKind { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let label = match self { - Self::FileInline => "file inline reference", - Self::Import => "import reference", - Self::ChildWorkflow => "child workflow reference", - Self::Dockerfile => "Dockerfile reference", - Self::GraphGoalFile => "graph goal file reference", - }; - f.write_str(label) - } -} - impl ReferenceKind { pub fn validate(self, value: &str) -> Result<(), StaticReferenceError> { validate_static_reference(value, self) diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 3a3e89890..dc9a0d2cf 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -19,14 +19,13 @@ use fabro_config::{ }; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; +use fabro_graphviz::static_reference::ReferenceKind; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ApprovalMode, ResolvedGoalSource, ResolvedRunGoal, RunMode}; use fabro_types::{DirtyStatus, GitContext, ManifestPath, WorkflowSettings}; use fabro_workflow::git::{ GitSyncStatus, branch_needs_push, head_sha, push_branch_noninteractive, sync_status, }; -use fabro_workflow::static_reference::ReferenceKind; - use crate::workflow_bundler::WorkflowBundler; #[derive(Debug, Default)] diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 61633c229..7b150e629 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -8,12 +8,12 @@ use fabro_config::project::WorkflowLocation; use fabro_config::{EnvironmentDockerfileLayer, EnvironmentImageLayer, SettingsLayer}; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; +use fabro_graphviz::static_reference::{self, AttributeScope, ReferenceKind}; use fabro_template::{ BundleTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, TemplateContext, TemplateDependencyClosure, TemplateRenderMode, TemplateSource, }; use fabro_types::ManifestPath; -use fabro_workflow::static_reference::{self, AttributeScope, ReferenceKind}; use crate::{manifest_path_from_absolute, normalize_absolute_path}; diff --git a/lib/components/fabro-store/src/workflow_version_store.rs b/lib/components/fabro-store/src/workflow_version_store.rs index 691f2fa24..4101487d6 100644 --- a/lib/components/fabro-store/src/workflow_version_store.rs +++ b/lib/components/fabro-store/src/workflow_version_store.rs @@ -37,16 +37,6 @@ pub enum WorkflowVersionStoreError { }, } -impl WorkflowVersionStoreError { - #[must_use] - pub fn is_dependency_unavailable(&self) -> bool { - matches!( - self, - Self::DependencyNotFound { .. } | Self::DependencyInvalid { .. } - ) - } -} - #[derive(Clone, Debug)] pub struct WorkflowVersionStore { blobs: Arc, diff --git a/lib/components/fabro-workflow-version/src/lib.rs b/lib/components/fabro-workflow-version/src/lib.rs index 73e65d6d7..61976595d 100644 --- a/lib/components/fabro-workflow-version/src/lib.rs +++ b/lib/components/fabro-workflow-version/src/lib.rs @@ -95,24 +95,6 @@ pub enum WorkflowVersionError { }, } -impl WorkflowVersionError { - #[must_use] - pub fn missing_dependencies(&self) -> Option<&[WorkflowPath]> { - match self { - Self::DependencyMismatch { missing, .. } => Some(missing), - _ => None, - } - } - - #[must_use] - pub fn unused_dependencies(&self) -> Option<&[WorkflowPath]> { - match self { - Self::DependencyMismatch { unused, .. } => Some(unused), - _ => None, - } - } -} - #[derive(Clone, Debug, PartialEq, Eq, Serialize)] pub struct WorkflowVersion { entrypoint: WorkflowPath, @@ -131,7 +113,8 @@ impl WorkflowVersion { files, dependencies, }; - version.validate()?; + version.validate_structure()?; + version.canonical_bytes()?; Ok(version) } @@ -150,12 +133,12 @@ impl WorkflowVersion { &self.dependencies } - pub fn validate(&self) -> Result<(), WorkflowVersionError> { - self.canonical_bytes().map(|_| ()) - } - + /// Serialize to the canonical wire form. + /// + /// Structural validity is guaranteed by construction (`new` and + /// `Deserialize` both validate), so this only serializes and enforces + /// the canonical size limit. pub fn canonical_bytes(&self) -> Result, WorkflowVersionError> { - self.validate_structure()?; let bytes = serde_json::to_vec(self) .map_err(|source| WorkflowVersionError::Serialization { source })?; if bytes.len() > MAX_WORKFLOW_VERSION_BYTES { @@ -194,10 +177,16 @@ impl WorkflowVersion { } fn validate_path_collisions(&self) -> Result<(), WorkflowVersionError> { - let file_paths = self.files.keys().collect::>(); - for (index, first) in file_paths.iter().enumerate() { - for second in &file_paths[index + 1..] { - if first.is_ancestor_of(second) || second.is_ancestor_of(first) { + // Keys are unique within each map, so equality can only collide + // across files and dependencies. + let paths = self + .files + .keys() + .chain(self.dependencies.keys()) + .collect::>(); + for (index, first) in paths.iter().enumerate() { + for second in &paths[index + 1..] { + if first == second || first.is_ancestor_of(second) || second.is_ancestor_of(first) { return Err(WorkflowVersionError::PathCollision { first: (*first).clone(), second: (*second).clone(), @@ -205,32 +194,6 @@ impl WorkflowVersion { } } } - - let dependency_paths = self.dependencies.keys().collect::>(); - for (index, first) in dependency_paths.iter().enumerate() { - for second in &dependency_paths[index + 1..] { - if first.is_ancestor_of(second) || second.is_ancestor_of(first) { - return Err(WorkflowVersionError::PathCollision { - first: (*first).clone(), - second: (*second).clone(), - }); - } - } - } - - for file in &file_paths { - for dependency in &dependency_paths { - if file == dependency - || file.is_ancestor_of(dependency) - || dependency.is_ancestor_of(file) - { - return Err(WorkflowVersionError::PathCollision { - first: (*file).clone(), - second: (*dependency).clone(), - }); - } - } - } Ok(()) } diff --git a/lib/components/fabro-workflow/src/handler/manager_loop.rs b/lib/components/fabro-workflow/src/handler/manager_loop.rs index 5668954c6..6d58f4cc5 100644 --- a/lib/components/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/components/fabro-workflow/src/handler/manager_loop.rs @@ -5,6 +5,7 @@ use std::time::Duration; use async_trait::async_trait; use fabro_graphviz::graph::{AttrValue, Graph, Node}; +use fabro_graphviz::static_reference::{ReferenceKind, validate_static_reference}; use fabro_store::{ArtifactStore, Database}; use fabro_types::WorkflowSettings; use object_store::memory::InMemory; @@ -20,7 +21,6 @@ use crate::operations::{ValidateInput, WorkflowInput, validate_with_catalog}; use crate::outcome::{Outcome, OutcomeExt, StageOutcome}; use crate::pipeline::types::Initialized; use crate::run_options::RunOptions; -use crate::static_reference::{ReferenceKind, validate_static_reference}; use crate::{ManifestPath, pipeline, stage_scope}; /// Orchestrates a child workflow engine, polling for completion or stop diff --git a/lib/components/fabro-workflow/src/lib.rs b/lib/components/fabro-workflow/src/lib.rs index bd7f3e665..c34bec62c 100644 --- a/lib/components/fabro-workflow/src/lib.rs +++ b/lib/components/fabro-workflow/src/lib.rs @@ -332,7 +332,6 @@ pub(crate) mod sandbox_git_runtime; pub mod services; pub(crate) mod stage_execution; mod stage_scope; -pub mod static_reference; pub mod steering_hub; #[cfg(any(test, feature = "test-support"))] pub mod test_support; diff --git a/lib/components/fabro-workflow/src/static_reference.rs b/lib/components/fabro-workflow/src/static_reference.rs deleted file mode 100644 index 161ce2e78..000000000 --- a/lib/components/fabro-workflow/src/static_reference.rs +++ /dev/null @@ -1,17 +0,0 @@ -pub use fabro_graphviz::static_reference::{ - AttributeScope, ReferenceKind, StaticReferenceError, reference_kind_for_attribute, - validate_static_reference, -}; - -#[cfg(test)] -mod tests { - use super::{AttributeScope, ReferenceKind, reference_kind_for_attribute}; - - #[test] - fn compatibility_reexport_remains_usable() { - assert_eq!( - reference_kind_for_attribute(AttributeScope::Node, "prompt", "@prompt.md"), - Some(ReferenceKind::FileInline), - ); - } -} diff --git a/lib/components/fabro-workflow/src/transforms/import.rs b/lib/components/fabro-workflow/src/transforms/import.rs index e0082ba9d..fe42079dd 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_graphviz::parser; +use fabro_graphviz::static_reference::{ReferenceKind, validate_static_reference}; use fabro_template::TemplateContext; use fabro_validate::Diagnostic; @@ -11,7 +12,6 @@ use super::file_inlining::template_render_store; use super::{FileInliningTransform, Transform}; use crate::error::Error; use crate::file_resolver::{FileResolver, ResolvedFile}; -use crate::static_reference::{ReferenceKind, validate_static_reference}; use crate::transforms::variable_expansion::{ RenderMode, TemplateRenderTarget, TemplateTransform, render_template_for_target, }; diff --git a/lib/components/fabro-workflow/src/transforms/importable_field.rs b/lib/components/fabro-workflow/src/transforms/importable_field.rs index acfcc52e8..51978e845 100644 --- a/lib/components/fabro-workflow/src/transforms/importable_field.rs +++ b/lib/components/fabro-workflow/src/transforms/importable_field.rs @@ -13,8 +13,9 @@ //! [`super::file_inlining`], where the `FileResolver` and current-dir context //! live. +use fabro_graphviz::static_reference::{ReferenceKind, validate_static_reference}; + use crate::error::Error; -use crate::static_reference::{ReferenceKind, validate_static_reference}; /// A field value that is either inline content or an `@path` file import. /// diff --git a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs index e2c11d169..642a1557c 100644 --- a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs @@ -4,6 +4,9 @@ use std::fmt::Write as _; use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Graph, Node}; +use fabro_graphviz::static_reference::{ + AttributeScope, ReferenceKind, reference_kind_for_attribute, validate_static_reference, +}; use fabro_template::{ TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateSourceOrigin, TemplateStore, @@ -17,9 +20,6 @@ use fabro_validate::{Diagnostic, Severity}; use super::Transform; use crate::error::Error; use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE}; -use crate::static_reference::{ - AttributeScope, ReferenceKind, reference_kind_for_attribute, validate_static_reference, -}; /// How the template-expansion pass should treat undefined input variables. /// diff --git a/lib/foundation/fabro-types/src/workflow_path.rs b/lib/foundation/fabro-types/src/workflow_path.rs index bf8bbaad1..32297d93e 100644 --- a/lib/foundation/fabro-types/src/workflow_path.rs +++ b/lib/foundation/fabro-types/src/workflow_path.rs @@ -58,24 +58,19 @@ impl WorkflowPath { #[must_use] pub fn is_ancestor_of(&self, other: &Self) -> bool { - let self_components = self.0.split('/').collect::>(); - let other_components = other.0.split('/').collect::>(); - self_components.len() < other_components.len() - && other_components.starts_with(&self_components) + other.0.len() > self.0.len() + && other.0.starts_with(self.0.as_str()) + && other.0.as_bytes()[self.0.len()] == b'/' } pub fn resolve_reference(&self, reference: &str) -> Result { - Self::resolve_from(self.parent().as_ref(), reference) - } - - pub fn resolve_from_root(reference: &str) -> Result { - Self::resolve_from(None, reference) - } - - fn resolve_from(base: Option<&Self>, reference: &str) -> Result { validate_reference_shape(reference)?; - let mut components = - base.map_or_else(Vec::new, |path| path.0.split('/').collect::>()); + let mut components = self + .0 + .rsplit_once('/') + .map_or_else(Vec::new, |(parent, _)| { + parent.split('/').collect::>() + }); for component in reference.split('/') { match component { @@ -126,17 +121,16 @@ impl fmt::Display for WorkflowPath { fn validate(value: &str) -> Result<(), WorkflowPathParseError> { validate_reference_shape(value)?; - let components = value.split('/').collect::>(); - if components - .iter() - .any(|component| matches!(*component, "." | "..")) + if value + .split('/') + .any(|component| matches!(component, "." | "..")) { return Err(WorkflowPathParseError::new( value, "dot segments are not allowed in stored paths", )); } - if components.len() > MAX_WORKFLOW_PATH_COMPONENTS { + if value.split('/').count() > MAX_WORKFLOW_PATH_COMPONENTS { return Err(WorkflowPathParseError::new( value, "path has too many components", diff --git a/lib/foundation/fabro-types/src/workflow_version_id.rs b/lib/foundation/fabro-types/src/workflow_version_id.rs index d1c15d3ef..039da2c61 100644 --- a/lib/foundation/fabro-types/src/workflow_version_id.rs +++ b/lib/foundation/fabro-types/src/workflow_version_id.rs @@ -1,13 +1,13 @@ use std::fmt; use std::str::FromStr; -use serde::de::Error as _; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::RunBlobId; -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(into = "String", try_from = "String")] pub struct WorkflowVersionId(RunBlobId); impl From for WorkflowVersionId { @@ -28,6 +28,12 @@ impl fmt::Display for WorkflowVersionId { } } +impl From for String { + fn from(value: WorkflowVersionId) -> Self { + value.to_string() + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq, Error)] #[error("workflow version ID must be exactly 64 lowercase hexadecimal characters")] pub struct WorkflowVersionIdParseError; @@ -36,11 +42,9 @@ impl FromStr for WorkflowVersionId { type Err = WorkflowVersionIdParseError; fn from_str(value: &str) -> Result { - if value.len() != 64 - || !value - .bytes() - .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) - { + // `RunBlobId` enforces length and hex charset but accepts uppercase digits; + // the canonical wire form is lowercase only. + if value.bytes().any(|byte| byte.is_ascii_uppercase()) { return Err(WorkflowVersionIdParseError); } value @@ -50,23 +54,11 @@ impl FromStr for WorkflowVersionId { } } -impl Serialize for WorkflowVersionId { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - serializer.serialize_str(&self.to_string()) - } -} +impl TryFrom for WorkflowVersionId { + type Error = WorkflowVersionIdParseError; -impl<'de> Deserialize<'de> for WorkflowVersionId { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - String::deserialize(deserializer)? - .parse() - .map_err(D::Error::custom) + fn try_from(value: String) -> Result { + value.parse() } }