From 13bcac88dc4663c26abbe72d35efc551f959b471 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 16:23:02 -0400 Subject: [PATCH 1/2] refactor: remove negative scratch assertions and rename child dir to stages/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove test assertions that verified legacy files (final.patch, workflow_bundle.json, manifest.json, cache/artifacts/values/) do not exist in scratch directories — these are a test smell since the code that wrote them is long gone. Also rename child workflow scratch path from nodes/{id}_{visit}/child to stages/{id}@{visit}/child to align with stage_id convention. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-config/src/storage.rs | 10 -- lib/crates/fabro-server/src/server.rs | 118 +----------------- .../src/handler/manager_loop.rs | 4 +- .../fabro-workflow/src/operations/start.rs | 6 - .../tests/it/daytona_integration.rs | 17 --- .../fabro-workflow/tests/it/integration.rs | 45 +------ 6 files changed, 6 insertions(+), 194 deletions(-) diff --git a/lib/crates/fabro-config/src/storage.rs b/lib/crates/fabro-config/src/storage.rs index a4bf4736c..542a4f89e 100644 --- a/lib/crates/fabro-config/src/storage.rs +++ b/lib/crates/fabro-config/src/storage.rs @@ -239,16 +239,6 @@ mod tests { assert!(scratch.worktree_dir().exists()); assert!(scratch.runtime_dir().exists()); assert!(scratch.artifact_files_dir().exists()); - assert!( - !scratch - .root() - .join("cache") - .join("artifacts") - .join("values") - .exists() - ); - assert!(!scratch.root().join("final.patch").exists()); - scratch.remove().unwrap(); assert!(!scratch.root().exists()); } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 6cbf171e2..bef68aa61 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -5934,32 +5934,6 @@ mod tests { run_id } - async fn create_direct_run(state: &Arc, settings: &Settings) -> RunId { - operations::create( - state.store.as_ref(), - operations::CreateRunInput { - workflow: operations::WorkflowInput::DotSource { - source: MINIMAL_DOT.to_string(), - base_dir: None, - }, - settings: settings.clone(), - cwd: PathBuf::from("/tmp"), - workflow_slug: None, - workflow_path: None, - workflow_bundle: None, - submitted_manifest_bytes: None, - run_id: None, - host_repo_path: None, - repo_origin_url: None, - base_branch: None, - provenance: None, - }, - ) - .await - .unwrap() - .run_id - } - async fn create_durable_run_with_events( state: &Arc, run_id: RunId, @@ -6535,15 +6509,9 @@ mod tests { assert_eq!(accepted_definition["workflow_path"], "workflow.fabro"); assert!(accepted_definition["workflows"]["workflow.fabro"].is_object()); - let run_dir = PathBuf::from( - created["properties"]["run_dir"] - .as_str() - .expect("run.created should include run_dir"), - ); - assert!( - !run_dir.join("workflow_bundle.json").exists(), - "run scratch should no longer persist workflow_bundle.json" - ); + created["properties"]["run_dir"] + .as_str() + .expect("run.created should include run_dir"); } #[tokio::test] @@ -6863,86 +6831,6 @@ mod tests { assert_eq!(response.status(), StatusCode::BAD_REQUEST); } - #[tokio::test] - async fn api_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_run(&app, MINIMAL_DOT) - .await - .parse::() - .unwrap(); - let artifact_path = Storage::new(settings.storage_dir()) - .run_scratch(&run_id) - .artifact_files_dir() - .join("code") - .join("retry_2") - .join("src/lib.rs"); - std::fs::create_dir_all(artifact_path.parent().unwrap()).unwrap(); - std::fs::write(&artifact_path, "legacy scratch only").unwrap(); - - let req = Request::builder() - .method("GET") - .uri(api(&format!("/runs/{run_id}/stages/code@2/artifacts"))) - .body(Body::empty()) - .unwrap(); - let response = app.clone().oneshot(req).await.unwrap(); - assert_eq!(response.status(), StatusCode::OK); - let body = body_json(response.into_body()).await; - assert_eq!(body["data"].as_array().unwrap().len(), 0); - - let req = Request::builder() - .method("GET") - .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts/download?filename=src/lib.rs" - ))) - .body(Body::empty()) - .unwrap(); - let response = app.oneshot(req).await.unwrap(); - assert_eq!(response.status(), StatusCode::NOT_FOUND); - } - - #[tokio::test] - 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_direct_run(&state, &settings).await; - let artifact_path = Storage::new(settings.storage_dir()) - .run_scratch(&run_id) - .artifact_files_dir() - .join("code") - .join("retry_2") - .join("src/lib.rs"); - std::fs::create_dir_all(artifact_path.parent().unwrap()).unwrap(); - std::fs::write(&artifact_path, "legacy scratch only").unwrap(); - let req = Request::builder() - .method("GET") - .uri(api(&format!("/runs/{run_id}/stages/code@2/artifacts"))) - .body(Body::empty()) - .unwrap(); - let response = app.clone().oneshot(req).await.unwrap(); - assert_eq!(response.status(), StatusCode::OK); - let body = body_json(response.into_body()).await; - assert_eq!(body["data"].as_array().unwrap().len(), 0); - - let req = Request::builder() - .method("GET") - .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts/download?filename=src/lib.rs" - ))) - .body(Body::empty()) - .unwrap(); - let response = app.oneshot(req).await.unwrap(); - assert_eq!(response.status(), StatusCode::NOT_FOUND); - } - #[tokio::test] async fn create_run_returns_submitted() { let state = create_app_state(); diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index 6e2eb879b..a0742fc79 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -194,7 +194,7 @@ impl Handler for SubWorkflowHandler { // Build child RunOptions let visit = visit_from_context(context) as u64; - let child_logs = run_dir.join(format!("nodes/{}_{visit}/child", node.id)); + let child_logs = run_dir.join(format!("stages/{}@{visit}/child", node.id)); let _ = std::fs::create_dir_all(&child_logs); let cancel_token = Arc::new(AtomicBool::new(false)); @@ -399,7 +399,7 @@ mod tests { .contains("Child completed") ); assert!( - dir.path().join("nodes/manager_1/child").exists(), + dir.path().join("stages/manager@1/child").exists(), "child logs should default to first-visit directory naming" ); } diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 6609d91e2..0051d413e 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -1054,12 +1054,6 @@ mod tests { .await .unwrap(); - let bundle_file = created.run_dir.join("workflow_bundle.json"); - assert!( - !bundle_file.exists(), - "run scratch should not persist workflow_bundle.json" - ); - let started = start( &run_dir, test_start_services(&store, &run_dir, emitter, registry).await, diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 979c9a1d7..813fefecc 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -728,13 +728,6 @@ async fn daytona_git_checkpoint_remote_emits_events() { "checkpoint should have git_commit_sha" ); - // Assert scratch final.patch is no longer written - let final_patch = dir.path().join("final.patch"); - assert!( - !final_patch.exists(), - "final.patch should not be written to scratch" - ); - env.cleanup().await.unwrap(); } @@ -1243,13 +1236,6 @@ async fn daytona_git_checkpoint_with_shadow_branch() { "sandbox commit should have Fabro-Run trailer, got:\n{commit_msg}" ); - // Assert scratch final.patch is no longer written - let final_patch = dir.path().join("final.patch"); - assert!( - !final_patch.exists(), - "final.patch should not be written to scratch" - ); - env.cleanup().await.unwrap(); } @@ -1367,9 +1353,6 @@ async fn daytona_asset_collection() { let content = std::fs::read_to_string(&report_path).unwrap(); assert!(content.contains("testsuites")); - let manifest_path = artifacts_dir.join("manifest.json"); - assert!(!manifest_path.exists(), "manifest.json should not exist"); - env.cleanup().await.unwrap(); } diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 4adfa3932..e08ff6d59 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -8665,17 +8665,6 @@ async fn large_context_values_are_offloaded_to_artifact_store() { "value should be a durable blob ref" ); - assert!( - !RunScratch::new(dir.path()) - .root() - .join("cache") - .join("artifacts") - .join("values") - .join(format!("{expected_blob_id}.json")) - .exists(), - "legacy host blob cache file should not exist" - ); - // WorkflowRunCompleted artifact_count now tracks captured artifacts, not offloaded values. let evts = events.lock().unwrap(); let completed_event = evts @@ -10387,13 +10376,6 @@ async fn git_checkpoint_host_emits_events_and_diff_patch() { "checkpoint should have git_commit_sha" ); - // 9. Assert scratch final.patch is no longer written - let final_patch = run_dir.path().join("final.patch"); - assert!( - !final_patch.exists(), - "final.patch should not be written to scratch" - ); - // Cleanup worktree let _ = std::process::Command::new("git") .args(["worktree", "remove", "--force"]) @@ -10828,14 +10810,7 @@ async fn parallel_git_branching_host_e2e() { "parallel branch ref should still exist for debugging" ); - // 11. Verify scratch final.patch is no longer written - let final_patch = run_dir.path().join("final.patch"); - assert!( - !final_patch.exists(), - "final.patch should not be written to scratch" - ); - - // 12. Verify events + // 11. Verify events let events = events.lock().unwrap(); let parallel_started: Vec<_> = events .iter() @@ -10972,13 +10947,6 @@ async fn git_checkpoint_host_skips_empty_diff_patch() { .expect("pipeline should succeed"); assert_eq!(outcome.status, StageStatus::Success); - // final.patch should NOT exist either - let final_patch = run_dir.path().join("final.patch"); - assert!( - !final_patch.exists(), - "final.patch should not exist when there are no changes" - ); - // Cleanup let _ = std::process::Command::new("git") .args(["worktree", "remove", "--force"]) @@ -12683,14 +12651,6 @@ async fn asset_collection_local_sandbox_success() { let report_content = std::fs::read_to_string(&report_path).unwrap(); assert!(report_content.contains("testsuites")); - // Check manifest.json is no longer written - let manifest_path = artifacts_dir.join("manifest.json"); - assert!( - !manifest_path.exists(), - "manifest.json should not exist at {}", - manifest_path.display() - ); - // Check that ArtifactCaptured events were emitted let captured_events = events.lock().unwrap(); let asset_events: Vec<&RunEvent> = captured_events @@ -12889,9 +12849,6 @@ async fn asset_collection_docker_sandbox() { let content = std::fs::read_to_string(&report_path).unwrap(); assert!(content.contains("testsuites")); - let manifest_path = artifacts_dir.join("manifest.json"); - assert!(!manifest_path.exists(), "manifest.json should not exist"); - sandbox.cleanup().await.unwrap(); } From dad58c5b4ad17da6b59ad793d0bc9891cd64e91b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 16:24:11 -0400 Subject: [PATCH 2/2] plan --- ...-services-command-context-refactor-plan.md | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md b/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md index c90755082..02c988dfb 100644 --- a/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md +++ b/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md @@ -6,6 +6,7 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz ## Public Types And Interfaces - Add eager, invocation-scoped `CommandContext`: - Holds `cwd`, `base_config_path`, `machine_settings`, `server_mode`, and a cached server client cell. + - `cwd` is the invocation working directory and replaces repeated inline `std::env::current_dir()` lookups in migrated commands such as `preflight`, `validate`, `graph`, `fabro settings`, and workflow-oriented run creation paths. - `base_config_path` is the local settings file path chosen by `--config`, `FABRO_CONFIG`, or the default path. - `machine_settings` is the result of the existing local settings loaders for the command: - base settings for commands using `load_settings()` @@ -22,7 +23,8 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz - `ServerMode::ByStorageDir { target_override: Option, storage_dir_override: Option }` - `ByTarget` maps to the current `connect_server_only(...)` behavior. - `ByTarget` also covers the current `ServerSummaryLookup::connect(...)` resolution path used by run, pr, runs, and artifact lookup commands. - - `ByStorageDir` maps to the current `connect_server_backed_api_client(...)` and `connect_server_backed_api_client_with_storage_dir(...)` behaviors. + - `ByStorageDir` maps to the current `connect_server_backed_api_client_with_storage_dir(...)` behavior when a real storage-dir-aware connection mode is needed. + - current callers of `connect_server_backed_api_client(...)` migrate to `ByTarget` in this refactor because their existing `None` storage-dir path collapses to the same local settings load as `connect_server_only(...)`. - Do not collapse these two behaviors into one variant with optional target and storage fields. - Reuse existing target concepts instead of introducing a second target enum: - keep `ServerTargetArgs`, `ServerConnectionArgs`, and the resolved `ServerTarget` model already used by `user_config` and `server_client` @@ -44,6 +46,11 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz - to make both modes uniform, refactor the current storage-dir-backed path, which now returns a bare `fabro_api::Client`, to construct a `ServerStoreClient` first and expose the generated client through an accessor - migrated connection logic must use `machine_settings` and `base_config_path` already loaded on `CommandContext`; it should stop re-calling `load_settings()` and `load_settings_with_storage_dir(...)` inside `server_client.rs` - HTTP and HTTPS targets never auto-start +- Keep the existing run-summary lookup pattern, but separate lookup construction from connection: + - add `ServerSummaryLookup::from_client(client: Arc) -> Result` + - make the existing `ServerSummaryLookup::connect(...)` a compatibility wrapper during migration, then remove direct call sites from migrated commands + - migrated commands that currently do `ServerSummaryLookup::connect(...)` should instead do `ServerSummaryLookup::from_client(ctx.server().await?)` + - this keeps summary listing, sorting, and selector resolution behavior intact while moving connection ownership into `CommandContext` ## Implementation Changes - Keep `main` bootstrap ordering intact: @@ -59,7 +66,7 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz - migrated commands should reuse the shared `server_client::map_api_error` - remove remaining verbatim local copies in the in-scope command surface - Implement in this order: - - Step 1: add `CommandContext`, `ServerMode`, `ctx.server()` caching semantics, convert the storage-dir-backed connect path to construct `ServerStoreClient`, and thread preloaded `machine_settings` / `base_config_path` into server resolution so migrated commands stop re-loading settings inside connection helpers + - Step 1: add `CommandContext`, `ServerMode`, `ctx.server()` caching semantics, convert the storage-dir-backed connect path to construct `ServerStoreClient`, add `ServerSummaryLookup::from_client(...)`, and thread preloaded `machine_settings` / `base_config_path` into server resolution so migrated commands stop re-loading settings inside connection helpers - Step 2: migrate the workflow-oriented server commands: - the main `run` command in [`commands/run/command.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-cli/src/commands/run/command.rs) - `run create` @@ -78,23 +85,26 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz - `preflight` - `validate` - `graph` - - Step 3: migrate the remaining user-facing commands that resolve by target or resolved-target lookup: - - `pr` (treat separately inside this step because it mixes settings for app ID and `ServerSummaryLookup::connect`) - - `runs` - - `artifact` - - Step 4: migrate the user-facing commands that use the storage-dir-aware settings loader: + - Step 3: migrate the remaining user-facing commands that use target-based resolution or resolved-target lookup: - `model` - `secret` - `provider login` - `repo init` - `doctor` + - `pr` (treat separately inside this step because it mixes settings for app ID and `ServerSummaryLookup::connect`) + - `runs` + - `artifact` + - these commands should use `ServerMode::ByTarget` after migration, even when they currently call `connect_server_backed_api_client(...)`, because their existing `None` storage-dir path collapses to the same local settings load as `connect_server_only(...)` + - Step 4: migrate the user-facing commands that genuinely resolve through a storage-dir-aware server mode: - `system info` - `system df` - `system events` - `system prune` + - these commands should use `ServerMode::ByStorageDir` - Step 5: adapt `fabro settings` to use `CommandContext` only for base local settings inputs while keeping its existing layer-building and effective-settings logic - Step 6: remove obsolete helper entrypoints from migrated call sites and reduce `server_client.rs` to the minimal shared surface still needed by explicit out-of-scope and internal commands: - migrated commands should stop calling `connect_server_only(...)`, `connect_server_backed_api_client(...)`, `connect_server_backed_api_client_with_storage_dir(...)`, and `ServerSummaryLookup::connect(...)` directly + - migrated commands that need run lookup/resolve behavior should use `ServerSummaryLookup::from_client(ctx.server().await?)` - keep `connect_server(...)`, `connect_api_client(...)`, and `connect_server_target_direct(...)` only for direct storage-dir or direct-target flows that remain explicit out-of-scope or internal - Stage dependencies: - Steps 2, 3, and 4 all depend on Step 1 @@ -112,6 +122,7 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz - `sandbox` - `upgrade` - hidden analytics and panic upload commands + - direct storage-dir run lookup flows, including `ServerRunLookup`, remain unchanged in this pass - Cleanup target after the pass: - the remaining old helper surface should exist only for those explicitly out-of-scope or internal commands - migrated user-facing commands should no longer call settings loaders or top-level server connect helpers directly @@ -136,8 +147,8 @@ Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centraliz - `exec` with an explicit server target still constructs the server-backed adapter path correctly using `ServerStoreClient` accessors - Existing integration coverage that must keep passing for migrated command groups: - Step 2: the main `run` command, `run create`, `run start`, `run attach`, `run diff`, `run logs`, `run preview`, `run ssh`, `run resume`, `run rewind`, `run fork`, `run wait`, `run cp`, `preflight`, `validate`, and `graph` - - Step 3: representative `pr`, `artifact`, and `runs` commands - - Step 4: representative `model`, `secret`, `provider login`, `repo init`, `doctor`, `system info`, `system df`, `system events`, and `system prune` commands + - Step 3: representative `model`, `secret`, `provider login`, `repo init`, `doctor`, `pr`, `artifact`, and `runs` commands + - Step 4: representative `system info`, `system df`, `system events`, and `system prune` commands - Step 5: `fabro settings` base-settings-input path adopted from `CommandContext` while layer-building and effective-settings logic stay unchanged ## Assumptions And Defaults