From 26b2c091cac75dc620229ef195266bc40a0c98d3 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 24 Aug 2026 13:02:40 -0400 Subject: [PATCH] Derive clone sources from the persisted run target Start reconciled the persisted target against its stored GitContext projection field by field and failed the run on any drift, which forced every RunSpec writer to keep the pair in lockstep forever. The target is validated at admission and owns the grammar, so derive the clone source from it alone; the projection stays persisted as display metadata that can no longer fail an otherwise-healthy start. Co-Authored-By: Claude Fable 5 --- .../fabro-workflow/src/operations/start.rs | 55 ++++++++----------- 1 file changed, 23 insertions(+), 32 deletions(-) diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 19cd41587..980c5db42 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -23,7 +23,6 @@ use fabro_types::settings::run::{ }; use fabro_types::{ ManifestPath, RunId, RunRunnableSource, RunSpec, SandboxProviderKind, TargetValidationError, - normalize_git_commit_sha, }; use fabro_util::error::collect_chain; use fabro_vault::Vault; @@ -588,8 +587,10 @@ fn clone_source_for_run(record: &RunSpec) -> Result { }; // The Git-target grammar is owned by `RunTarget::validate` in fabro-types; - // admission accepts targets through the same rules this start path - // re-derives the clone source from. + // admission accepts targets through the same rules, and this start path + // re-derives the clone source from the persisted target alone. The + // persisted `git` projection is display metadata, never a clone input, so + // writers cannot break starts by letting the pair drift. let validated = target.clone().validate().map_err(|error| { Error::engine(match error { TargetValidationError::Repository => { @@ -599,31 +600,10 @@ fn clone_source_for_run(record: &RunSpec) -> Result { TargetValidationError::Sha => "persisted Git run target has an invalid SHA", }) })?; - let git = record - .git - .as_ref() - .ok_or_else(|| Error::engine("persisted Git run target is missing its Git projection"))?; - let projected_sha = git - .sha - .as_deref() - .map(|value| { - normalize_git_commit_sha(value) - .ok_or_else(|| Error::engine("persisted Git projection has an invalid SHA")) - }) - .transpose()?; - let expected = validated.git; - if git.origin_url != expected.origin_url - || git.branch != expected.branch - || projected_sha != expected.sha - { - return Err(Error::engine( - "persisted Git run target disagrees with its Git projection", - )); - } Ok(CloneSourceForRun { - origin_url: Some(expected.origin_url), - branch: Some(expected.branch), - commit_sha: expected.sha, + origin_url: Some(validated.git.origin_url), + branch: Some(validated.git.branch), + commit_sha: validated.git.sha, }) } @@ -2746,22 +2726,33 @@ reasoning = false } #[test] - fn clone_commit_persisted_git_target_rejects_projection_drift() { + fn clone_commit_persisted_git_target_is_authoritative_over_projection() { let mut spec = test_support::test_run_spec(); spec.target = Some(RunTarget::Git { repo: "fabro-sh/fabro".to_string(), branch: "main".to_string(), sha: None, }); + // A drifted (or absent) projection never feeds the clone source: the + // validated target alone does. spec.git = Some(fabro_types::GitContext { - origin_url: "https://github.com/fabro-sh/fabro".to_string(), + origin_url: "https://github.com/fabro-sh/other".to_string(), branch: "other".to_string(), - sha: None, + sha: Some("abcdef0123456789abcdef0123456789abcdef01".to_string()), dirty: fabro_types::DirtyStatus::Clean, }); - let error = clone_source_for_run(&spec).unwrap_err(); + let source = clone_source_for_run(&spec).unwrap(); - assert!(error.to_string().contains("disagrees")); + assert_eq!( + source.origin_url.as_deref(), + Some("https://github.com/fabro-sh/fabro") + ); + assert_eq!(source.branch.as_deref(), Some("main")); + assert_eq!(source.commit_sha, None); + + spec.git = None; + let source = clone_source_for_run(&spec).unwrap(); + assert_eq!(source.branch.as_deref(), Some("main")); } }