scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-08-28 05:27:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Simplify workflow version validation and supporting types

Browse source 
- Validate WorkflowVersion structure once at construction so
  canonical_bytes only serializes and enforces the size limit
- Collapse the three pairwise path-collision loops into one over the
  combined file and dependency keys
- Make WorkflowPath::is_ancestor_of allocation-free and remove unused
  resolve_from_root and error accessors
- Parse WorkflowVersionId via serde into/try_from, delegating length and
  charset checks to RunBlobId
- Derive ReferenceKind's Display with strum instead of a hand-written
  match
- Drop the fabro-workflow static_reference re-export shim; consumers
  import from fabro-graphviz directly
- Collapse duplicate JSON-rejection arms in the workflow-versions
  handler

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-08-11 12:32:01 -04:00
parent 88a3d8ce75
commit f1ddf7a26d
14 changed files with 67 additions and 167 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
lib/apps/fabro-server/src/server/handler/workflow_versions.rs
View file

@ -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(),

22
lib/components/fabro-graphviz/src/static_reference.rs
View file

@ -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)

3
lib/components/fabro-manifest/src/lib.rs
View file

@ -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)]

2
lib/components/fabro-manifest/src/workflow_bundler.rs
View file

@ -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};

10
lib/components/fabro-store/src/workflow_version_store.rs
View file

@ -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<BlobStore>,

71
lib/components/fabro-workflow-version/src/lib.rs
View file

@ -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<Vec<u8>, 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::<Vec<_>>();
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::<Vec<_>>();
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::<Vec<_>>();
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(())
}

2
lib/components/fabro-workflow/src/handler/manager_loop.rs
View file

@ -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

1
lib/components/fabro-workflow/src/lib.rs
View file

@ -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;

17
lib/components/fabro-workflow/src/static_reference.rs
View file

@ -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),
);
}
}

2
lib/components/fabro-workflow/src/transforms/import.rs
View file

@ -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,
};

3
lib/components/fabro-workflow/src/transforms/importable_field.rs
View file

@ -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.
///

6
lib/components/fabro-workflow/src/transforms/variable_expansion.rs
View file

@ -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.
///

32
lib/foundation/fabro-types/src/workflow_path.rs
View file

@ -58,24 +58,19 @@ impl WorkflowPath {
#[must_use]
pub fn is_ancestor_of(&self, other: &Self) -> bool {
let self_components = self.0.split('/').collect::<Vec<_>>();
let other_components = other.0.split('/').collect::<Vec<_>>();
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, WorkflowPathParseError> {
Self::resolve_from(self.parent().as_ref(), reference)
}
pub fn resolve_from_root(reference: &str) -> Result<Self, WorkflowPathParseError> {
Self::resolve_from(None, reference)
}
fn resolve_from(base: Option<&Self>, reference: &str) -> Result<Self, WorkflowPathParseError> {
validate_reference_shape(reference)?;
let mut components =
base.map_or_else(Vec::new, |path| path.0.split('/').collect::<Vec<_>>());
let mut components = self
.0
.rsplit_once('/')
.map_or_else(Vec::new, |(parent, _)| {
parent.split('/').collect::<Vec<_>>()
});
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::<Vec<_>>();
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",

40
lib/foundation/fabro-types/src/workflow_version_id.rs
View file

@ -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<RunBlobId> for WorkflowVersionId {
@ -28,6 +28,12 @@ impl fmt::Display for WorkflowVersionId {
}
}
impl From<WorkflowVersionId> 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<Self, Self::Err> {
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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
serializer.serialize_str(&self.to_string())
}
}
impl TryFrom<String> for WorkflowVersionId {
type Error = WorkflowVersionIdParseError;
impl<'de> Deserialize<'de> for WorkflowVersionId {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
String::deserialize(deserializer)?
.parse()
.map_err(D::Error::custom)
fn try_from(value: String) -> Result<Self, Self::Error> {
value.parse()
}
}