From 5e79285a4329c74d0ba68eaf06003da0bb449b93 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 4 Sep 2026 16:55:28 -0400 Subject: [PATCH] Honor the workflow's configured run.scm repository in target derivation The replaced manifest builder resolved the run's repository identity from the workflow's run.scm settings before falling back to the checkout's origin. The new standalone derivation always used the checkout's origin, so a fork checkout of a workflow that names its upstream repository silently targeted the fork and pushed there. Read the run.scm layer from the resolved workflow.toml and project.toml (or from the inline workflow.toml bytes) and pass it through both the CLI and the standalone run-tool adapter. When the configured repository is not the checkout's origin, nothing can be proven about it, so derivation now fails with a message naming that mismatch instead of the generic "push the commit" hint. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/src/commands/run/create.rs | 4 +- lib/apps/fabro-server/src/run_tool_create.rs | 147 ++++++++++++++++-- lib/components/fabro-manifest/src/lib.rs | 106 +++++++++++-- 3 files changed, 227 insertions(+), 30 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index 1724c7e32..9e600cf16 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -71,13 +71,15 @@ pub(crate) async fn create_run( }, resolve_run_environment(client.as_ref(), args.environment.as_deref()), )?; + let configured_repo_origin_url = + fabro_manifest::configured_repo_origin_url_for_location(package.workflow_location())?; let fabro_manifest::DerivedRunTarget { target, dirty_worktree, } = fabro_manifest::derive_run_target_for_provider( environment.settings.provider, &canonical_cwd, - None, + configured_repo_origin_url.as_deref(), )?; if dirty_worktree { fabro_util::printerr!( diff --git a/lib/apps/fabro-server/src/run_tool_create.rs b/lib/apps/fabro-server/src/run_tool_create.rs index 987f7608c..71501d808 100644 --- a/lib/apps/fabro-server/src/run_tool_create.rs +++ b/lib/apps/fabro-server/src/run_tool_create.rs @@ -4,9 +4,9 @@ use anyhow::{Context, Result, bail}; use async_trait::async_trait; use fabro_environment::DEFAULT_ENVIRONMENT_ID; use fabro_manifest::{ - CollectedWorkflowClosure, DerivedRunTarget, ResolvedLocalWorkflowPackage, - collect_inline_workflow_versions, derive_run_target_for_provider, - resolve_local_workflow_package, + CollectedWorkflowClosure, DerivedRunTarget, collect_inline_workflow_versions, + configured_repo_origin_url_for_location, configured_repo_origin_url_from_workflow_toml, + derive_run_target_for_provider, resolve_local_workflow_package, }; use fabro_tool::{ CreateRunWorkflowSource, PreparedRunCreate, RunCreateAdapter, ValidatedCreateRunSpec, @@ -104,6 +104,7 @@ impl ServerRunCreateAdapter { client: &fabro_client::Client, spec: &ValidatedCreateRunSpec, cwd: &Path, + configured_repo_origin_url: Option<&str>, ) -> Result { if let Some(target) = &spec.target { if matches!(target, RunTarget::Folder { .. }) && !self.has_shared_filesystem() { @@ -152,11 +153,16 @@ impl ServerRunCreateAdapter { let canonical_cwd = fs::canonicalize(cwd).await.with_context(|| { format!("failed to canonicalize run directory {}", cwd.display()) })?; + let configured_repo_origin_url = configured_repo_origin_url.map(str::to_owned); let DerivedRunTarget { target, dirty_worktree, } = task::spawn_blocking(move || { - derive_run_target_for_provider(provider, &canonical_cwd, None) + derive_run_target_for_provider( + provider, + &canonical_cwd, + configured_repo_origin_url.as_deref(), + ) }) .await .context("run target derivation task failed")??; @@ -172,11 +178,7 @@ impl ServerRunCreateAdapter { } } - async fn collect_selector( - &self, - selector: &str, - cwd: &Path, - ) -> Result { + async fn collect_selector(&self, selector: &str, cwd: &Path) -> Result { if !self.has_shared_filesystem() { bail!( "workflow selectors require a shared Local filesystem; send inline files or an exact stored workflow version ID from Docker or Daytona" @@ -186,15 +188,27 @@ impl ServerRunCreateAdapter { let cwd = cwd.to_path_buf(); let user_workflows_root = self.user_workflows_root().map(Path::to_path_buf); task::spawn_blocking(move || { - resolve_local_workflow_package(&selector, &cwd, user_workflows_root.as_deref()) - .map(ResolvedLocalWorkflowPackage::into_closure) - .map_err(anyhow::Error::new) + let package = + resolve_local_workflow_package(&selector, &cwd, user_workflows_root.as_deref())?; + let configured_repo_origin_url = + configured_repo_origin_url_for_location(package.workflow_location())?; + Ok(CollectedWorkflow { + closure: package.into_closure(), + configured_repo_origin_url, + }) }) .await .context("workflow package collection task failed")? } } +/// A packaged workflow closure plus the `run.scm` repository its config names, +/// which standalone target derivation honors over the checkout's own origin. +struct CollectedWorkflow { + closure: CollectedWorkflowClosure, + configured_repo_origin_url: Option, +} + #[async_trait] impl RunCreateAdapter for ServerRunCreateAdapter { async fn prepare( @@ -204,11 +218,16 @@ impl RunCreateAdapter for ServerRunCreateAdapter { cwd: &Path, ) -> Result { let goal = self.resolve_goal(spec, cwd).await?; - let closure = match &spec.workflow { + let CollectedWorkflow { + closure, + configured_repo_origin_url, + } = match &spec.workflow { CreateRunWorkflowSource::Stored { workflow_version_id, } => { - let resolved_target = self.resolve_target(client, spec, cwd).await?; + // A stored version's config is not available locally, so + // derivation uses the checkout's own origin. + let resolved_target = self.resolve_target(client, spec, cwd, None).await?; return Ok(PreparedRunCreate { workflow_version_id: *workflow_version_id, target: resolved_target.target, @@ -221,7 +240,9 @@ impl RunCreateAdapter for ServerRunCreateAdapter { } CreateRunWorkflowSource::Inline(source) => collect_inline_workflow(source).await?, }; - let resolved_target = self.resolve_target(client, spec, cwd).await?; + let resolved_target = self + .resolve_target(client, spec, cwd, configured_repo_origin_url.as_deref()) + .await?; let versions = closure .versions() .map(|(_, version)| version.version()) @@ -246,11 +267,30 @@ struct ResolvedTarget { /// exact key of the file map, never a checkout selector. async fn collect_inline_workflow( source: &fabro_tool::InlineWorkflowSource, -) -> Result { +) -> Result { let entrypoint = source.entrypoint.clone(); let files = source.files.clone(); task::spawn_blocking(move || { - collect_inline_workflow_versions(&entrypoint, &files).map_err(anyhow::Error::new) + let closure = collect_inline_workflow_versions(&entrypoint, &files)?; + // The inline config is either the entrypoint itself or the + // `workflow.toml` beside the entrypoint graph. + let config_path = if Path::new(entrypoint.as_str()) + .extension() + .is_some_and(|ext| ext == "toml") + { + Some(entrypoint.clone()) + } else { + entrypoint.resolve_reference("workflow.toml").ok() + }; + let configured_repo_origin_url = config_path + .and_then(|path| files.get(&path)) + .map(|source| configured_repo_origin_url_from_workflow_toml(source)) + .transpose()? + .flatten(); + Ok(CollectedWorkflow { + closure, + configured_repo_origin_url, + }) }) .await .context("inline workflow package collection task failed")? @@ -931,4 +971,77 @@ mod tests { assert_eq!(prepared.target, RunTarget::None {}); } + + #[tokio::test] + async fn workflow_version_standalone_honors_configured_scm_repository_over_checkout_origin() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("workspace"); + let origin = temp.path().join("origin.git"); + fs::create_dir(&workspace).await.unwrap(); + run_git(temp.path(), &[ + "init", + "--bare", + "--quiet", + origin.to_str().unwrap(), + ]); + run_git(&workspace, &[ + "init", + "--quiet", + "--initial-branch", + "feature", + ]); + run_git(&workspace, &["config", "user.name", "Fabro Test"]); + run_git(&workspace, &["config", "user.email", "fabro@example.com"]); + let workflow_dir = workspace.join(".fabro/workflows/demo"); + fs::create_dir_all(&workflow_dir).await.unwrap(); + fs::write(workspace.join(".fabro/project.toml"), "_version = 1\n") + .await + .unwrap(); + fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.scm]\nowner = \"acme\"\nrepository = \"widgets\"\n", + ) + .await + .unwrap(); + fs::write( + workflow_dir.join("workflow.fabro"), + "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .await + .unwrap(); + run_git(&workspace, &["add", "."]); + run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); + // The checkout is a fork; the workflow names the upstream repository. + run_git(&workspace, &[ + "remote", + "add", + "origin", + "https://github.com/alice/widgets.git", + ]); + let push_url = format!("file://{}", origin.display()); + run_git(&workspace, &[ + "remote", "set-url", "--push", "origin", &push_url, + ]); + let server = MockServer::start_async().await; + mock_environment(&server, "default", "docker").await; + let registered = Arc::new(Mutex::new(Vec::new())); + let registration = + dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; + let client = no_proxy_client(&server.url("")); + let spec = validated_spec(&json!({ "workflow": "demo" })); + let adapter = ServerRunCreateAdapter::standalone(None); + + let error = adapter + .prepare(&client, &spec, &workspace) + .await + .expect_err("a fork checkout must not silently become the run's repository"); + + registration.assert_calls_async(0).await; + assert!( + error + .to_string() + .contains("run.scm repository that is not the local checkout's origin"), + "unexpected error: {error:#}" + ); + } } diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 876ea5b89..532bc2e0e 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -17,10 +17,13 @@ use std::path::{Component, Path, PathBuf}; use anyhow::{Context, Result, anyhow}; use fabro_api::types; use fabro_config::project::{self, WorkflowLocation, discover_project_config}; -use fabro_config::run::{resolve_run_goal_from_layer, resolve_run_goal_from_namespace}; +use fabro_config::run::{ + parse_run_layer_from_settings_toml, resolve_run_goal_from_layer, + resolve_run_goal_from_namespace, +}; use fabro_config::{ CliLayer, EnvironmentLayer, EnvironmentLifecycleLayer, MergeMap, ReplaceMap, - RunEnvironmentLayer, RunExecutionLayer, RunGoalLayer, RunLayer, RunModelLayer, + RunEnvironmentLayer, RunExecutionLayer, RunGoalLayer, RunLayer, RunModelLayer, RunScmLayer, WorkflowSettingsBuilder, }; use fabro_graphviz::graph::AttrValue; @@ -547,9 +550,42 @@ fn none_target_for_unversioned_directory( /// /// Returns an error when the source cannot be parsed or resolved. pub fn configured_repo_origin_url_from_workflow_toml(source: &str) -> Result> { - let settings = WorkflowSettingsBuilder::from_toml(source) - .context("failed to resolve workflow settings from workflow.toml")?; - Ok(configured_repo_origin_url(&settings)) + let run = parse_run_layer_from_settings_toml(source) + .context("failed to parse run settings from workflow.toml")?; + Ok(configured_repo_origin_url_from_scm_layer( + &run.scm.unwrap_or_default(), + )) +} + +/// The configured `run.scm` GitHub repository for a resolved local workflow. +/// `workflow.toml` values take precedence over the discovered +/// `.fabro/project.toml`, field by field, matching run settings layering. +/// `None` when neither names a repository. +/// +/// # Errors +/// +/// Returns an error when either config exists but cannot be read or parsed. +pub fn configured_repo_origin_url_for_location( + location: &WorkflowLocation, +) -> Result> { + let workflow = location + .toml + .as_deref() + .map(read_scm_layer) + .transpose()? + .unwrap_or_default(); + let project = discover_project_config(&location.dir)? + .as_deref() + .map(read_scm_layer) + .transpose()? + .unwrap_or_default(); + let scm = RunScmLayer { + provider: workflow.provider.or(project.provider), + owner: workflow.owner.or(project.owner), + repository: workflow.repository.or(project.repository), + github: workflow.github.or(project.github), + }; + Ok(configured_repo_origin_url_from_scm_layer(&scm)) } struct LocalGitObservation { @@ -626,15 +662,23 @@ fn github_run_target(origin_url: &str, branch: &str) -> Option { fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { let scm = &settings.run.scm; - if !scm - .provider - .as_deref() - .is_none_or(|provider| provider.eq_ignore_ascii_case("github")) - { + configured_repo_origin_url_from_scm( + scm.provider.as_deref(), + scm.owner.as_deref(), + scm.repository.as_deref(), + ) +} + +fn configured_repo_origin_url_from_scm( + provider: Option<&str>, + owner: Option<&str>, + repository: Option<&str>, +) -> Option { + if !provider.is_none_or(|provider| provider.eq_ignore_ascii_case("github")) { return None; } - let owner = scm.owner.as_deref()?; - let repository = scm.repository.as_deref()?; + let owner = owner?; + let repository = repository?; if owner.trim().is_empty() || repository.trim().is_empty() { return None; } @@ -643,6 +687,22 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { (!normalized.is_empty()).then_some(normalized) } +fn configured_repo_origin_url_from_scm_layer(scm: &RunScmLayer) -> Option { + configured_repo_origin_url_from_scm( + scm.provider.as_deref(), + scm.owner.as_deref(), + scm.repository.as_deref(), + ) +} + +fn read_scm_layer(path: &Path) -> Result { + let source = std::fs::read_to_string(path) + .with_context(|| format!("failed to read {}", path.display()))?; + let run = parse_run_layer_from_settings_toml(&source) + .with_context(|| format!("failed to parse run settings from {}", path.display()))?; + Ok(run.scm.unwrap_or_default()) +} + struct ManifestRepoInfo { /// The `origin` URL as libgit2 reports it (after `insteadOf` rewrites). origin_url: Option, @@ -825,6 +885,28 @@ pub(crate) mod test_fixtures { #[cfg(test)] mod tests { + #[test] + fn configured_repo_origin_url_reads_run_scm_from_workflow_toml() { + let configured = super::configured_repo_origin_url_from_workflow_toml( + "_version = 1\n[run.scm]\nowner = \"acme\"\nrepository = \"widgets\"\n", + ) + .unwrap(); + assert_eq!( + configured.as_deref(), + Some("https://github.com/acme/widgets") + ); + + let unconfigured = + super::configured_repo_origin_url_from_workflow_toml("_version = 1\n").unwrap(); + assert_eq!(unconfigured, None); + + let other_provider = super::configured_repo_origin_url_from_workflow_toml( + "_version = 1\n[run.scm]\nprovider = \"gitlab\"\nowner = \"acme\"\nrepository = \"widgets\"\n", + ) + .unwrap(); + assert_eq!(other_provider, None); + } + use fabro_workflow::git::head_sha; use super::*;