scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-15 23:32:46 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Merge remote-tracking branch 'origin/main' into production-web-ui

Browse source
This commit is contained in:
Bryan Helmkamp 2026-04-08 14:44:35 -04:00
commit 8bab1335c9
No known key found for this signature in database
7 changed files with 273 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

149
docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md Normal file
View file

@ -0,0 +1,149 @@
# CLI CommandContext And Server Access Refactor Plan
## Summary
Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centralizes local settings loading and server connection setup for user-facing server-backed commands. This is a greenfield codebase with no backward-compatibility constraints, so the refactor should be done in one full pass for the in-scope command surface rather than preserving a long-lived mixed model. Keep config-layer composition command-local where commands genuinely need it, keep the generated `fabro_api::Client` as the main HTTP interface, and reuse the existing `ServerStoreClient` type instead of layering a second handwritten endpoint facade on top of it.
## Public Types And Interfaces
- Add eager, invocation-scoped `CommandContext`:
- Holds `cwd`, `base_config_path`, `machine_settings`, `server_mode`, and a cached server client cell.
- `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()`
- base settings plus storage-dir override for commands using `load_settings_with_storage_dir(...)`
- `machine_settings` does not include workflow or project config layers.
- Do not store `ConfigLayer` on `CommandContext`.
- Keep config-layer composition command-local:
- `fabro settings` continues to build `EffectiveSettingsLayers` with its existing helpers.
- workflow and manifest code continues to use `ConfigLayer::for_workflow(...)`, `discover_project_config(...)`, and existing workflow resolution structs where individual layers matter.
- `CommandContext` should not attempt to reconstruct or cache workflow/project config layers.
- Use two explicit server access modes that match the real connection paths in the codebase:
- `ServerMode::None`
- `ServerMode::ByTarget { target_override: Option<String> }`
- `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.
- 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`
- do not introduce `ServerTargetInput` in this pass
- Keep `ServerStoreClient`:
- do not rename it during the same refactor
- add narrow accessors as needed for:
- the generated `fabro_api::Client`
- raw `reqwest::Client`
- `base_url`
- this preserves the current `exec` adapter path without adding a second server session type
- `CommandContext::server().await?`:
- available only for `ServerMode::ByTarget` and `ServerMode::ByStorageDir`
- returns `Arc<ServerStoreClient>`
- caches the first successful connection in a `OnceCell`
- does not cache failures; a later retry should attempt a fresh connection
- `ByTarget` performs current target-based resolution and Unix-socket auto-start behavior
- `ByStorageDir` performs current storage-dir-backed daemon resolution and startup behavior
- 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
## Implementation Changes
- Keep `main` bootstrap ordering intact:
- continue loading enough local settings before tracing init to determine log level and upgrade-check behavior
- `CommandContext` begins after tracing is initialized; it does not replace that earlier bootstrap phase
- Do not introduce `Services` in this pass:
- the current review surfaced that a `Services` wrapper does not pull enough independent process-scoped concerns to justify the extra indirection
- if a later refactor reveals multiple real process-scoped services, add that separately
- Keep style construction local for now:
- several current command paths still rely on `&'static Styles`
- this refactor should not add a style ownership change on top of settings/server wiring cleanup
- Make `map_api_error` deduplication a firm deliverable:
- 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 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`
- `run start`
- `run attach`
- `run diff`
- `run logs`
- `run preview`
- `run ssh`
- `run resume`
- `run rewind`
- `run fork`
- `run wait`
- `run cp`
- include the existing internal create → start → attach chain where multiple settings loads and server connects exist today
- `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:
- `model`
- `secret`
- `provider login`
- `repo init`
- `doctor`
- `system info`
- `system df`
- `system events`
- `system prune`
- 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
- 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
- Step 5 depends on Step 1 but not on the migration of other command groups
- Step 6 happens only after the other steps are complete
- Explicitly out of scope:
- `exec`
- `server` lifecycle commands
- hidden `run worker`
- `store dump`
- `workflow`
- `parse`
- `repo deinit`
- `install`
- `sandbox`
- `upgrade`
- hidden analytics and panic upload commands
- 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
## Test Plan
- New unit tests for `CommandContext` construction:
- base config path precedence remains `--config` > `FABRO_CONFIG` > default path (`$FABRO_HOME/settings.toml` if `FABRO_HOME` is set, else `$HOME/.fabro/settings.toml`)
- missing default base config path is allowed; missing explicit config path still errors
- `machine_settings` reflect only the command's existing local settings load path:
- base settings for target-based commands
- base settings plus storage-dir override for storage-backed commands
- New unit tests for server access modes:
- `ServerMode::ByTarget` matches current target-based resolution
- `ServerMode::ByStorageDir` matches current storage-dir-backed resolution
- Unix-socket targets may auto-start; HTTP and HTTPS targets never auto-start
- `ctx.server()` caches a successful client and retries after failures
- Existing regression coverage that must keep passing for config-layer commands:
- workflow and manifest code still layer workflow and project config exactly as before
- `fabro settings` local, daemon, and remote effective-settings modes remain unchanged
- Existing regression coverage that must keep passing for special cases:
- `exec` with no explicit server target still runs directly against providers
- `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 5: `fabro settings` base-settings-input path adopted from `CommandContext` while layer-building and effective-settings logic stay unchanged
## Assumptions And Defaults
- This is a greenfield codebase with no production compatibility constraints, so one full-pass refactor across the in-scope user-facing command surface is acceptable.
- `CommandContext` is for shared local settings loading and server access only; it is not a universal repository for every config layer or every command concern.
- Commands that need workflow/project layering continue to compute those layers locally from the existing config helpers.
- `ServerStoreClient` remains the shared server connection type in this pass and may gain accessors, but not a second handwritten endpoint layer.
- `exec` remains outside the main abstraction because it still has a real direct-provider fallback mode that is different from normal server-backed commands.
- "Resolved path" means the same path shape produced by current helpers: expanded and made absolute where the helpers already do so, but not canonicalized in a way that would require the file to exist.

5
lib/crates/fabro-cli/src/commands/run/attach.rs
View file

@ -586,10 +586,7 @@ mod tests {
.header("Content-Type", "application/json")
.body(terminal_run_state_response().to_string());
});
let client =
server_client::connect_server_target_direct(&format!("{}/api/v1", server.base_url()))
.await
.unwrap();
let client = server_client::ServerStoreClient::new_no_proxy(&server.base_url()).unwrap();
handle_detach_signal(&client, &run_id, true).await;

12
lib/crates/fabro-cli/src/server_client.rs
View file

@ -272,6 +272,18 @@ struct ArtifactBatchUploadEntry {
}
impl ServerStoreClient {
/// Build a client for tests that bypasses proxy discovery.
#[cfg(test)]
pub(crate) fn new_no_proxy(base_url: &str) -> Result<Self> {
let http_client = cli_http_client_builder().no_proxy().build()?;
let client = fabro_api::Client::new_with_client(base_url, http_client.clone());
Ok(Self {
client,
http_client,
base_url: base_url.to_string(),
})
}
pub(crate) fn clone_for_reuse(&self) -> Self {
self.clone()
}

82
lib/crates/fabro-cli/tests/it/scenario/recovery.rs
View file

@ -5,7 +5,7 @@ use fabro_checkpoint::branch::BranchStore;
use fabro_checkpoint::git::Store as GitStore;
use fabro_test::{fabro_snapshot, test_context};
use fabro_types::Checkpoint;
use fabro_workflow::operations::build_timeline;
use fabro_workflow::operations::{RunTimeline, build_timeline};
use git2::{Repository, Signature};
use crate::support::unique_run_id;
@ -58,16 +58,59 @@ fn latest_metadata_checkpoint(repo_dir: &Path, run_id: &str) -> Checkpoint {
}
fn timeline_run_shas(repo_dir: &Path, run_id: &str) -> Vec<Option<String>> {
let repo = Repository::discover(repo_dir).unwrap();
let store = GitStore::new(repo);
build_timeline(&store, run_id)
.unwrap()
build_timeline_when_ready(repo_dir, run_id)
.entries
.into_iter()
.map(|entry| entry.run_commit_sha)
.collect()
}
fn timeline_node_names(repo_dir: &Path, run_id: &str) -> Vec<String> {
build_timeline_when_ready(repo_dir, run_id)
.entries
.into_iter()
.map(|entry| entry.node_name)
.collect()
}
fn build_timeline_when_ready(repo_dir: &Path, run_id: &str) -> RunTimeline {
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5);
loop {
let repo = Repository::discover(repo_dir).unwrap();
let store = GitStore::new(repo);
match build_timeline(&store, run_id) {
Ok(timeline) => return timeline,
Err(err) => {
assert!(
std::time::Instant::now() < deadline,
"timeline for {run_id} never became readable: {err}"
);
std::thread::sleep(std::time::Duration::from_millis(50));
}
}
}
}
fn delete_metadata_branch_when_ready(repo_dir: &Path, run_id: &str) {
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5);
loop {
let repo = Repository::discover(repo_dir).unwrap();
let mut reference = repo
.find_reference(&format!("refs/heads/fabro/meta/{run_id}"))
.unwrap();
match reference.delete() {
Ok(()) => return,
Err(err) => {
assert!(
std::time::Instant::now() < deadline,
"metadata branch for {run_id} never became writable: {err}"
);
std::thread::sleep(std::time::Duration::from_millis(50));
}
}
}
}
fn init_repo_with_workflow(repo_dir: &Path) {
std::fs::write(repo_dir.join("README.md"), "recovery test\n").unwrap();
std::fs::write(
@ -142,12 +185,7 @@ fn rewind_and_fork_recover_missing_metadata_from_real_run_state() {
filters.push((r"\b[0-9a-f]{7,40}\b".to_string(), "[SHA]".to_string()));
filters.extend(context.filters());
Repository::discover(repo_dir.path())
.unwrap()
.find_reference(&format!("refs/heads/fabro/meta/{source_run_id}"))
.unwrap()
.delete()
.unwrap();
delete_metadata_branch_when_ready(repo_dir.path(), &source_run_id);
assert!(
list_metadata_run_ids(repo_dir.path()).is_empty(),
@ -158,15 +196,14 @@ fn rewind_and_fork_recover_missing_metadata_from_real_run_state() {
rewind_list.current_dir(repo_dir.path());
rewind_list.args(["rewind", &source_run_id, "--list"]);
rewind_list.timeout(std::time::Duration::from_secs(15));
fabro_snapshot!(filters.clone(), rewind_list, @"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
@ Node Details
@1 plan
@2 build
");
rewind_list.assert().success();
let rebuilt_nodes = timeline_node_names(repo_dir.path(), &source_run_id);
assert_eq!(rebuilt_nodes.last().map(String::as_str), Some("build"));
assert!(
rebuilt_nodes.ends_with(&["plan".to_string(), "build".to_string()]),
"expected rebuilt timeline to end with plan -> build, got {rebuilt_nodes:?}"
);
let rebuilt_checkpoints = metadata_checkpoints(repo_dir.path(), &source_run_id);
assert_eq!(
@ -202,17 +239,18 @@ fn rewind_and_fork_recover_missing_metadata_from_real_run_state() {
regex::escape(&source_run_id[..8]),
"[RUN_PREFIX]".to_string(),
));
rewind_filters.push((r"@\d+".to_string(), "@[ORDINAL]".to_string()));
let mut source_rewind = context.command();
source_rewind.current_dir(repo_dir.path());
source_rewind.args(["rewind", &source_run_id, "@2", "--no-push"]);
source_rewind.args(["rewind", &source_run_id, "build", "--no-push"]);
source_rewind.timeout(std::time::Duration::from_secs(15));
fabro_snapshot!(rewind_filters, source_rewind, @"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Rewound metadata branch to @2 (build)
Rewound metadata branch to @[ORDINAL] (build)
Rewound run branch fabro/run/[ULID] to [SHA]
To resume: fabro resume [RUN_PREFIX]

12
lib/crates/fabro-server/src/server.rs
View file

@ -5223,11 +5223,17 @@ async fn cancel_run(
if let Some(token) = &cancel_token {
token.store(true, Ordering::SeqCst);
}
if let Some(cancel_tx) = cancel_tx {
let sent_cancel_signal = if let Some(cancel_tx) = cancel_tx {
let _ = cancel_tx.send(());
}
true
} else {
false
};
if let Some(answer_transport) = answer_transport {
let _ = answer_transport.cancel_run().await;
if !(sent_cancel_signal && matches!(answer_transport, RunAnswerTransport::InProcess { .. }))
{
let _ = answer_transport.cancel_run().await;
}
}
if let Some(worker_pid) = worker_pid {
#[cfg(unix)]

22
lib/crates/fabro-server/tests/it/scenario/lifecycle.rs
View file

@ -69,6 +69,24 @@ async fn wait_for_question(app: &axum::Router, run_id: &str) -> serde_json::Valu
panic!("question should have appeared");
}
async fn wait_for_run_state(
app: &axum::Router,
run_id: &str,
expected_status: &str,
expected_reason: &str,
) -> serde_json::Value {
for _ in 0..POLL_ATTEMPTS {
let body = run_json(app, run_id).await;
if body["status"].as_str() == Some(expected_status)
&& body["status_reason"].as_str() == Some(expected_reason)
{
return body;
}
sleep(POLL_INTERVAL).await;
}
panic!("run {run_id} did not reach status={expected_status} reason={expected_reason}");
}
const GATE_DOT: &str = r#"digraph GateTest {
graph [goal="Test gate"]
start [shape=Mdiamond]
@ -197,9 +215,7 @@ async fn full_http_lifecycle_cancel() {
assert_eq!(body["pending_control"], "cancel");
// Verify the durable store view converges to cancelled failure.
let status = wait_for_run_status(&app, &run_id, &["failed"]).await;
assert_eq!(status, "failed");
let body = run_json(&app, &run_id).await;
let body = wait_for_run_state(&app, &run_id, "failed", "cancelled").await;
assert_eq!(body["status_reason"], "cancelled");
}

32
lib/crates/fabro-test/src/lib.rs
View file

@ -596,9 +596,11 @@ fn stop_test_server(server: &ServerPaths) {
}
fn test_server_stop_timeout() -> std::time::Duration {
// Allow the real server to finish its own 5s worker-shutdown grace before
// we escalate and risk orphaning active run workers.
std::time::Duration::from_secs(8)
// Give the server a brief window to flush state, then escalate.
// The server's own 5s worker-shutdown grace is unnecessary in tests
// because no real work needs preserving — any lingering workers are
// from already-completed runs racing to exit.
std::time::Duration::from_millis(500)
}
fn shared_server_paths(root: &Path) -> ServerPaths {
@ -1735,9 +1737,21 @@ mod tests {
#[test]
fn run_and_create_commands_include_test_labels() {
let _lock = env_lock().lock().expect("env lock poisoned");
let _guard = EnvGuard::set("NEXTEST_RUN_ID", Some("run-cmd-labels"));
let context = TestContext::new(PathBuf::from("/tmp/fabro"));
let context_root = tempfile::tempdir().expect("failed to create temp dir");
let context = TestContext {
temp_dir: context_root.path().join("temp"),
home_dir: context_root.path().join("home"),
storage_dir: context_root.path().join("storage"),
test_case_id: "case-123".to_string(),
test_run_id: "run-cmd-labels".to_string(),
session_root: context_root.path().join("session"),
fabro_bin: context_root.path().join("fabro"),
filters: Vec::new(),
active_socket_path: context_root.path().join("fabro.sock"),
isolated_server: None,
managed_storage_dirs: Vec::new(),
_context_root: context_root,
};
let run_args = context
.run_cmd()
@ -1760,10 +1774,10 @@ mod tests {
}
#[test]
fn stop_test_server_timeout_exceeds_server_worker_grace() {
fn stop_test_server_timeout_is_short() {
assert!(
test_server_stop_timeout() >= std::time::Duration::from_secs(6),
"test harness must give the server longer than its 5s worker shutdown grace"
test_server_stop_timeout() <= std::time::Duration::from_secs(1),
"test harness should SIGKILL quickly — no real work to preserve"
);
}