From 38cb2ec250b2b4bc504dfd79eda635cda05b85f2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 7 Apr 2026 23:01:33 -0400 Subject: [PATCH] refactor(artifacts): remove obsolete artifact storage flag Drop the dead artifact storage capability split from run records, run.created events, and workflow/server create paths. Worker artifact upload is now unconditional, and tests/snapshots no longer encode a legacy object-backed distinction. --- lib/crates/fabro-checkpoint/src/metadata.rs | 1 - .../fabro-cli/src/commands/run/runner.rs | 36 ++++++++++------- .../fabro-cli/src/commands/store/dump.rs | 2 - lib/crates/fabro-cli/tests/it/cmd/attach.rs | 1 - lib/crates/fabro-cli/tests/it/cmd/run.rs | 1 - lib/crates/fabro-server/src/run_manifest.rs | 1 - lib/crates/fabro-server/src/server.rs | 39 ++++--------------- lib/crates/fabro-store/src/run_state.rs | 1 - lib/crates/fabro-store/src/slate/mod.rs | 1 - lib/crates/fabro-types/src/lib.rs | 4 +- lib/crates/fabro-types/src/run.rs | 18 --------- lib/crates/fabro-types/src/run_event/run.rs | 4 +- .../fabro-workflow/src/artifact_snapshot.rs | 1 - lib/crates/fabro-workflow/src/event.rs | 4 -- .../fabro-workflow/src/operations/create.rs | 15 +------ .../src/operations/rebuild_meta.rs | 2 - .../fabro-workflow/src/operations/start.rs | 1 - .../src/pipeline/execute/tests.rs | 1 - .../fabro-workflow/src/pipeline/initialize.rs | 1 - .../fabro-workflow/src/pipeline/persist.rs | 2 - .../src/pipeline/pull_request.rs | 6 --- .../fabro-workflow/src/pipeline/retro.rs | 2 - lib/crates/fabro-workflow/src/run_lookup.rs | 2 - .../fabro-workflow/src/runtime_store.rs | 2 - lib/crates/fabro-workflow/src/test_support.rs | 1 - 25 files changed, 35 insertions(+), 114 deletions(-) diff --git a/lib/crates/fabro-checkpoint/src/metadata.rs b/lib/crates/fabro-checkpoint/src/metadata.rs index d6d634ae1..cdbe86711 100644 --- a/lib/crates/fabro-checkpoint/src/metadata.rs +++ b/lib/crates/fabro-checkpoint/src/metadata.rs @@ -214,7 +214,6 @@ mod tests { repo_origin_url: None, base_branch: None, labels: HashMap::new(), - artifact_storage: None, provenance: None, } } diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index ebb1bfbfd..4d441ae5e 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -63,12 +63,8 @@ pub(crate) async fn execute( .run .as_ref() .ok_or_else(|| anyhow!("Run {run_id} has no run record in store"))?; - let artifact_uploader = build_artifact_uploader( - run_id, - run_record, - client.clone_for_reuse(), - artifact_upload_token, - ); + let artifact_uploader = + build_artifact_uploader(run_id, client.clone_for_reuse(), artifact_upload_token); let interviewer = Arc::new(ControlInterviewer::new()); tokio::spawn(read_worker_control_stream( io::stdin(), @@ -146,14 +142,9 @@ async fn apply_worker_control_line(interviewer: &ControlInterviewer, line: &str) fn build_artifact_uploader( run_id: RunId, - run_record: &fabro_types::RunRecord, client: server_client::ServerStoreClient, artifact_upload_token: Option, ) -> Option> { - if !run_record.uses_object_backed_artifacts() { - return None; - } - let uploader: Arc = match artifact_upload_token { Some(token) => Arc::new(HttpArtifactUploader { run_id, @@ -484,8 +475,9 @@ mod tests { use serde_json::json; use super::{ - WorkerTitlePhase, apply_worker_control_line, execute, initial_worker_title_phase, - read_worker_control_stream, worker_title, worker_title_phase_for_event, + WorkerTitlePhase, apply_worker_control_line, build_artifact_uploader, execute, + initial_worker_title_phase, read_worker_control_stream, worker_title, + worker_title_phase_for_event, }; use crate::args::RunWorkerMode; use fabro_interview::{AnswerValue, ControlInterviewer, Interviewer, Question, QuestionType}; @@ -588,6 +580,24 @@ mod tests { ); } + #[tokio::test] + async fn build_artifact_uploader_does_not_depend_on_run_capability_flag() { + let server = MockServer::start_async().await; + + let uploader = build_artifact_uploader( + fixtures::RUN_1, + crate::server_client::connect_server_target_direct(&format!( + "{}/api/v1", + server.base_url() + )) + .await + .unwrap(), + Some("token".to_string()), + ); + + assert!(uploader.is_some()); + } + #[tokio::test] async fn worker_bootstrap_loads_run_state_without_prefetching_run_events() { let server = MockServer::start_async().await; diff --git a/lib/crates/fabro-cli/src/commands/store/dump.rs b/lib/crates/fabro-cli/src/commands/store/dump.rs index 7c19b0ca9..9039527be 100644 --- a/lib/crates/fabro-cli/src/commands/store/dump.rs +++ b/lib/crates/fabro-cli/src/commands/store/dump.rs @@ -342,7 +342,6 @@ mod tests { repo_origin_url: Some("https://github.com/fabro-sh/fabro".to_string()), base_branch: Some("main".to_string()), labels: HashMap::from([("team".to_string(), "infra".to_string())]), - artifact_storage: None, provenance: None, } } @@ -488,7 +487,6 @@ mod tests { base_branch: run_record.base_branch.clone(), workflow_slug: run_record.workflow_slug.clone(), db_prefix: None, - artifact_storage: run_record.artifact_storage, provenance: run_record.provenance.clone(), }, ) diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index e73d1a221..e828bf743 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -567,7 +567,6 @@ fn attach_json_errors_without_prompting_for_human_input() { "event": "run.created", "id": "[EVENT_ID]", "properties": { - "artifact_storage": "object_store_v1", "graph": { "attrs": { "goal": { diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 62445ba66..cc0110850 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -646,7 +646,6 @@ fn json_run_implies_auto_approve_for_human_gates() { "event": "run.created", "id": "[EVENT_ID]", "properties": { - "artifact_storage": "object_store_v1", "graph": { "attrs": { "goal": { diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 9f8bfce7a..a471d9db3 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -131,7 +131,6 @@ pub(crate) fn create_run_input(prepared: PreparedManifest) -> CreateRunInput { .as_ref() .map(|git| fabro_github::normalize_repo_origin_url(&git.origin_url)), base_branch: prepared.git.as_ref().map(|git| git.branch.clone()), - artifact_storage: None, provenance: None, } } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 6cfbdfd6f..f9407d031 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -34,9 +34,9 @@ use fabro_store::{ ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, StageId, }; use fabro_types::{ - EventBody, InterviewQuestionRecord, InterviewQuestionType, RunArtifactStorage, RunBlobId, - RunClientProvenance, RunControlAction, RunEvent, RunId, RunProvenance, RunServerProvenance, - RunSubjectProvenance, Settings, + EventBody, InterviewQuestionRecord, InterviewQuestionType, RunBlobId, RunClientProvenance, + RunControlAction, RunEvent, RunId, RunProvenance, RunServerProvenance, RunSubjectProvenance, + Settings, }; use fabro_util::redact::redact_jsonl_line; use fabro_util::version::FABRO_VERSION; @@ -3097,7 +3097,6 @@ async fn create_run( let mut create_input = run_manifest::create_run_input(prepared.clone()); create_input.run_id = Some(run_id); - create_input.artifact_storage = Some(RunArtifactStorage::ObjectStoreV1); create_input.provenance = Some(run_provenance(&headers, &subject)); let created = match Box::pin(operations::create(state.store.as_ref(), create_input)).await { @@ -5856,7 +5855,7 @@ mod tests { run_id } - async fn create_legacy_run(state: &Arc, settings: &Settings) -> RunId { + async fn create_direct_run(state: &Arc, settings: &Settings) -> RunId { operations::create( state.store.as_ref(), operations::CreateRunInput { @@ -5873,7 +5872,6 @@ mod tests { host_repo_path: None, repo_origin_url: None, base_branch: None, - artifact_storage: None, provenance: None, }, ) @@ -6580,7 +6578,7 @@ mod tests { } #[tokio::test] - async fn create_run_marks_object_backed_artifacts() { + async fn create_run_persists_run_record() { let state = create_app_state(); let app = build_router(Arc::clone(&state), AuthMode::Disabled); @@ -6597,13 +6595,7 @@ mod tests { .await .unwrap(); - assert!( - run_state - .run - .as_ref() - .unwrap() - .uses_object_backed_artifacts() - ); + assert!(run_state.run.is_some()); } #[tokio::test] @@ -6768,14 +6760,14 @@ mod tests { } #[tokio::test] - async fn legacy_runs_do_not_fallback_to_scratch_artifacts() { + async fn directly_created_runs_do_not_fallback_to_scratch_artifacts() { let temp = tempfile::tempdir().unwrap(); let mut settings = dry_run_settings(); settings.storage_dir = Some(temp.path().join("storage")); let state = create_app_state_with_options(settings.clone(), 5); let app = build_router(Arc::clone(&state), AuthMode::Disabled); - let run_id = create_legacy_run(&state, &settings).await; + let run_id = create_direct_run(&state, &settings).await; let artifact_path = Storage::new(settings.storage_dir()) .run_scratch(&run_id) .artifact_files_dir() @@ -6784,21 +6776,6 @@ mod tests { .join("src/lib.rs"); std::fs::create_dir_all(artifact_path.parent().unwrap()).unwrap(); std::fs::write(&artifact_path, "legacy scratch only").unwrap(); - let run_state = state - .store - .open_run_reader(&run_id) - .await - .unwrap() - .state() - .await - .unwrap(); - assert!( - !run_state - .run - .as_ref() - .unwrap() - .uses_object_backed_artifacts() - ); let req = Request::builder() .method("GET") .uri(api(&format!("/runs/{run_id}/stages/code@2/artifacts"))) diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index 4d793e18e..5b898ec07 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -93,7 +93,6 @@ impl RunProjection { repo_origin_url: props.repo_origin_url.clone(), base_branch: props.base_branch.clone(), labels, - artifact_storage: props.artifact_storage, provenance: props.provenance.clone(), }); self.graph_source.clone_from(&props.workflow_source); diff --git a/lib/crates/fabro-store/src/slate/mod.rs b/lib/crates/fabro-store/src/slate/mod.rs index cecafad7c..3ae03d978 100644 --- a/lib/crates/fabro-store/src/slate/mod.rs +++ b/lib/crates/fabro-store/src/slate/mod.rs @@ -288,7 +288,6 @@ mod tests { repo_origin_url: Some("https://github.com/fabro-sh/fabro".to_string()), base_branch: Some("main".to_string()), labels: std::collections::HashMap::from([("team".to_string(), "infra".to_string())]), - artifact_storage: None, provenance: None, } } diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index 0331da290..6749e5545 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -45,8 +45,8 @@ pub use retro::{ OpenItemKind, Retro, RetroNarrative, SmoothnessRating, StageRetro, }; pub use run::{ - RunArtifactStorage, RunAuthMethod, RunClientProvenance, RunProvenance, RunRecord, - RunServerProvenance, RunSubjectProvenance, + RunAuthMethod, RunClientProvenance, RunProvenance, RunRecord, RunServerProvenance, + RunSubjectProvenance, }; pub use run_blob_id::RunBlobId; pub use run_event::{EventBody, RunEvent, RunNoticeLevel}; diff --git a/lib/crates/fabro-types/src/run.rs b/lib/crates/fabro-types/src/run.rs index e25e31f92..cafef8d28 100644 --- a/lib/crates/fabro-types/src/run.rs +++ b/lib/crates/fabro-types/src/run.rs @@ -48,12 +48,6 @@ pub struct RunProvenance { pub subject: Option, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum RunArtifactStorage { - ObjectStoreV1, -} - #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RunRecord { pub run_id: RunId, @@ -71,17 +65,5 @@ pub struct RunRecord { #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub labels: HashMap, #[serde(default, skip_serializing_if = "Option::is_none")] - pub artifact_storage: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] pub provenance: Option, } - -impl RunRecord { - #[must_use] - pub fn uses_object_backed_artifacts(&self) -> bool { - matches!( - self.artifact_storage, - Some(RunArtifactStorage::ObjectStoreV1) - ) - } -} diff --git a/lib/crates/fabro-types/src/run_event/run.rs b/lib/crates/fabro-types/src/run_event/run.rs index bf5fcd2a0..bae55712a 100644 --- a/lib/crates/fabro-types/src/run_event/run.rs +++ b/lib/crates/fabro-types/src/run_event/run.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; -use crate::{Graph, RunArtifactStorage, RunControlAction, RunProvenance, Settings, StatusReason}; +use crate::{Graph, RunControlAction, RunProvenance, Settings, StatusReason}; use super::{BilledTokenCounts, RunNoticeLevel}; @@ -29,8 +29,6 @@ pub struct RunCreatedProps { #[serde(default, skip_serializing_if = "Option::is_none")] pub db_prefix: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub artifact_storage: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] pub provenance: Option, } diff --git a/lib/crates/fabro-workflow/src/artifact_snapshot.rs b/lib/crates/fabro-workflow/src/artifact_snapshot.rs index 3bba9a506..a18124296 100644 --- a/lib/crates/fabro-workflow/src/artifact_snapshot.rs +++ b/lib/crates/fabro-workflow/src/artifact_snapshot.rs @@ -352,7 +352,6 @@ mod tests { use super::*; use fabro_agent::sandbox::ExecResult; use std::collections::HashMap; - use std::fs; /// Minimal mock sandbox for artifact_snapshot tests. struct AssetMockSandbox { diff --git a/lib/crates/fabro-workflow/src/event.rs b/lib/crates/fabro-workflow/src/event.rs index 518b052b1..fe80c16de 100644 --- a/lib/crates/fabro-workflow/src/event.rs +++ b/lib/crates/fabro-workflow/src/event.rs @@ -54,8 +54,6 @@ pub enum Event { #[serde(default, skip_serializing_if = "Option::is_none")] db_prefix: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - artifact_storage: Option<::fabro_types::RunArtifactStorage>, - #[serde(default, skip_serializing_if = "Option::is_none")] provenance: Option<::fabro_types::RunProvenance>, }, WorkflowRunStarted { @@ -1390,7 +1388,6 @@ fn event_body_from_event(event: &Event) -> EventBody { base_branch, workflow_slug, db_prefix, - artifact_storage, provenance, .. } => EventBody::RunCreated(fabro_types::RunCreatedProps { @@ -1406,7 +1403,6 @@ fn event_body_from_event(event: &Event) -> EventBody { base_branch: base_branch.clone(), workflow_slug: workflow_slug.clone(), db_prefix: db_prefix.clone(), - artifact_storage: *artifact_storage, provenance: provenance.clone(), }), Event::WorkflowRunStarted { diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index f311f2baf..2618fe7ae 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -3,7 +3,7 @@ use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_model::{Catalog, Provider}; use fabro_sandbox::SandboxProvider; use fabro_store::Database; -use fabro_types::{RunArtifactStorage, RunId, RunProvenance, Settings}; +use fabro_types::{RunId, RunProvenance, Settings}; use std::collections::BTreeMap; use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -35,7 +35,6 @@ pub struct CreateRunInput { pub host_repo_path: Option, pub repo_origin_url: Option, pub base_branch: Option, - pub artifact_storage: Option, pub provenance: Option, } @@ -57,7 +56,6 @@ struct PersistCreateOptions { working_directory: PathBuf, host_repo_path: Option, repo_origin_url: Option, - artifact_storage: Option, provenance: Option, } @@ -85,7 +83,6 @@ pub async fn create(store: &Database, request: CreateRunInput) -> Result Result