mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-06 02:48:25 +00:00
Merge remote-tracking branch 'origin/main'
This commit is contained in:
Bryan Helmkamp
commit
622760952d
7 changed files with 26 additions and 203 deletions
|
|
@ -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<String>, storage_dir_override: Option<PathBuf> }`
|
||||
- `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<ServerStoreClient>) -> Result<Self>`
|
||||
- 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
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6009,32 +6009,6 @@ mod tests {
|
|||
run_id
|
||||
}
|
||||
|
||||
async fn create_direct_run(state: &Arc<AppState>, 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<AppState>,
|
||||
run_id: RunId,
|
||||
|
|
@ -6611,15 +6585,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]
|
||||
|
|
@ -6939,86 +6907,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::<RunId>()
|
||||
.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();
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue